Summary:	ASTERISK-23713: Voicemail on FS overwrites last message when last index = MSGLIMIT
Reporter:	Miguel Tavares (mtavares)	Labels:
Date Opened:	2014-05-03 01:54:32	Date Closed:	2014-05-22 06:24:04
Priority:	Major	Regression?
Status:	Closed/Complete	Components:	Applications/app_voicemail
Versions:	SVN 12.2.0	Frequency of Occurrence	Constant
Related Issues:	is related to ASTERISK-20597 Cannot forward voice mail due to "Mailbox is full with capacity of 100" even though the folder has less than 100 messages
Environment:	app_voicemail delivery on filesystem. Any asterisk supported platform is probably affected.	Attachments:
Description:	When leaving a voicemail the function last_message_index will stop at index only until the default or configured max messages (vmu->maxmsg) although it should continue until MAXMSGLIMIT. Example, let's say the asterisk admin set the maximum messages to 10, the user received 10 messages and then deleted 9 apart from the last one. In the filesystem the files msg0010.txt and msg0010.wav will be present. The last_message_index will detect that that's the only message available but then  when finding out what's the index it will stop at 9 (the configure maximum when counting from 0) and return that as the last message index, making he voicemail app to overwrite the previous existing last message. Possible fix, in app_voicemail.c {noformat} 112a113 > #include <string.h> 4434c4435 <       unsigned char map[MAXMSGLIMIT] = ""; --- >       unsigned char map[MAXMSGLIMIT]; 4440a4442 >     memset (map, 0, sizeof(map)); //set the memory to 0 for Safety sake. 4458c4460 <       for (x = 0; x < vmu->maxmsg; x++) { --- >       for (x = 0; x < MAXMSGLIMIT; x++) { {noformat} The important change is running the for cycle until MAXMSGLIMIT but as far as I know there's no guarantee that memory allocated in the stack is zeroed so that's why I also included the memset call.
Comments:	By: Rusty Newton (rnewton) 2014-05-03 14:17:15.527-0500 It wasn't clear to me the ways in which this would affect a user. I just tested, and here is *one* example other than the overwrite: * User has a maximum new message limit of three and receives three new messages. * User begins listening to new messages. * User deletes the first two messages. * On the third message, the user chooses to save the third message into the New folder (INBOX). Asterisk says that the mailbox is full (despite having just deleted the previous two messages) and prompts you again to save the message in a different folder. I believe this issue would be caused by the same index problem. By: Rusty Newton (rnewton) 2014-05-03 14:20:06.276-0500 [~mtavares] Would you like to re-attach your patch to the issue as a code contribution so that we can consider it for inclusion and attribute the contribution to you? Here is the [Code Review|https://wiki.asterisk.org/wiki/display/AST/Code+Review] process. However for such a small patch, if you just attach it under license, we can likely throw it in assuming it fixes the issue and there is no major issues with it. By: Miguel Tavares (mtavares) 2014-05-04 12:20:25.214-0500 The patch presented only limits the effect of the issue. If the user get's to have a message 9999 the same issue will arise and either the last message is overwritten or the user can't receive more messages. Some discussion on how to better solve this might be needed. On possible solution is to just try to find the first empty msg slot (for example if user as a msg 0002 and 0003 then the next message would be 0001, the one after would be 0004). This breaks the relationship between the ID of the messages and the order of reception so the user might get confused. If it's ok then I can try to look more carefully into the code and create a patch for using the reuse of empty msg slots. By: Miguel Tavares (mtavares) 2014-05-05 13:34:47.079-0500 Here is the proper patch (not just a diff). I'll try to follow the code review (so that I also get to learn how to do that). EDIT: my password doesn't work with the review board so this might take some time. _removed inline patch_ By: Michael L. Young (elguero) 2014-05-06 09:30:12.614-0500 Miguel, Thanks for the patch.  You need to attach patches as files so that it falls under the license agreement that you need to sign if you haven't done so already.  We cannot accept inline patches. Can you please attach a full debug log?  At least debug level of 4 since there is a debug message that will help track how the index is being determined. [Collecting Debug Information | https://wiki.asterisk.org/wiki/display/AST/Collecting+Debug+Information] I am not seeing where your patch solves the real issue.  MAXMSGLIMIT is not set by the admin.  There is a global setting, maxmsg, that is set by the admin.  Then there is a per user setting, vmu->maxmsg.  I don't think we should change that for the default per directory message limit MAXMSGLIMIT. There might be a problem elsewhere with handling properly what is being returned by this function.  The debug log would be helpful to getting started on looking at this sooner. By: Michael L. Young (elguero) 2014-05-06 09:56:54.928-0500 Rusty, Your scenario above is a tricky one.  If a user deletes the messages, they can also un-delete them until they hangup.  The index will count those messages and include that as part of the number of messages under the user's mailbox.  When saving a message as new, it is counting how many messages the user has (3) and the maximum messages the user is allowed (3).  If I recall properly, it does not just mark the message the user listened to as unheard (new), it actually makes a copy of the file and therefore the maximum message limit for the user kicks in. Please look at my comment on ASTERISK-20597.  I still think that a change went in some time ago that changed the documented behavior and causes issues like these to exist.  There were never any comments to support or to negate that thinking. By: Miguel Tavares (mtavares) 2014-05-06 13:23:43.037-0500 Michael, I think count_messages(..) is the function that is used to actually know how many messages are present in the mailbox. It seems to me to be correct in its implementation. The function last_message_index is used to know what numbers to put in the filename of the stored messages (not for counting). Using vmu->maxmsg in the for loop means that if the configured maxmsg is 10 and the user as one single message with index 9 then last_message_index would return 9 and the message already in store will be overwritten. That's my understanding of the code. I'm not sure how the "undelete" function works. My guess is that the messages are only unlinked at the end of the user accessing the voicemail (but I'm not sure about this). About the patch: I sign the license but have not been contacted back so I have no idea when / if  I'll be able to post the patch to the open source asterisk. Well.. at least I wasn't asked for my first born son when signing the license. :) By: Rusty Newton (rnewton) 2014-05-06 13:37:57.313-0500 Miguel, press Send Back or use Enter Feedback when responding to throw the issue back to bug marshals or the previous assignee. Sometimes it takes a couple days for legal to process the license agreement. Normally it is same day. By: Rusty Newton (rnewton) 2014-05-06 13:41:16.119-0500 [~elguero] bq. Your scenario above is a tricky one. If a user deletes the messages, they can also un-delete them until they hangup. The index will count those messages and include that as part of the number of messages under the user's mailbox. When saving a message as new, it is counting how many messages the user has (3) and the maximum messages the user is allowed (3). If I recall properly, it does not just mark the message the user listened to as unheard (new), it actually makes a copy of the file and therefore the maximum message limit for the user kicks in. I see.. that is tricky. I hope whoever works on this can create a behavior that is more sensible to the end user of the voicemail system, as they will truly not know what is going on and instead just be frustrated at the inability to save their message in a folder that appears to have room. Thank you guys for looking into it. By: Rusty Newton (rnewton) 2014-05-12 09:45:38.376-0500 [~mtavares] I have word from our legal department that your license agreement should be processed. You should be able to attach the patch to the issue as a code contribution now. The next step would be following the [code review process|https://wiki.asterisk.org/wiki/display/AST/Code+Review] as indicated on the wiki. Remember to also attach the debug log requested by Michael Young, as that will be helpful to those looking at the patch and debugging the issue. By: Miguel Tavares (mtavares) 2014-05-13 13:57:08.130-0500 Rusty Newton : I confirm I have received the email from the legal department. Sorry I took so much time to respond. I'm moving and there has not been much free time available. I'll submit a patch but I think its not a complete solution (as I stated in a previous comment). I've lost the log (they were in one of the internal drives of an old eeepc that in the mean time had became a brick, unable to boot). I'll try to create the same scenario again. By: Michael L. Young (elguero) 2014-05-13 17:43:57.829-0500 Miguel, I see you have posted to reviewboard but haven't posted your patch here to this issue yet. Please review the guidelines and process for contributing patches here: https://wiki.asterisk.org/wiki/display/AST/Patch+Contribution+Process Thanks By: Michael L. Young (elguero) 2014-05-13 18:15:19.531-0500 {quote} Example, let's say the asterisk admin set the maximum messages to 10, the user received 10 messages and then deleted 9 apart from the last one. In the filesystem the files msg0010.txt and msg0010.wav will be present. {quote} I tried reproducing this issue based on your description.  I have not been able to do so.  With the scenario that you are presenting, is the max msg limit set globally or per user? Also, if the max masg limit is 10, there will not be present the "msg0010.txt and msg0010.wav" files.  Remember the index starts at 0.  The highest number would be "msg0009.txt and msg0009.wav".  I know that might have just been a mistake but wanted to mention that to help clarify the scenario we are trying to fix. Thanks By: Michael L. Young (elguero) 2014-05-13 18:39:12.599-0500 Rusty, I can confirm your scenario.  What happens is that the messages are marked as deleted but the files are still there on the filesystem until the user hangs up.  When trying to save to the INBOX folder, it checks the index (count of files present) and sees the messages that were deleted are still there. By: Miguel Tavares (mtavares) 2014-05-14 02:36:01.318-0500 Hi. Some small examples of behavior. "map" represents the messages that exist in the file system (position 0 corresponds to msg0000, position 1 is msg0001, etc). "last msg index" is the return of calling the last_msg_index function and the file name is obvious. With vmu->maxmsg=5 and MAXMSGLIMIT=10: map = (0,0,0,0,0,0,0,0,0,0) last msg index = -1 creating file msg0000.(txt,wav) map = (1,0,0,0,1,0,0,0,0,0) last msg index = 4 creating file msg0005.(txt,wav) map = (1,0,0,0,1,1,0,0,0,0) last msg index = 4 creating file msg0005.(txt,wav) Notice how the user has only 3 messages but the next message will overwrite message 5. With the patch supplied: map = (1,0,0,0,1,1,0,0,0,0) last msg index = 5 creating file msg0006.(txt,wav) By: Michael L. Young (elguero) 2014-05-14 06:44:29.774-0500 Miguel, Something is just not quite right here.  This is the part that I am trying to understand and figure out how to reproduce it before we can look at the patch and what it is doing to fix this.  How does the user's INBOX jump from last_msg_index being == -1 to having last_msg_index == 4?  That should not happen.  Is this scenario using only app_voicemail?  Nothing external is adding messages to the user's INBOX? If the last_msg_index is jumping from -1 to 4, that is the problem that needs to be tackled. Please try to reproduce and outline the exact steps required to reproduce it.  Also, please capture a debug log when you are able to reproduce it. Thanks By: Miguel Tavares (mtavares) 2014-05-16 01:32:56.461-0500 Hi, It was just an example and not sequential. I'm going to try better understand why my installation of asterisk was overwriting the last message when other slots were available. By: Miguel Tavares (mtavares) 2014-05-21 14:41:20.034-0500 Ok, my bad. I still don't know why the the voicemail inbox is being left with messages out of sequence. When the voicemail app closes the inbox and if the last message index == maxmsg it calls the resequence_mailbox function that reorders the messages numbers so that again they start on 0 and proceed on increments of 1. (I check this to be true with a clean instance of asterisk) My guess is that there was something that was deleting messages without using the voicemail app. I think I'll never know for sure as it was running on a old eeepc that in the mean time had a motherboard failure and doesn't boot anymore. My guess is that it was the web interface that was done by a housemate that is gone in the meantime. Sorry for the time consumed with this issue. By: Matt Jordan (mjordan) 2014-05-22 06:24:04.137-0500 I'm going to go ahead and close this out as "Can't Reproduce". If you do find out what was causing the resequencing problems, and feel it is due to a bug in Asterisk, comment here and we can reopen the issue.