Thread: Re: libpgtcl bug (and symptomatic treatment)
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
>
> 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?
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@candle.pha.pa.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)
[please cc: to me the answer, as I am not on the mailinglist (yet?)] A levelezőm azt hiszi, hogy Tom Lane a következőeket írta: > > 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? Exactly, the connection had a password. I can't tell you exactly where the core dump occurred, but surely it was inside PQreset. > > 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. And anyway the password goes out plaintext on the net (okay, it can go crypt()ed, but the crypted version is also enough to connect to your postgres account, should someone snooping on the net). As setting up kerberos is a PITA, especially for us in the free world (in cryptoexportlaw sense), is it possible to hack in some other light yet unsnoopable authentication method? (SRP comes to mind) Also, the encryption of the connections would be a nifty thing. [I am aware of the following facts: (1) there is kerberos also in the free world, (2) with ssh port forwarding I can work around the 'plain on the net' problem.] > > 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... I guess it would break compatibility. Maybe a two-step method would be worth considering: first insert a warning (to stderr), that PQreset is considered harmful. After some time remove it. Maybe it is not worth to do the second step. -- GNU GPL: csak tiszta forrásból