[Asterisk-code-review] rtp engine/res rtp asterisk: Fix RTP struct reentrancy crashes. (asterisk[master])

[Richard Mudgett]

Hello George Joseph, Anonymous Coward #1000019, Joshua Colp,

I'd like you to reexamine a change.  Please visit

    https://gerrit.asterisk.org/5343

to look at the new patch set (#5).

Change subject: rtp_engine/res_rtp_asterisk: Fix RTP struct reentrancy crashes.
......................................................................

rtp_engine/res_rtp_asterisk: Fix RTP struct reentrancy crashes.

The struct ast_rtp_instance has historically been indirectly protected
from reentrancy issues by the channel lock because early channel drivers
held the lock for really long times.  Holding the channel lock for such a
long time has caused many deadlock problems in the past.  Along comes
chan_pjsip/res_pjsip which doesn't necessarily hold the channel lock
because sometimes there may not be an associated channel created yet or
the channel pointer isn't available.

In the case of ASTERISK-26835 a pjsip serializer thread was processing a
message's SDP body while another thread was reading a RTP packet from the
socket.  Both threads wound up changing the rtp->rtcp->local_addr_str
string and interfering with each other.  The classic reentrancy problem
resulted in a crash.

In the case of ASTERISK-26853 a pjsip serializer thread was processing a
message's SDP body while another thread was reading a RTP packet from the
socket.  Both threads wound up processing ICE candidates in pjproject and
interfering with each other.  The classic reentrancy problem resulted in a
crash.

* rtp_engine.c: Make the ast_rtp_instance_xxx() calls lock the RTP
instance struct.

* rtp_engine.c: Make ICE and DTLS wrapper functions to lock the RTP
instance struct for the API call.

* res_rtp_asterisk.c: Lock the RTP instance to prevent a reentrancy
problem with rtp->rtcp->local_addr_str in the scheduler thread running
ast_rtcp_write().

* res_rtp_asterisk.c: Avoid deadlock when local RTP bridging in
bridge_p2p_rtp_write() because there are two RTP instance structs
involved.

* res_rtp_asterisk.c: Avoid deadlock when trying to stop scheduler
callbacks.  We cannot hold the instance lock when trying to stop a
scheduler callback.

* res_rtp_asterisk.c: Remove the lock in struct dtls_details and use the
struct ast_rtp_instance ao2 object lock instead.  The lock was used to
synchronize two threads to prevent a race condition between starting and
stopping a timeout timer.  The race condition is no longer present between
dtls_perform_handshake() and __rtp_recvfrom() because the instance lock
prevents these functions from overlapping each other with regards to the
timeout timer.

* res_rtp_asterisk.c: Remove the lock in struct ast_rtp and use the struct
ast_rtp_instance ao2 object lock instead.  The lock was used to
synchronize two threads using a condition signal to know when TURN
negotiations complete.

This patch exposed a race condition between a PJSIP serializer thread
setting up an ICE session in ice_create() and another thread reading RTP
packets.

* res_rtp_asterisk.c:ice_create(): Set the new rtp->ice pointer after we
have re-locked the RTP instance to prevent the other thread from trying to
process ICE packets on an incomplete ICE session setup.

A similar race condition is between a PJSIP serializer thread resetting up
an ICE session in ice_create() and the timer_worker_thread() processing
the completion of the previous ICE session.

* res_rtp_asterisk.c:ast_rtp_on_ice_complete(): Abort if we do not have an
ICE session.  Also protect against an uninitialized/null remote_address
after calling update_address_with_ice_candidate().

ASTERISK-26835 #close
ASTERISK-26853 #close

Change-Id: I780b39ec935dcefcce880d50c1a7261744f1d1b4
---
M main/rtp_engine.c
M res/res_rtp_asterisk.c
2 files changed, 811 insertions(+), 126 deletions(-)


  git pull ssh://gerrit.asterisk.org:29418/asterisk refs/changes/43/5343/5
-- 
To view, visit https://gerrit.asterisk.org/5343
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I780b39ec935dcefcce880d50c1a7261744f1d1b4
Gerrit-PatchSet: 5
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Anonymous Coward #1000019
Gerrit-Reviewer: George Joseph <gjoseph at digium.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>