[asterisk-dev] [Code Review] 3752: astobj2: Fix race condition in ref_debug log

[Corey Farrell]

> On July 11, 2014, 11:01 p.m., Corey Farrell wrote:
> > /team/group/media_formats-reviewed-trunk/main/astobj2.c, lines 503-507
> > <https://reviewboard.asterisk.org/r/3752/diff/1/?file=62839#file62839line503>
> >
> >     This reorder will prevent ref_log from recording a double-free.
> >     
> >     Example:
> >     obj = ao2_alloc(..);
> >     ao2_ref(obj, -1);
> >     ao2_ref(obj, -1);
> >     
> >     Currently this block of code would produce an error in the ref_log.
> >     
> >     Also I know it sounds weird but I've seen:
> >     
> >     Thread 1: ao2_ref(obj, -1) == 2
> >     Thread 2: ao2_ref(obj, -1) == 1
> >     
> >     And for thread 2 (which runs the destructor) actually finishing first (and the last 2 entries to ref_log are reversed.
> 
> Matt Jordan wrote:
>     I'm not sure either of those are worth not fixing this:
>     
>     (1) A double free is highly likely to crash immediately, in which case, the ref debug log is only so useful. Plus, in a double free, you typically have the memory address that is now hostile, which can be correlated back to a ref debug log.
>     
>     (2) The out of order printing of destruction is possible, but is handled by the refcounter script since it looks for the destructor message (and will now actually get it)
>     
>     With the current code, we can get an admittedly large number of false positives, which isn't super useful.

The out of order destruction printing is actually worse with this update.  It causes the destructor to be 2nd to last, then the last entry is unref from 2.  This last entry appears as a skew and leak, separate from the rest of the history for that object.  So we will still get the same false positive, but in this case we'll be able to see the history of refs to know that it's a false positive.


- Corey


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


On July 11, 2014, 10:45 p.m., Matt Jordan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/3752/
> -----------------------------------------------------------
> 
> (Updated July 11, 2014, 10:45 p.m.)
> 
> 
> Review request for Asterisk Developers and Corey Farrell.
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> As Corey noticed, when an object's reference is changed rapidly the ref debug log will report the wrong reference. This is due to accessing the reference (which is not thread safe) prior to the reference count being changed. Since the change in reference count is returned by internal_ao2_ref, it can be used instead to determine what the current state of the object is.
> 
> 
> Diffs
> -----
> 
>   /team/group/media_formats-reviewed-trunk/main/astobj2.c 418435 
> 
> Diff: https://reviewboard.asterisk.org/r/3752/diff/
> 
> 
> Testing
> -------
> 
> Objects that were incorrectly reported as leaked no longer are.
> 
> 
> Thanks,
> 
> Matt Jordan
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20140712/173c6fd6/attachment-0001.html>