----------------------------------------------------------- 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 > >
-- _____________________________________________________________________ -- Bandwidth and Colocation Provided by http://www.api-digital.com -- asterisk-dev mailing list To UNSUBSCRIBE or update options visit: http://lists.digium.com/mailman/listinfo/asterisk-dev
