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

[Corey Farrell]

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

(Updated Nov. 6, 2014, 3:05 a.m.)


Status
--

This change has been marked as submitted.


Review request for Asterisk Developers.


Changes
---

Committed in revision 427380


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

-- 
_
-- 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

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

[Corey Farrell]

> On Nov. 3, 2014, 1:12 p.m., rmudgett wrote:
> > Minor nit.
> > 
> > Does the patch still fix the reporter's memory leak?

Mostly, yes (as much as the original patch).  I'm not sure it can be fixed 
completely.  I tested with the reporters sip.conf.  Each peer originally 
allocated 2000 bytes for the stringfield's.  After 37 iterations of 'sip 
reload' the stringfield of all 500 peers added a new pool (allocating 8144 
bytes each).  The original allocations are never freed, however the memory 
never grows beyond the 2nd allocation (tested to 2000 reload iterations).  At 
least one field in the original pool is not being blanked during reload, 
causing the original memory pool to never free.  The existing code causes the 
memory usage to increase indefinitely, so this is a major improvement.


- Corey


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


On Nov. 3, 2014, 12:55 p.m., Corey Farrell wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4114/
> ---
> 
> (Updated Nov. 3, 2014, 12:55 p.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
> 
>

-- 
_
-- 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

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

[Corey Farrell]

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

(Updated Nov. 3, 2014, 1:53 p.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 (updated)
-

  /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

-- 
_
-- 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

Re: [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/#review13658
---

Ship it!


Minor nit.

Does the patch still fix the reporter's memory leak?


/branches/11/include/asterisk/stringfields.h


Use __p__ instead of ptr to reduce potential confusion here because of the 
mixed use of __p__ and ptr.


- rmudgett


On Nov. 3, 2014, 11:55 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, 11:55 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
> 
>

-- 
_
-- 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

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

[Corey Farrell]

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

(Updated Nov. 3, 2014, 12:55 p.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 (updated)
-

  /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

-- 
_
-- 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

Re: [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
> 
>

-- 
_
-- 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

Re: [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/#review13656
---



/branches/11/include/asterisk/stringfields.h


Use
ast_string_field target = *__p__;
to avoid any possible need for a cast.




/branches/11/include/asterisk/stringfields.h


*__p__ = target doesn't need to be done all the time, only when target != 
*ptr.

Probably should change to using __p__ instead of ptr to avoid having 
unexpected type problems dealing with ptr.


- rmudgett


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
> 
>

-- 
_
-- 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

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

[Corey Farrell]

> On Oct. 29, 2014, 7: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.

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.


- Corey


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


On Nov. 3, 2014, 8: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, 8: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
> 
>

-- 
_
-- 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

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

[Corey Farrell]

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

(Updated Nov. 3, 2014, 8: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 (updated)
-

  /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

-- 
_
-- 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

Re: [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


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


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

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

[Corey Farrell]

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

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