From 6348add6645975baf2610a7afb9994d79fc0603c Mon Sep 17 00:00:00 2001 From: Richard Mudgett Date: Fri, 1 Jul 2011 21:07:22 +0000 Subject: [PATCH] Better way to get chan and pvt lock for issue ASTERISK-17431. Redoes -r308945 for issue ASTERISK-17431 deadlock fix for sip_set_udptl_peer() and sip_set_rtp_peer(). * Lock the channels in the defined order and avoid the need for a deadlock avoidance loop. * Lock the channel before getting the pointer to the private structure to be sure that the pointer will not change due to a masquerade or channel hangup. * To preserve sanity, check that chan and p->owner are the same. (Pointer rearangements should not happen without the protection of locks because bad things tend to happen otherwise.) git-svn-id: https://origsvn.digium.com/svn/asterisk/branches/1.8@326144 65c4cc65-6c06-0410-ace0-fbb531ad65f3 --- channels/chan_sip.c | 54 ++++++++++++++++++++------------------------- 1 file changed, 24 insertions(+), 30 deletions(-) diff --git a/channels/chan_sip.c b/channels/chan_sip.c index 077d84efe5..f1a35a4d59 100644 --- a/channels/chan_sip.c +++ b/channels/chan_sip.c @@ -28087,25 +28087,23 @@ static struct ast_udptl *sip_get_udptl_peer(struct ast_channel *chan) static int sip_set_udptl_peer(struct ast_channel *chan, struct ast_udptl *udptl) { struct sip_pvt *p; - + + /* Lock the channel and the private safely. */ + ast_channel_lock(chan); p = chan->tech_pvt; if (!p) { + ast_channel_unlock(chan); return -1; } - /* - * Lock both the pvt and it's owner safely. - */ sip_pvt_lock(p); - while (p->owner && ast_channel_trylock(p->owner)) { - sip_pvt_unlock(p); - usleep(1); - sip_pvt_lock(p); - } - - if (!p->owner) { + if (p->owner != chan) { + /* I suppose it could be argued that if this happens it is a bug. */ + ast_debug(1, "The private is not owned by channel %s anymore.\n", chan->name); sip_pvt_unlock(p); + ast_channel_unlock(chan); return 0; } + if (udptl) { ast_udptl_get_peer(udptl, &p->udptlredirip); } else { @@ -28124,8 +28122,8 @@ static int sip_set_udptl_peer(struct ast_channel *chan, struct ast_udptl *udptl) } /* Reset lastrtprx timer */ p->lastrtprx = p->lastrtptx = time(NULL); - ast_channel_unlock(p->owner); sip_pvt_unlock(p); + ast_channel_unlock(chan); return 0; } @@ -28232,10 +28230,21 @@ static int sip_set_rtp_peer(struct ast_channel *chan, struct ast_rtp_instance *i struct sip_pvt *p; int changed = 0; + /* Lock the channel and the private safely. */ + ast_channel_lock(chan); p = chan->tech_pvt; if (!p) { + ast_channel_unlock(chan); return -1; } + sip_pvt_lock(p); + if (p->owner != chan) { + /* I suppose it could be argued that if this happens it is a bug. */ + ast_debug(1, "The private is not owned by channel %s anymore.\n", chan->name); + sip_pvt_unlock(p); + ast_channel_unlock(chan); + return 0; + } /* Disable early RTP bridge */ if ((instance || vinstance || tinstance) && @@ -28244,25 +28253,10 @@ static int sip_set_rtp_peer(struct ast_channel *chan, struct ast_rtp_instance *i return 0; } - /* - * Lock both the pvt and it's owner safely. - */ - sip_pvt_lock(p); - while (p->owner && ast_channel_trylock(p->owner)) { - sip_pvt_unlock(p); - usleep(1); - sip_pvt_lock(p); - } - - if (!p->owner) { - sip_pvt_unlock(p); - return 0; - } - if (p->alreadygone) { /* If we're destroyed, don't bother */ - ast_channel_unlock(p->owner); sip_pvt_unlock(p); + ast_channel_unlock(chan); return 0; } @@ -28270,8 +28264,8 @@ static int sip_set_rtp_peer(struct ast_channel *chan, struct ast_rtp_instance *i that are known to be behind a NAT, then stop the process now */ if (nat_active && !ast_test_flag(&p->flags[0], SIP_DIRECT_MEDIA_NAT)) { - ast_channel_unlock(p->owner); sip_pvt_unlock(p); + ast_channel_unlock(chan); return 0; } @@ -28313,8 +28307,8 @@ static int sip_set_rtp_peer(struct ast_channel *chan, struct ast_rtp_instance *i } /* Reset lastrtprx timer */ p->lastrtprx = p->lastrtptx = time(NULL); - ast_channel_unlock(p->owner); sip_pvt_unlock(p); + ast_channel_unlock(chan); return 0; } -- 2.47.2