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

(2 comments)

Responding to mmichelson's questions so it doesn't seem like I've ignored them.

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

PS6, Line 1980: /* PJPROJECT ICE callback */
              : static void ast_rtp_on_ice_rx_data(pj_ice_sess *ice, unsigned comp_id, unsigned transport_id, void *pkt, pj_size_t size, const pj_sockaddr_t *src_addr, unsigned src_addr_len)
              : {
> Why isn't the instance lock grabbed in this function?
Technically the instance lock should be grabbed because the flags are bitfields in the same unsigned int.  I'm not really sure it is necessary though.

The rtp->passthrough flag is always modified and checked by the same thread.

The rtp->rtp_passthrough and rtp->rtcp_passthrough flags are done by a different thread for TURN and I don't think there are very many packets involved in that path.


Line 1998: static pj_status_t ast_rtp_on_ice_tx_pkt(pj_ice_sess *ice, unsigned comp_id, unsigned transport_id, const void *pkt, pj_size_t size, const pj_sockaddr_t *dst_addr, unsigned dst_addr_len)
> Same here.
Here we have a chance to deadlock with a pjproject group lock when pj_turn_sock_sendto() is called.


-- 
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: 6
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: Mark Michelson <mmichelson 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