Re: [HACKERS] Re: libpgtcl bug (and symptomatic treatment)

>
> Over in pgsql-patches, Magosanyi Arpad <mag(at)bunuel(dot)tii(dot)matav(dot)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?

Please, do whatever you think is best in this area.

>
> 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 saw a comment around this code a week ago, saying it breaks PQreset(),
and was going to remove it myself, with a comment to this list in case
some else mentioned a problem. Yes, please remove the password erasure.

>
> 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.

Yes, if they can see the address space, they can see the password typed
in. If the app coredumps, they can read the password IF they have
access to the core file, but again, why would they?

>
> 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...

Interesting.

--
Bruce Momjian | 830 Blythe Avenue
maillist(at)candle(dot)pha(dot)pa(dot)us | Drexel Hill, Pennsylvania 19026
+ If your life is a hard drive, | (610) 353-9879(w)
+ Christ can be your backup. | (610) 853-3000(h)