Thread: [PATCH] Fix segfault calling PQflush on invalid connection

PQflush calls pqFlush, which performs struct access to the connection
without checking if it's valid, resulting in a segfault if called with
a null pointer.

Please find attached a patch adding a guard to PQflush().

Cheers

-- Daniele

Daniele Varrazzo <daniele.varrazzo@gmail.com> writes:
> PQflush calls pqFlush, which performs struct access to the connection
> without checking if it's valid, resulting in a segfault if called with
> a null pointer.

> Please find attached a patch adding a guard to PQflush().

Seems reasonable, but this tickled a thought that's been in my
hindbrain for awhile: just checking for a null pointer is not
much of a check for being passed a valid PGconn pointer.  Should
we add a magic number to struct PGconn, and modify all libpq's
entry points along the lines of

    if (!conn || conn->magic != PGCONN_MAGIC)
        return failure;

I'm honestly not entirely sure if this is worth the trouble;
I've not heard of many application bugs that this would've caught.
But the lack of any such check does seem like it's not up to
modern standards.

            regards, tom lane

Tom:

On Mon, 15 Aug 2022 at 03:44, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Please find attached a patch adding a guard to PQflush().
> Seems reasonable, but this tickled a thought that's been in my
> hindbrain for awhile: just checking for a null pointer is not
> much of a check for being passed a valid PGconn pointer.

Is there any place in the docs which states libpq just errors out ( as
opposed to dump core ) on null PGconn? I could not find it easily, and
I have always assumed the worst ( normally I use it wrapped, use
nullptr as invalid marked in my data and check that (and either do
nothing or intentionally SEGV for easier debugging ).

Even in PQstatus, which is the first one I would expect to tolerate a
null ptr, I have not been able to see it in the docs.

>  Should
> we add a magic number to struct PGconn, and modify all libpq's
> entry points along the lines of
>         if (!conn || conn->magic != PGCONN_MAGIC)
>                 return failure;
> I'm honestly not entirely sure if this is worth the trouble;
> I've not heard of many application bugs that this would've caught.

IMO stray pointers will lead to SEGV a little further down so it would
not catch many not already caught.


> But the lack of any such check does seem like it's not up to
> modern standards.

If conn->magic is assumed to be readable using and unsigned conn->idx
and an auxiliary table in the library holding the pointers it could be
made totally safe, check (conn && conn->idx<table_size &&
conn_table[conn->idx}==conn), find a free slot on conn creation ( I
assume this would need a lock plus a scan of the connection table, but
it seems tolerable for a connection build ), null the slot on freeing
pgconns. I'm probably forgetting something, and it is slower and more
cache-thrashing than just a magic check, but it seems fast enough for
DB access package.

But anyway, for this to be useful libpq should document the behaviour
of passing bad ptrs. IMO even the null ptr check is too much ( and can
lead to delayed bugs making tracing them more difficult, the nice
thing of not checking is a null dereference is easy to catch and
backtrace in a debugger. ) for anything not like PQfinish ( which I do
not see documented as tolerating nulls, but probably doing nothing,
like free(3), is useful there ). BTW, I do not see any reference of
PQconnect* being able to return null, but I suppose it is a
possibility on OOM.


Francisco Olarte.

On Mon, 15 Aug 2022 at 13:21, Francisco Olarte <folarte@peoplecall.com> wrote:

> Is there any place in the docs which states libpq just errors out ( as
> opposed to dump core ) on null PGconn? I could not find it easily, and
> I have always assumed the worst ( normally I use it wrapped, use
> nullptr as invalid marked in my data and check that (and either do
> nothing or intentionally SEGV for easier debugging ).

After exercising most of the libpq API in psycopg 3, PQflush() seems
the only one requiring a guard [1]. All the other functions seem to
behave well when passed an "invalid connection" - aka either a NULL
pointer or a connection in BAD status.

[1]
https://github.com/psycopg/psycopg/blob/923bb834ce7601f2743d10bff7e007698429933a/psycopg/psycopg/pq/pq_ctypes.py#L541-L548

-- Daniele


-- Daniele

Daniele Varrazzo <daniele.varrazzo@gmail.com> writes:
> After exercising most of the libpq API in psycopg 3, PQflush() seems
> the only one requiring a guard [1]. All the other functions seem to
> behave well when passed an "invalid connection" - aka either a NULL
> pointer or a connection in BAD status.

I went so far as to inspect every function listed in libpq/exports.txt.
All the ones that take a PGconn or PGresult check it for NULL except

PQflush
PQisnonblocking
PQdefaultSSLKeyPassHook_OpenSSL

PQisnonblocking is clearly an oversight, same as PQflush.
Maybe there is a case for not bothering to check in
PQdefaultSSLKeyPassHook_OpenSSL, on the grounds that you'd
not get that far without a connection, but I think for
consistency it should do so too.

There is not a lot of consistency about whether we insist on
the connection being in any particular "good" state.  For
some of the functions I think it's intentional that they should
still work on a failed connection, but I wonder if any of those
are likewise oversights.

            regards, tom lane

Daniele:

On Mon, 15 Aug 2022 at 16:58, Daniele Varrazzo
<daniele.varrazzo@gmail.com> wrote:
> On Mon, 15 Aug 2022 at 13:21, Francisco Olarte <folarte@peoplecall.com> wrote:
> > Is there any place in the docs which states libpq just errors out ( as
> > opposed to dump core ) on null PGconn? I could not find it easily, and
> > I have always assumed the worst ( normally I use it wrapped, use
> > nullptr as invalid marked in my data and check that (and either do
> > nothing or intentionally SEGV for easier debugging ).
> After exercising most of the libpq API in psycopg 3, PQflush() seems
> the only one requiring a guard [1]. All the other functions seem to
> behave well when passed an "invalid connection" - aka either a NULL
> pointer or a connection in BAD status.

If this is the case may be it could be documented, somewhere near the
top with something like "passing a null pgconn will be treated
similarly to passing a connection with BAD status". It can be a nice /
simple feature and greatly simplify some use cases. Having been bitten
before I normally assume only what is documented is guaranteed /
reportable as a bug, and sometimes not even all of it.

Francisco Olarte.