Summary:	ASTERISK-04123: [patch] Fix 2 memory leaks in config.c which also uncovers potential problem in config file processing
Reporter:	James Golovich (jamesgolovich)	Labels:
Date Opened:	2005-05-09 20:21:45	Date Closed:	2008-01-15 15:37:44.000-0600
Priority:	Minor	Regression?	No
Status:	Closed/Complete	Components:	Core/General
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) asterisk-configleak.patch.txt ( 1) asterisk-configleak2.patch.txt
Description:	This fixes 2 memory leaks in config.c.  Both occur when files don't exist that are trying to be loaded.  Also this has the side effect of actually returning NULL to ast_config_load if it couldn't load the config so proper debug messages will again be printed. Disclaimer on file
Comments:	By: James Golovich (jamesgolovich) 2005-05-10 03:17:31 This actually isn't ready to go in.  This exposed me to another issue.  If connfig.c:process_text_line returns -1 then it blows away the config.  So if you try to include a file that doesn't exist (with my patch so it detects this condition correctly) then buh-bye config. This can also be reproduced by adding an invalid line to extensions.conf.  Like defining a context but forgetting the closing ] like: [testcontext Removing the cfg = NULL; assignment fixes this but I need to check more to see if there is another situation I am missing By: James Golovich (jamesgolovich) 2005-05-10 03:26:09 A few more thoughts and another test later shows that an invalid line in the middle of extensions.conf (with my fixes) drops everything after the error.  In current asterisk CVS HEAD if an invalid line is hit everything in the file is dropped. I'm not sure what the official word is on how this should work, but it seems to me it would be best to just ignore invalid lines and continue processing the file. By: Kevin P. Fleming (kpfleming) 2005-05-14 23:22:17 If an invalid line is encountered, there is really no value in continuing to process that file, since the results are completely undefined. Granted, there _may_ be valid information later in the file, so this is a 50/50 decision... I'll leave the policy changes for another bug on another day... is this patch ready to apply otherwise (it seems to be)? By: James Golovich (jamesgolovich) 2005-05-17 01:20:38 The current patch fixes the leaks but changes it so if theres an error in the config file then anything up to that is processed but nothing after the error.  So it changes the way things work a bit.  I'll have to look and see what it would take to duplicate the existing way its handled. By: Michael Jerris (mikej) 2005-05-25 12:49:05 Any update on this? By: Olle Johansson (oej) 2005-06-05 17:11:06 Gentle reminder... What's happening? /Housekeeping By: James Golovich (jamesgolovich) 2005-06-05 23:19:42 I've been in Washington for a bit now and heading home tommorow for a bit.  I'll see if I can make the patch mimic the existing behavior, but I suspect that any code to do it would be a major hack and should never go in CVS. By: James Golovich (jamesgolovich) 2005-06-07 17:17:17 asterisk-configleak2.patch.txt uploaded.  this reproduces existing behavior by changing it so the failure to ast_config_internal_load on an #include file a fatal error By: Kevin P. Fleming (kpfleming) 2005-06-07 17:24:03 Committed to CVS HEAD, thanks! By: Digium Subversion (svnbot) 2008-01-15 15:37:44.000-0600 Repository: asterisk Revision: 5879 U   trunk/config.c ------------------------------------------------------------------------ r5879 | kpfleming | 2008-01-15 15:37:43 -0600 (Tue, 15 Jan 2008) | 2 lines fix memory leaks in config loader (bug ASTERISK-4123) ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=5879