pg.ddx.io pgsql-bugs@postgresql.org mailing list archive
help / color / mirror / Atom feed[PATCH] Fix segfault calling PQflush on invalid connection
6+ messages / 3 participants
[nested] [flat]
Commits from this thread:
2022-08-15 bb9237a129 c19024d74e ee4a17e200 2a280832e1 e37e9a6551 ef9f35964d d63a69157d Add missing bad-PGconn guards in libpq entry points.
* [PATCH] Fix segfault calling PQflush on invalid connection
@ 2022-08-15 00:44 Daniele Varrazzo <daniele.varrazzo@gmail.com>
0 siblings, 1 reply; 6+ messages in thread
From: Daniele Varrazzo @ 2022-08-15 00:44 UTC (permalink / raw)
To: pgsql-bugs
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
Attachments:
0001-Fix-segfault-calling-PQflush-on-invalid-connection.patch (text/x-patch, 672B) download
From 31f1bcdb2b10e780d6953b534444a86021eda604 Mon Sep 17 00:00:00 2001
From: Daniele Varrazzo <daniele.varrazzo@gmail.com>
Date: Mon, 15 Aug 2022 02:41:39 +0200
Subject: [PATCH] Fix segfault calling PQflush on invalid connection
---
src/interfaces/libpq/fe-exec.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/src/interfaces/libpq/fe-exec.c b/src/interfaces/libpq/fe-exec.c
index a36f5eb310d..8c2db77ba3f 100644
--- a/src/interfaces/libpq/fe-exec.c
+++ b/src/interfaces/libpq/fe-exec.c
@@ -3936,6 +3936,8 @@ PQisthreadsafe(void)
int
PQflush(PGconn *conn)
{
+ if (!conn || conn->status == CONNECTION_BAD)
+ return -1;
return pqFlush(conn);
}
--
2.25.1
* Re: [PATCH] Fix segfault calling PQflush on invalid connection
@ 2022-08-15 01:44 Tom Lane <tgl@sss.pgh.pa.us>
parent: Daniele Varrazzo <daniele.varrazzo@gmail.com>
0 siblings, 1 reply; 6+ messages in thread
From: Tom Lane @ 2022-08-15 01:44 UTC (permalink / raw)
To: Daniele Varrazzo <daniele.varrazzo@gmail.com>; +Cc: pgsql-bugs
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
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: [PATCH] Fix segfault calling PQflush on invalid connection
@ 2022-08-15 11:20 Francisco Olarte <folarte@peoplecall.com>
parent: Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 6+ messages in thread
From: Francisco Olarte @ 2022-08-15 11:20 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Daniele Varrazzo <daniele.varrazzo@gmail.com>; pgsql-bugs
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.
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: [PATCH] Fix segfault calling PQflush on invalid connection
@ 2022-08-15 14:57 Daniele Varrazzo <daniele.varrazzo@gmail.com>
parent: Francisco Olarte <folarte@peoplecall.com>
0 siblings, 2 replies; 6+ messages in thread
From: Daniele Varrazzo @ 2022-08-15 14:57 UTC (permalink / raw)
To: Francisco Olarte <folarte@peoplecall.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; pgsql-bugs
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/...
-- Daniele
-- Daniele
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: [PATCH] Fix segfault calling PQflush on invalid connection
@ 2022-08-15 15:54 Tom Lane <tgl@sss.pgh.pa.us>
parent: Daniele Varrazzo <daniele.varrazzo@gmail.com>
1 sibling, 0 replies; 6+ messages in thread
From: Tom Lane @ 2022-08-15 15:54 UTC (permalink / raw)
To: Daniele Varrazzo <daniele.varrazzo@gmail.com>; +Cc: Francisco Olarte <folarte@peoplecall.com>; pgsql-bugs
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
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: [PATCH] Fix segfault calling PQflush on invalid connection
@ 2022-08-16 16:08 Francisco Olarte <folarte@peoplecall.com>
parent: Daniele Varrazzo <daniele.varrazzo@gmail.com>
1 sibling, 0 replies; 6+ messages in thread
From: Francisco Olarte @ 2022-08-16 16:08 UTC (permalink / raw)
To: Daniele Varrazzo <daniele.varrazzo@gmail.com>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; pgsql-bugs
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.
^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2022-08-16 16:08 UTC | newest]
Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2022-08-15 00:44 [PATCH] Fix segfault calling PQflush on invalid connection Daniele Varrazzo <daniele.varrazzo@gmail.com>
2022-08-15 01:44 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-08-15 11:20 ` Francisco Olarte <folarte@peoplecall.com>
2022-08-15 14:57 ` Daniele Varrazzo <daniele.varrazzo@gmail.com>
2022-08-15 15:54 ` Tom Lane <tgl@sss.pgh.pa.us>
2022-08-16 16:08 ` Francisco Olarte <folarte@peoplecall.com>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox