[asterisk-commits] murf: branch 1.4 r150056 - /branches/1.4/cdr/cdr_odbc.c

[SVN commits to the Asterisk project]

Author: murf
Date: Thu Oct 16 10:26:10 2008
New Revision: 150056

URL: http://svn.digium.com/view/asterisk?view=rev&rev=150056
Log:
This patch is relevant to:

ABE-1628 and RYM-150398 and AST-103 in internal Digium 
bug trackers.

These fixes address a really subtle memory corruption
problem that would happen in machines heavily loaded
in production environments. The corruption would
always take the form of the STMT object getting
nulled out and one of the unixODBC calls would
crash trying to access statement->connection.

It isn't fully proven yet, but the server has
now been running 2.5 days without appreciable
memory growth, or any gain of %cpu, and no 
crashes. Whether this is the problem or not
on that server, these fixes are still warranted.

As it turns out, **I** introduced these errors
unwittingly, when I corrected another crash earlier.
I had formed the build_query routine, and failed
to remove mutex_unlock calls in 3 places in the
transplanted code. These unlocks would only
happen in error situations, but unlocking the
mutex early set the code up for a catastrophic
failure, it appears. It would happen only once
every 100K-200K or more calls, under heavy load... 
but that is enough.

If another crash occurs, with the same MO, 
I'll come back and remove my confession from the log, and
we'll keep searching, but the fact that we
have Asterisk dying from an asynchronous
wiping of the STMT object, only on some connection
error, and that the server has lived for 2.5
days on this code without a crash, sure make
it look like this was the problem!

Also, in several points, Statement handles are
set to NULL after SQLFreeHandle. This was mainly
for insurance, to guarantee a crash. As it turns
out, the code does not appear to be attempting
to use these freed pointers.

Asterisk owes a debt of gratitude to Federico Alves
and Frediano Ziglio for their untiring efforts in
finding this bug, among others.



Modified:
    branches/1.4/cdr/cdr_odbc.c

Modified: branches/1.4/cdr/cdr_odbc.c
URL: http://svn.digium.com/view/asterisk/branches/1.4/cdr/cdr_odbc.c?view=diff&rev=150056&r1=150055&r2=150056
==============================================================================
--- branches/1.4/cdr/cdr_odbc.c (original)
+++ branches/1.4/cdr/cdr_odbc.c Thu Oct 16 10:26:10 2008
@@ -84,9 +84,12 @@
 
 static void odbc_disconnect(void)
 {
+	ODBC_stmt = SQL_NULL_HANDLE;
 	SQLDisconnect(ODBC_con);
 	SQLFreeHandle(SQL_HANDLE_DBC, ODBC_con);
+	ODBC_con = SQL_NULL_HANDLE;
 	SQLFreeHandle(SQL_HANDLE_ENV, ODBC_env);
+	ODBC_env = SQL_NULL_HANDLE;
 	connected = 0;
 }
 
@@ -119,7 +122,6 @@
 		res = odbc_init();
 		if (res < 0) {
 			odbc_disconnect();
-			ast_mutex_unlock(&odbc_lock);
 			return;
 		}				
 	}
@@ -130,8 +132,8 @@
 		if (option_verbose > 10)
 			ast_verbose( VERBOSE_PREFIX_4 "cdr_odbc: Failure in AllocStatement %d\n", ODBC_res);
 		SQLFreeHandle(SQL_HANDLE_STMT, ODBC_stmt);
+		ODBC_stmt = SQL_NULL_HANDLE;
 		odbc_disconnect();
-		ast_mutex_unlock(&odbc_lock);
 		return;
 	}
 
@@ -145,8 +147,8 @@
 		if (option_verbose > 10)
 			ast_verbose( VERBOSE_PREFIX_4 "cdr_odbc: Error in PREPARE %d\n", ODBC_res);
 		SQLFreeHandle(SQL_HANDLE_STMT, ODBC_stmt);
+		ODBC_stmt = SQL_NULL_HANDLE;
 		odbc_disconnect();
-		ast_mutex_unlock(&odbc_lock);
 		return;
 	}
 
@@ -197,6 +199,7 @@
 				if (option_verbose > 10)
 					ast_verbose( VERBOSE_PREFIX_4 "cdr_odbc: Trying Query again!\n");
 				SQLFreeHandle(SQL_HANDLE_STMT, ODBC_stmt);
+				ODBC_stmt = SQL_NULL_HANDLE;
 				build_query(cdr, timestr, sizeof(timestr)); /* what a waste. If we have to reconnect, we have to build a new query */
 				res = odbc_do_query();
 				if (res < 0) {
@@ -210,6 +213,7 @@
 			ast_verbose( VERBOSE_PREFIX_4 "cdr_odbc: Query FAILED Call not logged!\n");
 	}
 	SQLFreeHandle(SQL_HANDLE_STMT, ODBC_stmt);
+	ODBC_stmt = SQL_NULL_HANDLE;
 	ast_mutex_unlock(&odbc_lock);
 	return 0;
 }
@@ -221,6 +225,7 @@
 		if (option_verbose > 10)
 			ast_verbose( VERBOSE_PREFIX_4 "cdr_odbc: Disconnecting from %s\n", dsn);
 		SQLFreeHandle(SQL_HANDLE_STMT, ODBC_stmt);
+		ODBC_stmt = SQL_NULL_HANDLE;
 		odbc_disconnect();
 	}
 	if (dsn) {
@@ -414,6 +419,7 @@
 			if (option_verbose > 10)
 				ast_verbose( VERBOSE_PREFIX_4 "cdr_odbc: Error SetEnv\n");
 			SQLFreeHandle(SQL_HANDLE_ENV, ODBC_env);
+			ODBC_env = SQL_NULL_HANDLE;
 			connected = 0;
 			return -1;
 		}
@@ -424,6 +430,7 @@
 			if (option_verbose > 10)
 				ast_verbose( VERBOSE_PREFIX_4 "cdr_odbc: Error AllocHDB %d\n", ODBC_res);
 			SQLFreeHandle(SQL_HANDLE_ENV, ODBC_env);
+			ODBC_env = SQL_NULL_HANDLE;
 			connected = 0;
 			return -1;
 		}
@@ -438,7 +445,9 @@
 		if (option_verbose > 10)
 			ast_verbose( VERBOSE_PREFIX_4 "cdr_odbc: Error SQLConnect %d\n", ODBC_res);
 		SQLFreeHandle(SQL_HANDLE_DBC, ODBC_con);
+		ODBC_con = SQL_NULL_HANDLE;
 		SQLFreeHandle(SQL_HANDLE_ENV, ODBC_env);
+		ODBC_env = SQL_NULL_HANDLE;
 		connected = 0;
 		return -1;
 	} else {