[asterisk-dev] [Code Review] Fix memory leaks in IMAP app_voicemail

[wdoekes]

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

Ship it!


Looks fine by me.


Since you're in the vicinity, you could replace "imappasswd" with "imapfolder" in the string below.
I'd do it, but I left my commit access at home ;)

        if (strcasecmp(vmu->imapfolder, "INBOX")) {
                ast_test_status_update(test, "Parse failure for imappasswd option\n");
                res = 1;
        }


Further, I see in:

        static int __messagecount(const char *context, const char *mailbox, const char *folder)

another find_user() but no free_user() of vmu.

And in leave_voicemail() only /some/ returns are preceded by a free_user(). And
more of the same in vm_authenticate().. and a pretty blatant one in
both vm_box_exists() and acf_mailbox_exists() and the list goes on.


I'll just ship-it and leave the rest for later ;)

- wdoekes


On Sept. 4, 2012, 9:42 p.m., Matt Jordan wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/2096/
> -----------------------------------------------------------
> 
> (Updated Sept. 4, 2012, 9:42 p.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Summary
> -------
> 
> This looked to be a fairly straight forward patch (provided by the issue reporter, Filip Jenicek) - however, since the last time a memory leak was 'fixed' in app_voicemail it caused a memory corruption, I figured I'd post it up for review.
> 
> This patch fixes two memory leaks:
> 
> 1. When find_user is called with NULL as its first parameter, the voicemail user returned is allocated on the heap.  The inboxcount2 function uses find_user in such a fashion when counting new messages, and fails to free the resulting voicemail user object.
> 
> 2. When populate_defaults is called on a voicemail user, it wipes whatever flags have been set on the object by copying over the global flags object.  If the VM_ALLOCED flag was set on the voicemail user prior to doing so, that flag is removed.  This leaks the voicemail user when free_user is later called.
> 
> This patch differs slightly from Filip's patch in that the populate_defaults function isn't changed to preserve the VM_ALLOCED flag, as that function affects a significant number of places in the code (and the comments for the function state that it copies over the global flags - so we probably should have expected that flags set prior to the function would be blown away).  Instead, places that set the VM_ALLOCED flag do so now after calling populate_defaults.
> 
> This affected IMAP users and realtime users (and a unit test).
> 
> 
> This addresses bug ASTERISK-19155.
>     https://issues.asterisk.org/jira/browse/ASTERISK-19155
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/apps/app_voicemail.c 372176 
> 
> Diff: https://reviewboard.asterisk.org/r/2096/diff
> 
> 
> Testing
> -------
> 
> 
> Thanks,
> 
> Matt
> 
>

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