Summary:	ASTERISK-20435: app_voicemail deletes the wrong greeting if both an unavailable and a temporary greeting is available and imap greetings are used
Reporter:	fhackenberger (fhackenberger)	Labels:
Date Opened:	2012-09-17 15:49:03	Date Closed:	2012-09-25 16:20:27
Priority:	Major	Regression?
Status:	Closed/Complete	Components:	Applications/app_voicemail/IMAP
Versions:	1.8.16.0	Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) asterisk-20435-imap-del-greeting.diff ( 1) msgnum_negative.patch
Description:	Set up voicemail with imap and imapgreetings=yes in voicemail.conf Dial into VoiceMailMain and do the following: * Record an unavail greeting * Hangup * Record a temp greeting * Hangup * Delete a temp greeting (BUG: it deletes the unavailable greeting) The bug seems to be that vm_imap_delete uses the msgnum parameter as an index into vms->msgArray even if it is negative, which is certainly wrong. I assume (talked to mjordan on IRC) that passing - 1 as the msgnum indicates that the current message should be deleted (vms->curmsg). The attached patch fixes that. It issue is present in 1.6 (where I debugged the issue) as well as the current 1.8 branch.
Comments:	By: Michael L. Young (elguero) 2012-09-17 22:53:59.187-0500 -1 is not to indicate that the current message should be deleted.  It would appear that passing -1 as the msgnum is used to indicate greetings so that those messages are handled differently than a regular voicemail message. Can you attach a debug log produced by 1.8 branch?  https://wiki.asterisk.org/wiki/display/AST/Collecting+Debug+Information Thanks By: Matt Jordan (mjordan) 2012-09-18 09:24:08.980-0500 Yup, that is the intent there.  The problem is that you can't use {{msgnum}} (with a value of \-1, which indicates you want to delete a greeting) to index into the msgArray - not only are you indexing into an array using a negative number (yikes!) but you have to use the value of {{vms->curmsg}} to delete the 'current' message that was retrieved from the IMAP backend.  Note that the 'current' message retrieved from the IMAP backend occurs prior to the call to {{vm_imap_delete}} in {{imap_retrieve_file}}/{{imap_retrieve_greeting}}.  The whole flow should go something like: * Call {{imap_retrieve_file}} ** If the message being retrieved is a greeting, call {{imap_retrieve_greeting}}.  This sets the entry in the array at {{vms->curmsg}} to the actual greeting message. ** If the message being retrieved is not a greeting, then msgnum will reference an actual message in the msgArray that was earlier retrieved from the IMAP backend. * Call {{vm_imap_delete}} ** If {{msgnum}} is \-1, then delete the appropriate greeting message using {{vms->curmsg}} ** If {{msgnum}} is not \-1, then delete the appropriate non-greeting message using {{msgnum}} By: Michael L. Young (elguero) 2012-09-18 12:00:47.712-0500 Yes, I saw the problem with using msgnum as an index.  I was looking at this briefly and added my comments to correct the comments from the reporter that -1 was being used to indicate delete last message. I can see how vms->curmsg can/should be used, but to keep in line with what is currently being done in the code shouldn't imap_delete_old_greeting be used? vm_imap_delete is called and if msgnum is -1, we call imap_delete_old_greeting and return.  If msgnum is not -1, we handle the deletion of the file as normal. By: Michael L. Young (elguero) 2012-09-18 14:39:21.321-0500 Here is my proposed fix to this bug: [^asterisk-20435-imap-del-greeting.diff] I set up IMAP voicemail storage on one of my dev boxes and was able to reproduce the bug reported here.  With the attached patch, the regular greetings file was no longer being deleted when erasing the temporary greeting. By: Michael L. Young (elguero) 2012-09-18 14:45:39.622-0500 One thing I just noticed in my patch, it can be optimized by removing the redundant "&& imapgreetings" in the if statement since we would never reach this part of the code if "!imapgreetings" due to the conditional statement a few lines above.