[asterisk-dev] [Code Review] Fix potential memory leaks in taskprocessor code

[jcolp]

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



/branches/1.8/main/taskprocessor.c
<https://reviewboard.asterisk.org/r/2253/#comment14474>

    Won't removing this ao2_ref from here and below cause a reference leak?


- jcolp


On Dec. 24, 2012, 2:37 p.m., Mark Michelson wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2253/
> -----------------------------------------------------------
> 
> (Updated Dec. 24, 2012, 2:37 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> Taskprocessors in Asterisk have the potential to leak memory. Taskprocessor tasks take a data pointer as a parameter. Since this task is executed in a separate thread from where the task was pushed, the task data is heap-allocated. When a task runs, the execution callback for the task frees the task data when it completes.
> 
> This is fine for typical operation, but in the case where a taskprocessor is shut down while there are still tasks in queue, we certainly don't want to execute the remaining tasks, we simply wish to remove them from the queue and free them. However, given the current model, there is no way for the task data to be freed properly. This patch introduces a change to ast_taskprocessor_push() to take a new destroy() callback. Rather than having the task execution callback be responsible for freeing task data, the freeing of task data is now handled automatically by the taskprocessor itself.
> 
> Some points of discussion:
> * chan_iax2's usage of taskprocessors is a bit unpredictable. Its task execution callback may or may not end up needing to re-use the task data at a later time. Thus we cannot pass a destroy() callback when the taskprocessor task is pushed. In my view, this may mean that there is still the potential to leak frames in chan_iax2 when its taskprocessor is shut down. Is there a better alternative that may be used here?
> * The set of changes presented here is made against 1.8 because the problem exists there. However, this introduces an API change and so it could be deemed an improper change for 1.8. While it is fixing potential memory leaks, the chances of the leaks to occur are low since the lifetime of taskprocessors is tied either to the lifetime of Asterisk or of the module where the taskprocessor was allocated. This means that the majority of the time, the leak would only occur during Asterisk shutdown. Is this set of changes appropriate for 1.8?
> 
> Note: this work uncovered a memory leak that occurs during a particular failure case in app_voicemail.c. The fix for this leak should go into all branches whether the taskprocessor API change is approved or not.
> 
> 
> This addresses bug ASTERISK-20786.
>     https://issues.asterisk.org/jira/browse/ASTERISK-20786
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/main/event.c 378204 
>   /branches/1.8/main/pbx.c 378204 
>   /branches/1.8/main/taskprocessor.c 378204 
>   /branches/1.8/apps/app_queue.c 378204 
>   /branches/1.8/apps/app_voicemail.c 378204 
>   /branches/1.8/channels/chan_iax2.c 378204 
>   /branches/1.8/include/asterisk/astobj2.h 378204 
>   /branches/1.8/include/asterisk/taskprocessor.h 378204 
>   /branches/1.8/main/astobj2.c 378204 
>   /branches/1.8/main/ccss.c 378204 
> 
> Diff: https://reviewboard.asterisk.org/r/2253/diff
> 
> 
> Testing
> -------
> 
> Started Asterisk and placed calls through. This exercises the taskprocessors in app_voicemail.c, pbx,c, event.c, and app_queue.c. I also did taskprocessor pings from the CLI. The one aspect that is untested is call completion.
> 
> 
> Thanks,
> 
> Mark
> 
>

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