[asterisk-dev] [Code Review] 4126: app_voicemail: Fix unchecked bounds of myArray in IMAP_STORAGE.

[Matt Jordan]

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



/branches/1.8/apps/app_voicemail.c
<https://reviewboard.asterisk.org/r/4126/#comment24124>

    You shouldn't need the error message here - ast_realloc will log an error message already if the allocation fails.



/branches/1.8/apps/app_voicemail.c
<https://reviewboard.asterisk.org/r/4126/#comment24125>

    Looking at the code, I'm not sure how it could be free'd on all possible code paths.
    
    Clearly if it gets associated with thread local storage, it will be free'd appropriately. That only happens however in vm_execmain (which, oddly enough, associates a structure on the stack with thread local storage, then clears it upon exiting to prevent an attempt to free memory on the stack). Other then that, I don't see where memory allocated here is free'd. I didn't find:
    (1) An instance where a caller of create_vm_state_from_user free'd memory. This is probably appropriate, however, since the caller of create_vm_state_from_user does not own the memory returned (it is either thread local storage or it is stored in the vmstates list)
    (2) When we remove an entry from vmstates in vmstate_delete, I did not see us actually destroy the vms instance.
    
    Given the somewhat vague ownership semantics surrounding this memory - it can be stack allocated, dynamically allocated and assigned to thread local storage/stored in a linked list - I'm hesitant to recommend anything here right now. It's probably worth opening a separate issue for this however.



/branches/1.8/apps/app_voicemail.c
<https://reviewboard.asterisk.org/r/4126/#comment24126>

    This seems fishy as well. I'm not sure what the point of copying vmArrayIndex would be if you don't also have the msgArray.


- Matt Jordan


On Oct. 29, 2014, 4:53 a.m., wdoekes wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviewboard.asterisk.org/r/4126/
> -----------------------------------------------------------
> 
> (Updated Oct. 29, 2014, 4:53 a.m.)
> 
> 
> Review request for Asterisk Developers.
> 
> 
> Bugs: ASTERISK-24190
>     https://issues.asterisk.org/jira/browse/ASTERISK-24190
> 
> 
> Repository: Asterisk
> 
> 
> Description
> -------
> 
> In update_messages_by_imapuser(), messages were appended without checking bounds:
> 
>     vms->msgArray[vms->vmArrayIndex++] = number;
> 
> This patch ensures that there is enough room.
> 
> 
> However, I did find quirky usage of thread local storage which I couldn't explain. Perhaps someone else can shed some light on the XXX's that I left in the code:
> 
> - vms is thread-local, so it may not need to be freed. But on line 3033, it is overwritten if (strcmp(vms_p->imapuser, vmu->imapuser) || strcmp(vms_p->username, vmu->mailbox))
>   (or should it be freed in vmstate_delete?)
> 
> - in vmstate_insert, an alternative mailbox overwrites the supplied one, but no msgArray copying is done. That can't be right.
> 
> 
> Diffs
> -----
> 
>   /branches/1.8/apps/app_voicemail.c 426569 
> 
> Diff: https://reviewboard.asterisk.org/r/4126/diff/
> 
> 
> Testing
> -------
> 
> The reporter -- Nick Adams -- has run this patch in production for a number of months now, without issues.
> 
> 
> Thanks,
> 
> wdoekes
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.digium.com/pipermail/asterisk-dev/attachments/20141029/5b1d7221/attachment-0001.html>