rtp_engine/res_rtp_asterisk: Fix RTP struct reentrancy crashes.
authorRichard Mudgett <rmudgett@digium.com>
Wed, 22 Mar 2017 21:05:49 +0000 (16:05 -0500)
committerRichard Mudgett <rmudgett@digium.com>
Wed, 19 Apr 2017 18:40:57 +0000 (13:40 -0500)
commitd165079cbcb51c94ce729b97f25b42cdbd7b443e
tree4aa762d04c6f313acd7b3d5333af1d6dfcbcc1d7
parentb84abac1449c56d6c9f781525a533691b326ad8c
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.

* res_rtp_asterisk.c: Avoid deadlock when trying to stop the TURN
ioqueue_worker_thread().  We cannot hold the instance lock when trying to
create or shut down the worker thread without a risk of deadlock.

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(): Protect against an
uninitialized/null remote_address after calling
update_address_with_ice_candidate().

* res_rtp_asterisk.c: Eliminate the chance of ice_reset_session()
destroying and setting the rtp->ice pointer to NULL while other threads
are using it by adding an ao2 wrapper around the PJPROJECT ice pointer.
Now when we have to unlock the RTP instance object to call a PJPROJECT ICE
function we will hold a ref to the wrapper.  Also added some rtp->ice NULL
checks after we relock the RTP instance and have to do something with the
ICE structure.

ASTERISK-26835 #close
ASTERISK-26853 #close

Change-Id: I780b39ec935dcefcce880d50c1a7261744f1d1b4
main/rtp_engine.c
res/res_rtp_asterisk.c