[Asterisk-code-review] chan iax2: Prevent deadlock between hangup and sending lagrq... (asterisk[master])

[Richard Mudgett]

Richard Mudgett has posted comments on this change.

Change subject: chan_iax2: Prevent deadlock between hangup and sending lagrq/ping
......................................................................


Patch Set 6: Code-Review-1

(2 comments)

https://gerrit.asterisk.org/#/c/169/6/channels/chan_iax2.c
File channels/chan_iax2.c:

Line 2071: 
         : 
         : 
         : 
         : 
A recent fix in the scheduler made AST_SCHED_DEL_SPINLOCK() no longer work and no longer possible to work.  The two uses of AST_SCHED_DEL_SPINLOCK() here are the only places left in the system that reference this macro.  The definition of AST_SCHED_DEL_SPINLOCK() needs be deleted from include/asterisk/sched.h as a result to prevent anyone else from using it.  The creation of AST_SCHED_DEL_SPINLOCK() was a bandaid anyway.

This patch will need to be done on master and v13 but not v11.

This deadlock can be easily fixed using the code fragment below:

static void iax2_destroy_helper(...)
{
int orig_pingid;
int orig_lagid;

...

/* No more pings or lagrq's */
orig_pingid = pvt->pingid;
pvt->pingid = DONT_RESCHEDULE;
orig_lagid = pvt->lagid;
pvt->lagid = DONT_RESCHEDULE;
ast_mutex_unlock(&iaxsl[pvt->callno]);
AST_SCHED_DEL(sched, orig_pingid);
AST_SCHED_DEL(sched, orig_lagid);
ast_mutex_lock(&iaxsl[pvt->callno]);
...


Line 1688: 		if (!iaxs[callno] || (iaxs[callno] && iaxs[callno]->destroy_initiated)) {
You cannot access the contents of iaxs[] without holding the iaxsl[] lock.  Doing so can result in dereferencing an incomplete update to the iaxs[] pointer with resulting hard to debug crash because it looks like memory corruption.

This comment is superseded by the other comment showing a better fix.


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

Gerrit-MessageType: comment
Gerrit-Change-Id: I03bec1fc8faacb89630269e935fa667c6d6c080c
Gerrit-PatchSet: 6
Gerrit-Project: asterisk
Gerrit-Branch: master
Gerrit-Owner: Y Ateya <y.ateya at starkbits.com>
Gerrit-Reviewer: George Joseph <george.joseph at fairview5.com>
Gerrit-Reviewer: Joshua Colp <jcolp at digium.com>
Gerrit-Reviewer: Matt Jordan <mjordan at digium.com>
Gerrit-Reviewer: Richard Mudgett <rmudgett at digium.com>
Gerrit-Reviewer: Y Ateya <y.ateya at starkbits.com>
Gerrit-HasComments: Yes