Summary:	ASTERISK-06149: [patch] Fixing a memory leak in res/res_odbc.c
Reporter:	Matthew Roth (matthew roth)	Labels:
Date Opened:	2006-01-20 15:22:36.000-0600	Date Closed:	2008-01-15 16:34:38.000-0600
Priority:	Minor	Regression?	No
Status:	Closed/Complete	Components:	Core/General
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) res_odbc-fix_mem_leak-1.0.patch ( 1) res_odbc-fix_mem_leak-1.2.patch
Description:	This patch fixes a memory leak in res/res_odbc.c The function new_odbc_obj() allocates memory, but doesn't free it prior to returning under an error condition. This is a fairly innocuous memory leak because it only occurs when allocations are failing, but I thought it should still be corrected. ****** ADDITIONAL INFORMATION ****** This issue is related to issue 6299. The patch from issue 6299 must be applied to res/res_odbc.c (SVN Revision 8275) prior to applying the attached patch.
Comments:	By: Tilghman Lesher (tilghman) 2006-01-22 09:11:42.000-0600 Please prepare this case for 1.2.  Memory leaks are addressable in the release branch. By: Matthew Roth (matthew roth) 2006-01-23 15:21:17.000-0600 Corydon, I have uploaded a new patch (res_odbc-fix_mem_leak-1.2.patch).  It applies to the 1.2 branch, revision 8511. Please let me know if you need anything else. Thanks, Matt By: Olle Johansson (oej) 2006-01-30 14:21:37.000-0600 Corydon76: Check if this is ready for svn commit, thanks. /O By: Matthew Fredrickson (mattf) 2006-01-31 17:11:47.000-0600 It still has problems.  The if statement at the beginning which allocates all the memory is not an "atomic" operation.  In other words, if any of those mallocs (or the calloc) fails,  when you cleanup, you will have to check each allocated field to see if it's not NULL before you free.  If you don't check the subfields before you free them, you will end up trying to free something not there, incurring (beyond the failed malloc, which is bad in itself) much more "badness". By: Matthew Roth (matthew roth) 2006-01-31 17:48:39.000-0600 mattf, I believe I covered this.  The only operation that has to be successful is the calloc.  If it is, each of the pointers in the struct (new->name, new->dsn, etc.) gets initialized to zero or NULL and can be safely freed.  If the calloc fails, none of the pointers need freed and doing so would result in the "badness" you speak of.  That is why I only test the pointer that was calloc'd prior to freeing all of them. I was skeptical too, so I wrote a little driver to make sure it worked.  I simulated the failure of each of the allocations with no problems. Please let me know if I missed something and I'll be happy to correct the patch. Thanks  = ) ============================================================ From malloc()'s man page: free() frees the memory space pointed to by ptr, which must have been returned by a previous call to malloc(), calloc() or realloc().  Otherwise, or if free(ptr) has already been called before, undefined behaviour occurs. If ptr is NULL, no operation is performed. By: Matthew Fredrickson (mattf) 2006-02-02 09:10:31.000-0600 Applied to trunk By: Matthew Fredrickson (mattf) 2006-02-02 09:16:31.000-0600 Put in 1.2 as well By: Digium Subversion (svnbot) 2008-01-15 16:31:29.000-0600 Repository: asterisk Revision: 9073 U   branches/1.2/res/res_odbc.c ------------------------------------------------------------------------ r9073 | mattf | 2008-01-15 16:31:29 -0600 (Tue, 15 Jan 2008) | 2 lines Fix for (ASTERISK-6149), potential (highly unlikely) memory leak in res_odbc ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=9073 By: Digium Subversion (svnbot) 2008-01-15 16:34:36.000-0600 Repository: asterisk Revision: 9279 _U  team/oej/managerstuff/ U   team/oej/managerstuff/Makefile U   team/oej/managerstuff/apps/Makefile U   team/oej/managerstuff/apps/app_macro.c U   team/oej/managerstuff/cdr/Makefile U   team/oej/managerstuff/channels/chan_iax2.c U   team/oej/managerstuff/channels/chan_oss.c U   team/oej/managerstuff/logger.c U   team/oej/managerstuff/res/res_odbc.c ------------------------------------------------------------------------ r9279 | oej | 2008-01-15 16:34:36 -0600 (Tue, 15 Jan 2008) | 42 lines Merged revisions 9073,9086,9156,9232-9233,9246,9262 via svnmerge from https://origsvn.digium.com/svn/asterisk/branches/1.2 ........ r9073 | mattf | 2006-02-02 17:12:13 +0100 (Thu, 02 Feb 2006) | 2 lines Fix for (ASTERISK-6149), potential (highly unlikely) memory leak in res_odbc ........ r9086 | kpfleming | 2006-02-02 19:37:04 +0100 (Thu, 02 Feb 2006) | 2 lines don't override ASTERISKVERSIONNUM to 000000 for non-svn builds ........ r9156 | tilghman | 2006-02-05 18:10:19 +0100 (Sun, 05 Feb 2006) | 2 lines Bug 6176 - Fix race condition ........ r9232 | mogorman | 2006-02-08 23:12:34 +0100 (Wed, 08 Feb 2006) | 4 lines Make logger report error,warning,notice if logger.conf not found, also updated chan_oss to give correct error message if its config file is not found. ........ r9233 | tilghman | 2006-02-08 23:34:38 +0100 (Wed, 08 Feb 2006) | 2 lines Leave it to RH/CentOS to put the freetds headers in a completely nonstandard location. ........ r9246 | russell | 2006-02-09 02:24:55 +0100 (Thu, 09 Feb 2006) | 2 lines reload peercontext on iax2 reload (issue ASTERISK-6279) ........ r9262 | russell | 2006-02-09 03:31:21 +0100 (Thu, 09 Feb 2006) | 2 lines add another location for postgresql headers (issue ASTERISK-6257) ........ ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=9279 By: Digium Subversion (svnbot) 2008-01-15 16:34:38.000-0600 Repository: asterisk Revision: 9281 _U  team/oej/metermaids/ U   team/oej/metermaids/Makefile U   team/oej/metermaids/apps/Makefile U   team/oej/metermaids/apps/app_macro.c U   team/oej/metermaids/cdr/Makefile U   team/oej/metermaids/channels/chan_iax2.c U   team/oej/metermaids/channels/chan_oss.c U   team/oej/metermaids/logger.c U   team/oej/metermaids/res/res_odbc.c ------------------------------------------------------------------------ r9281 | oej | 2008-01-15 16:34:38 -0600 (Tue, 15 Jan 2008) | 42 lines Merged revisions 9073,9086,9156,9232-9233,9246,9262 via svnmerge from https://origsvn.digium.com/svn/asterisk/branches/1.2 ........ r9073 | mattf | 2006-02-02 17:12:13 +0100 (Thu, 02 Feb 2006) | 2 lines Fix for (ASTERISK-6149), potential (highly unlikely) memory leak in res_odbc ........ r9086 | kpfleming | 2006-02-02 19:37:04 +0100 (Thu, 02 Feb 2006) | 2 lines don't override ASTERISKVERSIONNUM to 000000 for non-svn builds ........ r9156 | tilghman | 2006-02-05 18:10:19 +0100 (Sun, 05 Feb 2006) | 2 lines Bug 6176 - Fix race condition ........ r9232 | mogorman | 2006-02-08 23:12:34 +0100 (Wed, 08 Feb 2006) | 4 lines Make logger report error,warning,notice if logger.conf not found, also updated chan_oss to give correct error message if its config file is not found. ........ r9233 | tilghman | 2006-02-08 23:34:38 +0100 (Wed, 08 Feb 2006) | 2 lines Leave it to RH/CentOS to put the freetds headers in a completely nonstandard location. ........ r9246 | russell | 2006-02-09 02:24:55 +0100 (Thu, 09 Feb 2006) | 2 lines reload peercontext on iax2 reload (issue ASTERISK-6279) ........ r9262 | russell | 2006-02-09 03:31:21 +0100 (Thu, 09 Feb 2006) | 2 lines add another location for postgresql headers (issue ASTERISK-6257) ........ ------------------------------------------------------------------------ http://svn.digium.com/view/asterisk?view=rev&revision=9281