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

[Matt Jordan]

> On Sept. 5, 2012, 2:29 a.m., wdoekes wrote:
> > 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 ;)

Test problem fixed.

You actually don't need to call free_user on a voicemail user obtained from find_user unless the first parameter passed to find_user is NULL.  On the other hand, free_user should be safe to call in all cases, as a non-malloc'd voicemail user is simply passed over in free_user, and it would make the semantics of calling find_user consistent throughout app_voicemail.  I didn't want to expand the patch any more than I needed to (dramatic changes in app_voicemail have a tendency to haunt the programmer)


- Matt


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


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/53179dfb/attachment.htm>