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

[rmudgett]

> On Oct. 29, 2014, 6:55 p.m., rmudgett wrote:
> > 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.
> 
> Corey Farrell wrote:
>     1) I think I've fixed this.
>     2) To be honest I'm not sure how to fix this.
>     3) Fixed.
>     4) If this review is complete before the final 1.8 bug fix release.

2) I'm not sure if it really needs to be fixed as I didn't find any reported issues about this on architectures that are fussy about alignment.
4) I'd say it is too late since the final RC is expected to be made today.


- rmudgett


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


On Nov. 3, 2014, 7:29 a.m., Corey Farrell wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4114/
> -----------------------------------------------------------
> 
> (Updated Nov. 3, 2014, 7:29 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 427111 
>   /branches/11/include/asterisk/stringfields.h 427111 
> 
> 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/20141103/aaa60ee5/attachment-0001.html>