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

[Richard Mudgett]

Richard Mudgett has posted comments on this change. ( https://gerrit.asterisk.org/5341 )

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


Patch Set 1:

> My concern is that throwing locking into everything like this is
 > going to introduce unexpected locking inversions and patterns. I
 > think we should do the minimal needed to cover this particular
 > case, as we recently introduced it.

We did not "recently" introduce it.  The potential has actually been
around since v12 was released.  The particular crash demonstrated by
ASTERISK-26853 has been around since v12 was released.  The particular
crash demonstrated by ASTERISK-26835 was introduced about 8 months ago.
We got away with it in v11 and earlier because there was really only
one thread messing with the RTP instance structure.  With the new
bridging architecture there are two threads involved in native RTP
bridging.  ast_rtp_read/bridge_p2p_rtp_write() reads from one instance
and writes to the peer instance.  A reinvite that changes codecs, ICE, or
DTLS might cause reentrancy issues for ast_rtp_read/bridge_p2p_rtp_write().
chan_pjsip/res_pjsip introduced more threads messing with the RTP instance
structure.

Trying to protect the RTP instance structure with the channel lock is not
a good thing.  The channel lock is two architectural layers above the RTP
layer that it isn't obvious that it would need to be locked.  For
ast_rtp_read/bridge_p2p_rtp_write() the peer channel is not even available.

So yes the RTP instance locking is necessary to protect from reentrancy.

-- 
To view, visit https://gerrit.asterisk.org/5341
To unsubscribe, visit https://gerrit.asterisk.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I780b39ec935dcefcce880d50c1a7261744f1d1b4
Gerrit-PatchSet: 1
Gerrit-Project: asterisk
Gerrit-Branch: 13
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>
Gerrit-Reviewer: Matthew Fredrickson <creslin at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-HasComments: No