[asterisk-dev] [Code Review] 2839: Fix memory corruption race condition.

[rmudgett]

> On Sept. 10, 2013, 1:17 p.m., Matt Jordan wrote:
> > /branches/12/main/core_unreal.c, lines 468-471
> > <https://reviewboard.asterisk.org/r/2839/diff/1/?file=45765#file45765line468>
> >
> >     We need to bump the reference count on ast here prior to unlocking it, particularly since we queue a frame on it later in this method after it has been unlocked/locked.
> >     
> >     In particular, since my_chan is ast, we could theoretically nuke the channel ourselves if, while we have it locked, something else decrements the reference count to 1 and we deref it to 0 on line 492.
> 
> rmudgett wrote:
>     There is no need for us to bump the ast channel reference.  The caller is supposed to have one since they gave us the pointer.
> 
> Matt Jordan wrote:
>     I don't think relying on every caller of ast_indicate is a good idea. Prior to this patch, callers could rely on the channel being locked by ast_indicate and remaining locked. Your patch changes that behavior.
>     
>     This feels like it will lead to interesting bug reports without some defensive accounting for opening race conditions on the channel.

This patch does not change the unlocking of ast.  The unreal_queue_frame() call previously unlocked the ast channel.  The deadlock avoidance code in local channels has required unlocking the ast channel for some time.  (I think, somewhere around the v1.8.6 timeframe.)  I previously went through the code to ensure that ast_indicate() is never called with the channel already locked.  If ast_indicate() is called with the channel already locked on a local channel then deadlock is possible.


- rmudgett


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviewboard.asterisk.org/r/2839/#review9641
-----------------------------------------------------------


On Sept. 10, 2013, 12:42 a.m., rmudgett wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2839/
> -----------------------------------------------------------
> 
> (Updated Sept. 10, 2013, 12:42 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-22221
>     https://issues.asterisk.org/jira/browse/ASTERISK-22221
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> The masquerade super test is failing on v12 with high fence violations and crashing.  The fence violations are showing that party id allocated memory strings are somehow getting corrupted in the bridge_reconfigured_connected_line_update() function.  The invalid string values happen to be the freed memory fill pattern.
> 
> After much puzzling, I deduced that the bridge_reconfigured_connected_line_update() is copying a string out of the source channel's caller party id struct just as another thread is updating it with a new value.  The copying thread is using the old string pointer being freed by the updating thread.  A search of the code found the unreal_colp_redirect_indicate() routine updating the caller party id's without holding the channel lock.
> 
> A latent bug in v1.8 and v11 hatched in v12 because of the bridging and connected line changes. :)
> 
> 
> Diffs
> -----
> 
>   /branches/12/main/core_unreal.c 398732 
> 
> Diff: https://reviewboard.asterisk.org/r/2839/diff/
> 
> 
> Testing
> -------
> 
> Its hard to prove a negative.
> I manually did a local chain of 300 several times and MALLOC_DEBUG did not complain of any high fence violations.
> 
> 
> Thanks,
> 
> rmudgett
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20130910/61f451b0/attachment.htm>