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

[Matthew Fredrickson]

Matthew Fredrickson 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: Code-Review-1

(7 comments)

I got about 20% through the review yesterday, so it could be that my questions would have been answered by further research.  My day today is booked up, but I'll try to make more progress on it if I have some time between things.  I'm gonna post what I have now for the moment, flawed as it might be.

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 where the instance destructor is called and something else is interacting with the rtp instance.  This would mean that some piece of code is interacting with the instance without grabbing a proper ao2_ref() of it, no?


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

Line 2477: 		status = pj_ice_sess_send_data(rtp->ice,
Unlocking around pj_ice_sess_send_data(). Yuck. Yuck. Yuck. Yuck. Yuck.  Did I mention yuck.


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 the scheduler entry has a ao2_ref() reference to the rtp instance here, and dereferences it properly after the schedule operation.  This destroy callback should only be called when all references to the rtp instance have been relinquished, which should include the one held by the scheduled item of work.


Line 2981: 		pj_turn_sock_destroy(rtp->turn_rtp);
Possibly same here - again, if something async is happening related to turn_rtp, we shouldn't have to unlock and relock here if the code in question has a proper reference.


Line 2993: 		ao2_unlock(instance);
Again...


Line 3008: 		ao2_unlock(instance);
Again...


Line 5329: 					ao2_unlock(instance);
Everywhere we unlock within a callback function on the rtp_engine instance we have to acquire a reference to the instance.

If ast_rtp_prop_set() was called, and in the middle of that call another thread does an ast_rtp_instance_destroy, releasing the last reference, you could have the instance destructor start executing here, essentially starting the destruction of the rtp_instance in the middle of your ast_rtp_prop_set function.


-- 
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