CC_lookup_cs_new() bug

am 27.07.2006 15:52:12 von David Chappelle

This is a multi-part message in MIME format.

------_=_NextPart_001_01C6B183.DC7A43D9
Content-Type: text/plain;
charset="us-ascii"
Content-Transfer-Encoding: quoted-printable

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

=20

#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

=20

328 static char *

329 CC_lookup_cs_new(ConnectionClass *self)

330 {

331 char *encstr =3D NULL; =20

332 QResultClass *res;

333

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

335 if (res)

336 {

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

338

339 if (enc)

340 encstr =3D strdup(enc);

341 QR_Destructor(res);

342 }

343 return encstr;

344 }

=20

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:

=20

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 =3D 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 =3D QR_command_maybe_successful(res);

674 QR_Destructor(res);

=20

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.

=20

=20

=20

Questions/Comments

=20

=20

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

=20

------_=_NextPart_001_01C6B183.DC7A43D9
Content-Type: text/html;
charset="us-ascii"
Content-Transfer-Encoding: quoted-printable

xmlns:w=3D"urn:schemas-microsoft-com:office:word" =
xmlns=3D"http://www.w3.org/TR/REC-html40">

charset=3Dus-ascii">

style=3D'font-size:10.0pt;
font-family:Arial'>There is a bug in the function =
CC_lookup_cs_new(…) in
connections.c. Here is the stack trace from the =
crash.

style=3D'font-size:10.0pt;
font-family:Arial'>

style=3D'font-size:10.0pt;
font-family:Arial'>#0 0x7a748947 in TL_get_fieldval () from
/usr/local/lib/psqlodbc.so

style=3D'font-size:10.0pt;
font-family:Arial'>#1 0x7a74dfc9 in CC_lookup_cs_new () from
/usr/local/lib/psqlodbc.so

style=3D'font-size:10.0pt;
font-family:Arial'>#2 0x7a74e151 in CC_lookup_characterset () from
/usr/local/lib/psqlodbc.so

style=3D'font-size:10.0pt;
font-family:Arial'>#3 0x7a72be6f in CC_connect () from
/usr/local/lib/psqlodbc.so

style=3D'font-size:10.0pt;
font-family:Arial'>#4 0x7a734afb in PGAPI_DriverConnect () from
/usr/local/lib/psqlodbc.so

style=3D'font-size:10.0pt;
font-family:Arial'>#5 0x7a74ba99 in SQLDriverConnect () from
/usr/local/lib/psqlodbc.so

style=3D'font-size:10.0pt;
font-family:Arial'>#6 0x68b58a92 in SQLDriverConnect () from
/usr/local/lib/libodbc.so.1

style=3D'font-size:10.0pt;
font-family:Arial'>

style=3D'font-size:10.0pt;
font-family:Arial'> 328 static char =
*

style=3D'font-size:10.0pt;
font-family:Arial'> 329 =
CC_lookup_cs_new(ConnectionClass
*self)

style=3D'font-size:10.0pt;
font-family:Arial'> 330 {

style=3D'font-size:10.0pt;
font-family:Arial'> 331
char *encstr =3D NULL; =

style=3D'font-size:10.0pt;
font-family:Arial'> 332
QResultClass *res;

style=3D'font-size:10.0pt;
font-family:Arial'> 333

style=3D'font-size:10.0pt;
font-family:Arial'> 334 res =
=3D
CC_send_query(self, "select pg_client_encoding()", NULL,
CLEAR_RESULT_ON_ABORT);

style=3D'font-size:10.0pt;
font-family:Arial'> 335 if =
(res)

style=3D'font-size:10.0pt;
font-family:Arial'> 336 =
{

style=3D'font-size:10.0pt;
font-family:Arial'>
337         =
char    *enc
=3D QR_get_value_backend_row(res, 0, 0);

style=3D'font-size:10.0pt;
font-family:Arial'> 338

style=3D'font-size:10.0pt;
font-family:Arial'>
339 if =
(enc)

style=3D'font-size:10.0pt;
font-family:Arial'>
340 &nbs=
p;
encstr =3D strdup(enc);

style=3D'font-size:10.0pt;
font-family:Arial'>
341 =
QR_Destructor(res);

style=3D'font-size:10.0pt;
font-family:Arial'> 342 =
}

style=3D'font-size:10.0pt;
font-family:Arial'> 343 return
encstr;

style=3D'font-size:10.0pt;
font-family:Arial'> 344 }

style=3D'font-size:10.0pt;
font-family:Arial'>

style=3D'font-size:10.0pt;
font-family:Arial'>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:

style=3D'font-size:10.0pt;
font-family:Arial'>

style=3D'font-size:10.0pt;
font-family:Arial'> 659 /* If =
manual
commit and in transaction, then proceed. */

style=3D'font-size:10.0pt;
font-family:Arial'> 660 if
(!CC_is_in_autocommit(conn) && =
CC_is_in_trans(conn))

style=3D'font-size:10.0pt;
font-family:Arial'> 661 =
{

style=3D'font-size:10.0pt;
font-family:Arial'>
662 =
mylog("PGAPI_Transact:
sending on conn %d '%s'\n", conn, =
stmt_string);

style=3D'font-size:10.0pt;
font-family:Arial'> 663

style=3D'font-size:10.0pt;
font-family:Arial'>
664 res =3D =
CC_send_query(conn,
stmt_string, NULL, CLEAR_RESULT_ON_ABORT);

style=3D'font-size:10.0pt;
font-family:Arial'>
665 if =
(!res)

style=3D'font-size:10.0pt;
font-family:Arial'>
666 =
{

style=3D'font-size:10.0pt;
font-family:Arial'>
667 &nbs=
p; /*
error msg will be in the connection */

style=3D'font-size:10.0pt;
font-family:Arial'>
668 &nbs=
p;
CC_on_abort(conn, NO_TRANS);

style=3D'font-size:10.0pt;
font-family:Arial'>
669 &nbs=
p;
CC_log_error(func, "", conn);

style=3D'font-size:10.0pt;
font-family:Arial'>
670 &nbs=
p;
return SQL_ERROR;

style=3D'font-size:10.0pt;
font-family:Arial'>
671 =
}

style=3D'font-size:10.0pt;
font-family:Arial'> 672

style=3D'font-size:10.0pt;
font-family:Arial'>
673 ok =3D
QR_command_maybe_successful(res);

style=3D'font-size:10.0pt;
font-family:Arial'>
674 =
QR_Destructor(res);

style=3D'font-size:10.0pt;
font-family:Arial'>

style=3D'font-size:10.0pt;
font-family:Arial'>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.

style=3D'font-size:10.0pt;
font-family:Arial'>

style=3D'font-size:10.0pt;
font-family:Arial'>Questions/Comments

style=3D'font-size:10.0pt;
font-family:Arial'>

style=3D'font-size:10.0pt'>David
Chappelle

Software Engineer

519.880.2400 ext.3005

Sandvine Incorporated

=
size=3D2>

style=3D'font-size:
12.0pt'>

------_=_NextPart_001_01C6B183.DC7A43D9--