CC_lookup_cs_new() bug

There is a bug in the function CC_lookup_cs_new(...) in connections.c.
Here is the stack trace from the crash.

#0 0x7a748947 in TL_get_fieldval () from /usr/local/lib/psqlodbc.so

#1 0x7a74dfc9 in CC_lookup_cs_new () from /usr/local/lib/psqlodbc.so

#2 0x7a74e151 in CC_lookup_characterset () from
/usr/local/lib/psqlodbc.so

#3 0x7a72be6f in CC_connect () from /usr/local/lib/psqlodbc.so

#4 0x7a734afb in PGAPI_DriverConnect () from /usr/local/lib/psqlodbc.so

#5 0x7a74ba99 in SQLDriverConnect () from /usr/local/lib/psqlodbc.so

#6 0x68b58a92 in SQLDriverConnect () from /usr/local/lib/libodbc.so.1

328 static char *

329 CC_lookup_cs_new(ConnectionClass *self)

330 {

331 char *encstr = NULL;

332 QResultClass *res;

333

334 res = CC_send_query(self, "select pg_client_encoding()",
NULL, CLEAR_RESULT_ON_ABORT);

335 if (res)

336 {

337 char *enc = QR_get_value_backend_row(res, 0, 0);

338

339 if (enc)

340 encstr = strdup(enc);

341 QR_Destructor(res);

342 }

343 return encstr;

344 }

The call to CC_send_query returns a non-null result if PGRES_FATAL_ERROR
occurs and then passes the result to TL_get_fieldval via the macro
QR_get_value_backend_row without checking the status of res. In
execute.c I noticed that the result returned from CC_send_query is
checked appropriately as follows:

659 /* If manual commit and in transaction, then proceed. */

660 if (!CC_is_in_autocommit(conn) && CC_is_in_trans(conn))

661 {

662 mylog("PGAPI_Transact: sending on conn %d '%s'\n", conn,
stmt_string);

663

664 res = CC_send_query(conn, stmt_string, NULL,
CLEAR_RESULT_ON_ABORT);

665 if (!res)

666 {

667 /* error msg will be in the connection */

668 CC_on_abort(conn, NO_TRANS);

669 CC_log_error(func, "", conn);

670 return SQL_ERROR;

671 }

672

673 ok = QR_command_maybe_successful(res);

674 QR_Destructor(res);

So the solution would appear to be adding a
QR_command_maybe_successful(res) check in CC_lookup_cs_new. However, why
not just handle this inside send query and return NULL if the result is
bad? Especially if the flag CLEAR_RESULT_ON_ABORT leads you to believe
this is the desired behaviour.

Questions/Comments

David Chappelle
Software Engineer
519.880.2400 ext.3005
Sandvine Incorporated
http://www.sandvine.com