[asterisk-dev] [Code Review] Make AST_LIST_REMOVE safer

[Terry Wilson]

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



/branches/1.8/include/asterisk/linkedlists.h
<https://reviewboard.asterisk.org/r/1321/#comment7701>

    EXPLANATION: If elm == NULL and the list is empty, don't treat it like a match.



/branches/1.8/include/asterisk/linkedlists.h
<https://reviewboard.asterisk.org/r/1321/#comment7702>

    EXPLANATION: Here, if elm == NULL, then we won't match until we actually hit the end of the list which is a NULL. In this case, we end up not setting curelm = NULL, so we would fall into the if and set __res to the last element in the list, and then access NULL->field.next.
    
    At this point elm == curelm->field.next, but I don't test elm && curlem becuase elm is passed into the macro and might be something like &static_element which will throw warnings because it will always evaluate to true.



/branches/1.8/include/asterisk/linkedlists.h
<https://reviewboard.asterisk.org/r/1321/#comment7703>

    EXPLANATION: Don't modify an unmatched element.


- Terry


On July 15, 2011, 1:36 p.m., Terry Wilson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/1321/
> -----------------------------------------------------------
> 
> (Updated July 15, 2011, 1:36 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> AST_LIST_REMOVE sometimes modifies elements that are passed in for comparison, even if they aren't actually found in the list. There are three cases where this can happen. 1) The element is set to NULL in which case Asterisk will crash or 2) The element is a previously freed element in which case Asterisk may crash or 3) The element is a valid element, but not in the list in which case Asterisk will happily set that elements 'next' pointer to NULL effectively truncating whatever list it may have been a member of.
> 
> I will make comments for each of the changes in-line.
> 
> 
> This addresses bug ASTERISK-17917.
>     https://issues.asterisk.org/jira/browse/ASTERISK-17917
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/include/asterisk/linkedlists.h 328380 
>   /branches/1.8/tests/test_linkedlists.c PRE-CREATION 
> 
> Diff: https://reviewboard.asterisk.org/r/1321/diff
> 
> 
> Testing
> -------
> 
> This also adds a set of linked list tests. Asterisk would crash when the test was run on the old code, and does not do so with the new. All tests pass.
> 
> 
> Thanks,
> 
> Terry
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20110715/ed72fffa/attachment-0001.htm>