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

Tom:

On Mon, 15 Aug 2022 at 03:44, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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.