Summary:	ASTERISK-21302: [patch] app_voicemail crashes on config error and there are some potential memory leaks
Reporter:	Jaco Kroon (jkroon)	Labels:
Date Opened:	2013-03-19 09:03:01	Date Closed:	2013-04-12 17:25:19
Priority:	Major	Regression?
Status:	Closed/Complete	Components:	Applications/app_voicemail
Versions:	1.8.21.0 11.2.1	Frequency of Occurrence	Occasional
Related Issues:
Environment:	Linux	Attachments:	( 0) asterisk-11.3.0-app_voicemail-ast_config-fixes.patch ( 1) asterisk-11.3.0-app_voicemail-ast_config-fixes.patch ( 2) asterisk-21302-valid_cfg_and_mem_leaks_v3-1.8.diff
Description:	#0  ast_config_destroy (cfg=0xfffffffffffffffe) at config.c:1128 #1  0x00007fbcac12a4d8 in store_file (dir=<optimized out>, mailboxuser=<optimized out>, mailboxcontext=<optimized out>, msgnum=<optimized out>) at app_voicemail.c:4153 #2  0x00007fbcac13f1dc in leave_voicemail (chan=0x7fbca49801f8, ext=<optimized out>, options=<optimized out>) at app_voicemail.c:6689 #3  0x00007fbcac140680 in vm_exec (chan=0x7fbca49801f8, data=<optimized out>) at app_voicemail.c:11568 #4  0x00000000005269a4 in pbx_exec (c=0x7fbca49801f8, app=0x7fbcb4e767e0, data=0x7fbc7e28d5ac "sdgtech,su") at pbx.c:1589 #5  0x00007fbcadb95f22 in execif_exec (chan=0x7fbca49801f8, data=<optimized out>) at app_exec.c:273 #6  0x00000000005269a4 in pbx_exec (c=0x7fbca49801f8, app=0x7fbca41e0570, data=0x7fbc7e28f750 "1?VoiceMail(sdgtech,su)") at pbx.c:1589 #7  0x00000000005308ec in pbx_extension_helper (c=0x7fbca49801f8, con=0x0, context=<optimized out>, exten=0x7fbca4981098 "0100031015", priority=68, label=<optimized out>, callerid=0x7fbca4fd7500 "0116081375", action=E_SPAWN, found=0x7fbc7e291de0, combined_find_spawn=1) at pbx.c:4665 #8  0x0000000000535e28 in ast_spawn_extension (found=0x7fbc7e291dd0, callerid=0x7fbca4fd7500 "0116081375", priority=68, exten=0x7fbca4981098 "0100031015", context=<optimized out>, c=0x7fbca49801f8, combined_find_spawn=<optimized out>) at pbx.c:5781 #9  __ast_pbx_run (c=0x7fbca49801f8, args=0x0) at pbx.c:6256 #10 0x00000000005376cb in pbx_thread (data=<optimized out>) at pbx.c:6586 #11 0x000000000057919b in dummy_start (data=<optimized out>) at utils.c:1028 #12 0x00007fbcb9845d0c in start_thread (arg=0x7fbc7e292700) at pthread_create.c:301 #13 0x00007fbcbadd3bed in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:115 The code dealing with this is so horrible I don't even want to think about how to generate a patch.
Comments:	By: Michael L. Young (elguero) 2013-03-19 10:07:55.717-0500 Thank you for your bug report.   What was the config error?  Can you provide the bad config to help reproduce this problem. When getting a backtrace, please be sure you have DONT_OPTIMIZE enabled in menuselect within the Compiler Flags section, then: make install After enabling, reproduce the crash, and then execute the backtrace[1] instructions. When complete, attach that file to this issue report. [1] https://wiki.asterisk.org/wiki/display/AST/Getting+a+Backtrace By: Jaco Kroon (jkroon) 2013-03-19 10:28:07.928-0500 Hi, I wish.  5 seconds later my auto-recovery script restarted asterisk and all was well.  All I can tell you is that *any* error response from ast_config_load (other than CONFIG_STATUS_MISSING) *should* generate the crash.  There is no error checking happening on the return value at all. In the above case a value of -2 was generated (CONFIG_STATUS_FILEINVALID).  A return value of CONFIG_STATUS_UNCHANGED may trigger this too (although I don't think the particular invocation of ast_config_load will ever result in that.  The only error condition that will NOT trigger the segfault is CONFIG_STATUS_FILEMISSING (value of 0). Basically the issue is this:    if (cfg)        ast_config_destroy(cfg); Now, cfg of -1, -2 (or any other new error code which would probably be -3 and later -4 etc ...) is obviously not NULL, would result in the above being true, and shortly after a spectacular explosion should follow. By: Michael L. Young (elguero) 2013-03-19 12:22:03.790-0500 Here is a patch that seemed to resolve this for me.  I found some other places that needed to have the extra check added, otherwise the code continues and would suffer a crash as well. By: Jaco Kroon (jkroon) 2013-04-03 06:54:36.722-0500 Hi, Sorry for the delay, had a few crazy weeks behind me with (primarily) asterisk-11.* issues. Haven't tested the patch, but I did read through the patch and have managed to confirm that all of your changes are in order. Your assessment is correct, pretty much every invocation of ast_config_load() in app_voicemail.c is a bug.  I've also spotted a case for vm_change_password() where the cfg pointers are being leaked (thus resulting in a memory leak too). I personally think the ast_config API shouldn't mix a pointer return value and a error value, a better approach would be to pass a pointer to an int to store the error code in, and return NULL on *all* errors.  But that is just my personal opinion, obviously others will have different opinions.  Also, making this change throughout the entire asterisk codebase is massively intrusive. I'm busy creating a secondary patch in an attempt to address the memory leaks too.  Your patch looks good from what I can see though. I still think there is a reasonable risk of additional error codes being added at some point and then even the patched version would break, but for the moment this will have to do. By: Jaco Kroon (jkroon) 2013-04-03 07:17:31.515-0500 Ok, You had the last hunk wrong: @@ -14049,7 +14049,7 @@ fputs("00000002 => 9999,Mrs. Test\n", file); fclose(file); - if (!(cfg = ast_config_load(config_filename, config_flags))) { + if (!(cfg = ast_config_load(config_filename, config_flags)) || cfg != CONFIG_STATUS_FILEINVALID) { res = AST_TEST_FAIL; goto cleanup; } The new condition will *always* evaluate to true, the updated patch has this as || cfg == CONF... instead of !=. I also added three more hunks which should be checked, the first two in my patch adds missing ast_config_destroy lines, whereas the last one moves a ast_config_destroy out of an #ifndef block that would trigger a config leak in the case of IMAP storage. By: Jaco Kroon (jkroon) 2013-04-03 07:18:48.787-0500 Patch reviewed and amended.  Need feedback from Michael. By: Michael L. Young (elguero) 2013-04-03 13:44:50.717-0500 Good catch on that conditional! Also, thanks for finding the two memory leaks.  The one dealing with the #ifndef being moved looks good.  The other one, I think needs to be changed.  You could still potential have a leak if you hit those 'breaks;' first before running ast_config_destroy().  I am going to put together a patch incorporating your catches and let me know what you think. By: Jaco Kroon (jkroon) 2013-04-03 13:56:27.187-0500 Updated to catch the one break that could affect it.  The other break's are breaking out of the while and for loops and thus will not leak the ast_config instance. By: Michael L. Young (elguero) 2013-04-03 14:52:08.459-0500 Jaco, doesn't my updated patch take care of it as well?  In my opinion, calling ast_config_destroy() from one place is better than having to maintain several places and potentially add a leak again if those case statements, while loop or for loop change... to me that would be a better approach.  What do you think? By: Jaco Kroon (jkroon) 2013-04-03 17:19:12.280-0500 Overlooked your updated patch.  Apologies for that. But no, it does not.  If you look at the block you'll note that it loads two separate config files, there is an explicit fall-through between the case's in the switch, each individually loading a different config.  My patch does cover that completely (I think), your patch misses the case where the latter config load overwrites the pointer as created by the first load. I completely agree that it's better to add it in one place if possible. Another option is to add the same kind of if statement just prior to the second load (or at the end of the initial case just prior to the fall-through). Another idea is to use two separate pointer variables and then have two if statements at the bottom. By: Michael L. Young (elguero) 2013-04-03 21:22:25.071-0500 Doh!  You are correct... I don't know why I saw that, thought about that, then forgot about it... The way you have it looks to be the best way upon re-looking at it.  No need for the extra conditionals since we already know we have loaded the config and that it is valid. -If you are in agreement with the final updated patch I will be attaching, I can put it up for review and work on getting it into the code base.- If you want me to put the final patch you posted up for review, I can do so and work on getting it into the code base. By: Jaco Kroon (jkroon) 2013-04-03 23:56:55.420-0500 You're welcome.  I never did get access to the reviewboard sorted out.  I'll see if I can get hold of someone to assist on IRC at some point (hopefully) today. By: Michael L. Young (elguero) 2013-04-10 16:57:01.570-0500 Jaco, hope it was okay to post the patch on Reviewboard since I didn't see it go up yet.  The developers tend to look at reviews on Thursday and I would like to get this in as soon as possible. By: Jaco Kroon (jkroon) 2013-04-11 00:44:00.328-0500 No problem at all.  Been busy trying to track some quality issues for a client, so the result is that whilst I saw the email from someone at Digium who created me the reviewboard credentials I haven't properly looked at the reviewboard yet.  Have however started looking at some of the generated reviewboard emails on the dev mailing list.