Summary:	ASTERISK-09750: NULL voicemail state terminates Asterisk
Reporter:	James Rothenberger (jaroth)	Labels:
Date Opened:	2007-06-25 10:00:35	Date Closed:	2007-07-06 11:22:32
Priority:	Minor	Regression?	No
Status:	Closed/Complete	Components:	Applications/app_voicemail
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) 10053.patch ( 1) quota.patch
Description:	If the pointer to vms (vm_state) is NULL (which should not be the case but it has happened twice in our installation), Asterisk terminates during the quota check using IMAP storage. ****** ADDITIONAL INFORMATION ****** Added check for non-NULL vms before accessing the data structure.
Comments:	By: Jason Parker (jparker) 2007-06-25 10:18:40 This fix won't hurt anything, but it would probably be best to find out why vms is NULL in the first place. By: James Rothenberger (jaroth) 2007-06-25 10:21:26 Agreed.  It has only happened a few times and this patch at least keeps Asterisk running.  If and when I find a root cause I will be sure to post. By: Mark Michelson (mmichelson) 2007-06-25 11:39:11 I'm monitoring this now, too. jaroth, if possible could you upload some information regarding this crash? Are there a specific set of steps you can perform to reproduce or is this seemingly random? Do you have any backtraces available from a crash? Also, I'd be interested in seeing console debug output when the crash occurs. Thanks. By: James Rothenberger (jaroth) 2007-06-25 11:55:46 I'll be sure to submit any information if it happens again.  I don't have any historic data from previous crashes. By: Mark Michelson (mmichelson) 2007-06-25 17:16:16 After looking at this a bit more, I think I've found out how vms can be null on that line. I think this could happen any time when a person calls someone who is currently logged into VoiceMailMain checking messages. This happens because in leave_voicemail, if no vm_state is found, inboxcount is called, the assumption being that it will create a new vm_state and add it to the vmstate list. The problem is, if someone is logged in to VoiceMailMain when inboxcount is called, inboxcount will find the vm_state for the logged in user and return before it can create a new vm_state. As such, back in leave_voicemail, vms remains null. The patch I have uploaded creates the vm_state, initializes values, and inserts it into the vmstate list prior to calling inboxcount. This should eliminate the possibility of vms being null when the quota_limit is checked. Edit: The patch is back up now. It is 10053.patch By: James Rothenberger (jaroth) 2007-06-26 11:20:09 Good find.  The original idea was to only have one vm_state active for a user, whether they are logged in or not.  That is why there is an "interactive" flag.  When doing a vmstate_insert, a check is done to see if a vm_state already exists for the user.  If so, it just copies the info to the existing one (the idea was to keep one one existing state as up-to-date as possible).  The patch looks valid, but maybe it should set interactive=1 so that vmstate_update will just update the existing vm_state for the user that should already exist instead of adding another state for the user.   Just a thought. By: Mark Michelson (mmichelson) 2007-06-26 11:39:51 I see what you're saying, but the assumption behind the patch (as well as the only way I think this crash could happen) is that there is NOT an existing vmstate for the user. If there were, then the get_vm_state_by_mailbox call would not have returned null in the first place. As such, the vm_state that gets created and inserted in this patch is intended to be the "persistent" vm_state for the user being called. This is why I have not set interactive to 1 in the patch. Furthermore, I don't believe setting interactive to 1 would be a wise choice since the side effect of doing so would be that ALL vm_states in the list would end up having interactive being set to 1, thus making it impossible to distinguish between a persistent vm_state and an interactive vm_state. By: James Rothenberger (jaroth) 2007-06-26 12:05:21 When an "interactive" vm_state is added, the info is copied to the existing one, and a "copy" of the existing "non-interactive" vm_state is stored in vms->persist_vms.  When an interactive user logs out, vmstate_delete copies the interactive info back to the non-interactive vm_state and then deletes the interactive one, leaving things the way they were before the interactive login.   Having said that, I see what you mean that in this case the problem is that there is no existing state so we might as well add it as the patch indicates.   I guess I am just not sure how this could happen, as non-interactive states are added the  first time a mailbox is checked for new mail, which happens quite often, from the time that Asterisk is started. By: James Rothenberger (jaroth) 2007-06-26 15:23:53 I applied this patch (10053) and voicemail crashed when it got to this new section of code.  I'll see what I can find. By: James Rothenberger (jaroth) 2007-06-27 08:47:12 Ok, found it.  It is the ast_debug line.  If I replace it with ast_log(DEBUG...) it works fine.  When compiling with ast_debug in the code, I get a warning during compile: warning: implicit declaration of function `ast_debug' Any ideas? By: Mark Michelson (mmichelson) 2007-06-27 09:39:57 ast_debug is a new macro that was added a couple of weeks ago in trunk. The idea was to replace this construct if (option_debug > 2)    ast_log (LOG_DEBUG, "blah"); with ast_debug(3, "blah"); This macro is defined in logger.h. Upgrade to the latest logger.h and see if the problem goes away. By: James Rothenberger (jaroth) 2007-06-27 09:43:14 Got it.  I applied this patch to a 1.4-based version, that is why it didn't work.  Thanks! By: Mark Michelson (mmichelson) 2007-06-29 15:18:09 After some further investigation in the code, I found another cause for the crash you're seeing. Someone made a logic error in get_vm_state_by_imapuser when changing the code to use Asterisk's linked list macros. The mistake he made was that he forgot to actually put any sort of comparison between the imapuser passed in and the imapuser of the current vm_state. As such, the get_vm_state_by_imapuser calls in inboxcount could succeed even though that particular imapuser does NOT have a vm_state in the vmstates list. Because get_vm_state_by_imapuser is successful, no vm_state is inserted into the list. When control returns to leave_voicemail, it tries to call get_vm_state_by_mailbox and can't find a vm_state, thus vms remains NULL. I committed a fix to trunk (revision 72670) which corrects the logical error in get_vm_state_by_imapuser. This fix, however, does not eliminate the first situation for which I said vms could be null, so my patch I've uploaded still seems to be the answer for that. Edit: Fixed some grammar to make the note clearer. By: Mark Michelson (mmichelson) 2007-07-06 10:25:28 I haven't heard anything for a while on this, so I'm going to make the assumption that my patch is all right. I will soon be committing this as well as a similar patch for 1.4. By: Digium Subversion (svnbot) 2007-07-06 11:19:38 Repository: asterisk Revision: 73727 ------------------------------------------------------------------------ r73727 | mmichelson | 2007-07-06 11:19:38 -0500 (Fri, 06 Jul 2007) | 8 lines Fixing a rare case which causes voicemail to crash when compiled with IMAP storage. inboxcount has the possibility of finding an "interactive" vm_state when no persistent "non-interactive" vm_state exists for that mailbox. If this should happen when someone attempts to leave a message, it results in a crash. This patch, along with my commit in revision 72670 fix issue 10053, reported by jaroth. closes issue ASTERISK-9750 ------------------------------------------------------------------------ By: Digium Subversion (svnbot) 2007-07-06 11:22:32 Repository: asterisk Revision: 73728 ------------------------------------------------------------------------ r73728 | mmichelson | 2007-07-06 11:22:32 -0500 (Fri, 06 Jul 2007) | 16 lines Merged revisions 73727 via svnmerge from https://origsvn.digium.com/svn/asterisk/branches/1.4 ........ r73727 | mmichelson | 2007-07-06 11:36:17 -0500 (Fri, 06 Jul 2007) | 8 lines Fixing a rare case which causes voicemail to crash when compiled with IMAP storage. inboxcount has the possibility of finding an "interactive" vm_state when no persistent "non-interactive" vm_state exists for that mailbox. If this should happen when someone attempts to leave a message, it results in a crash. This patch, along with my commit in revision 72670 fix issue 10053, reported by jaroth. closes issue ASTERISK-9750 ........ ------------------------------------------------------------------------