Re: libpgtcl bug (and symptomatic treatment) - Mailing list pgsql-hackers

Over in pgsql-patches, Magosanyi Arpad <mag@bunuel.tii.matav.hu> wrote:
> libpgtcl segmentation faults in any pg_exec call, if it fails for any reason
> There is a patch which has worked for me. The real problem might be in
> PQreset, which can't reset a conninfo based connection. The patch:

> - --- pgtclCmds.c    1998/05/27 10:54:36    1.1
> +++ pgtclCmds.c    1998/05/27 10:58:07
> @@ -454,7 +454,7 @@
>      else {
>      /* error occurred during the query */
>      Tcl_SetResult(interp, conn->errorMessage, TCL_STATIC);
> - -    if (connStatus == CONNECTION_OK) {
> +    if (connStatus != CONNECTION_OK) {
>          PQreset(conn);
>          if (conn->status == CONNECTION_OK) {
>          result = PQexec(conn, argv[2]);
> - --

Actually, that entire block of "error recovery" code looks thoroughly
bogus to me.  I thought seriously about just ripping it out when I was
modifying libpgtcl last week, but I refrained.  Now I think I should've.
(For starters, the Tcl_SetResult call is wrong --- TCL_STATIC says that
the string passed to Tcl_SetResult is a constant.  But if the PQreset
path is taken then the error message will be overwritten; the Tcl code
will not see the original error message, but whatever is left there
after the reconnection.  Together with Magosanyi's observation that the
if-test is backwards, it seems clear that this section of the code has
never been tested or debugged.)  The larger point is that I don't think
this low-level routine has any business calling PQreset.  Blowing away
the connection and making another is a sledgehammer recovery method
that ought only be invoked by the application, not by library routines.
I don't like PQendcopy's use of PQreset either, and would like to take
that out too.  Any comments?

But the real reason I'm writing this message is the comment about PQreset
possibly failing.  I know of one case in which PQreset will not work:
if the database requires a password then PQreset will fail.  (Why, you
ask?  Because connectDB() in fe-connect.c deliberately erases the
password after the first successful connection.)  Is this the situation
you are running into, Magosanyi?  Or is there another problem in there?
It seems to me that the password issue should only result in a failed
reconnection, not a coredump.  Where exactly is the segfault occurring?

I have been intending to propose that connectDB's deletion of the
password be removed.  The security gain is marginal, if not completely
illusory.  (If a bad guy has access to the client's address space,
whether he can find the password is the least of your worries.  Besides,
where did the password come from?  There are probably other copies of
it outside libpq's purview.)  So I don't think it's worth breaking
PQreset for.

Alternatively, we could eliminate PQreset entirely.  It doesn't really
do anything that the client application can't do for itself (just close
and re-open the connection; two lines instead of one) and its presence
seems to encourage the use of poorly-considered error "recovery"
schemes...

<end rant>

            regards, tom lane