Summary:	ASTERISK-07736: [patch] Use of PQescapeString() compromises thread safety
Reporter:	Jeffrey Frey (freyguy)	Labels:
Date Opened:	2006-09-12 16:15:00	Date Closed:	2011-06-07 14:08:23
Priority:	Minor	Regression?	No
Status:	Closed/Complete	Components:	CDR/cdr_pgsql
Versions:		Frequency of Occurrence
Related Issues:
Environment:		Attachments:	( 0) 20061112__bug7946.diff.txt
Description:	If the PostgreSQL documentation is consulted, it may be noted that PQescapeString() is a deprecated function that is not thread safe. Building PostgreSQL 8.1.4 with threading enabled, I added the cdr_pgsql module to my Asterisk server and observed several subsequent soft crashes of Asterisk -- the daemon did not terminate, it merely ended up in a state whereby all phones were dead.  I wish I had some debugging logs for you, but this is a production system and we did not have that enabled at the time.  Disabling cdr_pgsql.so has kept the problem from occuring again, though, so it seems safe enough to assume that it was the root of the issue. ****** ADDITIONAL INFORMATION ****** It is possible to avoid the issue entirely if the cdr_pgsql module's logging function is modified to use the PQexecParams() function rather than a text-based PQexec() call.  This avoids any need to allocate additional memory for every logged field, and the escape-ing no longer needs to be done.  I've included a modified cdr_pgsql.c that implements this change. There was a second minor issue in cdr_pgsql.c whereby if the initial PQexec() failed and the database connection was reopened and a second PQexec() succeeded, the logging function would still return in error (-1).
Comments:	By: Tilghman Lesher (tilghman) 2006-09-12 18:34:16 For us to consider this, you need to have a disclaimer on file.  Also, please produce as a unified diff (diff -u) to the SVN tree (svn diff). By: Raphaël Jacquot (sxpert) 2006-09-13 03:29:42 According to the documentation there's a simpler way to do this, which is to use the PQescapeStringConn API that is slated to replace the deprecated PQescapeString function. see http://www.postgresql.org/docs/8.1/interactive/libpq-exec.html#LIBPQ-EXEC-ESCAPE-STRING By: Jeffrey Frey (freyguy) 2006-09-13 08:12:24 IMHO I don't see it as simpler for one major reason:  inside the logging function we're doing a bunch of malloc's for 2x the string length, then escaping the strings, then doing the logging, then free'ing all those memory segments.  Why do all that when the Postgres API includes a function that allows us to log the data _without_ the overhead of memory allocation and string escaping?  That seems simpler to me. By: Raphaël Jacquot (sxpert) 2006-09-13 08:21:40 because this was the path of least changes, and could be accepted as a bugfix in 1.4. your patch can then be applied for after 1.4 By: jmls (jmls) 2006-11-01 10:39:27.000-0600 Freyguy, did you send your disclaimer as requested ? By: Tilghman Lesher (tilghman) 2006-11-12 23:01:25.000-0600 Following sxpert's recommendation, here's a patch file which does the conversion to PQescapeStringConn(), which probably can be merged to 1.2, once it has been tested. Needs testing (add feedback to a bugnote). By: Serge Vecher (serge-v) 2006-11-17 09:21:34.000-0600 deleting non-disclaimed code. By: Tilghman Lesher (tilghman) 2006-12-11 00:13:47.000-0600 No response from reporter.  Please reopen if/when you have testing results (or ask a bug marshal to reopen if you are unable).