[asterisk-dev] [Code Review] 4114: Prevent stringfields from accumulating unused memory

[rmudgett]

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


1) In stringfields.h:ast_string_field_ptr_set_by_fields(), the __p__ and ptr pointers are the same by initialization so the test for *__p__ != *ptr is always false and will not release the old string value when __ast_string_field_alloc_space() allocates space for the new string value.  I think this is the primary leak.

2) In utils.c:__ast_string_field_ptr_grow(), the increase of pool->used doesn't seem right.  It should be increased to keep alignment similar to utils.c:__ast_string_field_alloc_space().

3) I think a check needs to be added to utils.c:__ast_string_field_ptr_build_va() for the case when the string created by vsnprintf() is empty so the pool string can be set to the constant __ast_string_field_empty pointer.  (Like is done in stringfields.h:ast_string_field_ptr_set_by_fields())

4) All of these fixes would apply to v1.8 as well.


/branches/11/main/utils.c
<https://reviewboard.asterisk.org/r/4114/#comment24140>

    This should be reverted.  ptr is the string being released from the pool and __ast_string_field_empty can never be in a pool buffer by definition.



/branches/11/main/utils.c
<https://reviewboard.asterisk.org/r/4114/#comment24141>

    Doing this check for every pool is overkill when you are only releasing one string from one pool.  Once the string is found in a pool you don't need to continue looking in any remaining pools.
    
    Setting pool->used = 0 is a good catch for the first pool as this fixes reclaiming the space of the first pool.


- rmudgett


On Oct. 27, 2014, 3:20 a.m., Corey Farrell wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4114/
> -----------------------------------------------------------
> 
> (Updated Oct. 27, 2014, 3:20 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24307
>     https://issues.asterisk.org/jira/browse/ASTERISK-24307
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> Any time a stringfield is blanked it currently prevents any currently allocated memory from being freed.  If a stringfield is repeatedly set to blank then set to a non-blank value, it causes new pools to be continuously allocated and never freed.
> 
> I'm unsure if the loop can be optimized, maybe the break can be re-added to the original location on the condition that ptr == __ast_string_field_empty?
> 
> 
> Diffs
> -----
> 
>   /branches/11/main/utils.c 426232 
> 
> Diff: https://reviewboard.asterisk.org/r/4114/diff/
> 
> 
> Testing
> -------
> 
> Manual test using https://github.com/elessard1/asterisk-lab/blob/master/examples/lab_stringfields_leak.c to verify that old pools are now freed.
> 
> Full testsuite against Asterisk 13.
> 
> 
> Thanks,
> 
> Corey Farrell
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20141029/4e9851bd/attachment.html>