[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 5:

(4 comments)

https://gerrit.asterisk.org/#/c/5341/5/main/rtp_engine.c
File main/rtp_engine.c:

Line 384: 		ao2_lock(instance);
> Is this locking necessary?  I don't think there should be a valid case wher
Yes this lock is necessary.  The comment above the lock says why it is needed.  The TURN code and the ast_rtp_asterisk.c scheduler needs it locked to shut down because there are other threads involved.  The other threads do not necessarily have a ref to the instance.  In the TURN case that thread cannot have a ref.


https://gerrit.asterisk.org/#/c/5341/5/res/res_rtp_asterisk.c
File res/res_rtp_asterisk.c:

Line 2962: 		AST_SCHED_DEL(rtp->sched, rtp->red->schedid);
> It seems like we shouldn't have to unlock around the AST_SCHED_DEL call if 
There may or may not be a ref given to rtp->red.  In this case a ref was not given because AST_SCHED_DEL() is the wrong macro to use if the scheduled callback is given a ref.  Deleting a scheduled entry will block until the scheduler entry is removed or an active callback completes.  If that callback needs to get the instance lock then we are deadlocked.


Line 2981: 		pj_turn_sock_destroy(rtp->turn_rtp);
> Possibly same here - again, if something async is happening related to turn
TURN cannot have a reference to instance as pjproject doesn't know anything about ao2 objects.


Line 5329: 					ao2_unlock(instance);
> Everywhere we unlock within a callback function on the rtp_engine instance 
No.  The locking is to prevent reentrancy corrupting the instance structures and not to prevent instance from going away.  We are depending upon the caller to be holding a valid reference or at the least a valid share of a reference to instance.  The scenario you are describing is a ref counting issue and not a locking issue with instance.


-- 
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: 5
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-Reviewer: Sebastian Gutierrez <scgm11 at gmail.com>
Gerrit-HasComments: Yes