Fix PQport to never return NULL if the connection is valid

Started by Daniele Varrazzo8 months ago8 messages

daniele.varrazzo@gmail.com

8 months ago

1 attachment(s)

The documentation of PQport says that the function will never return
NULL, unless the connection is null:
https://www.postgresql.org/docs/17/libpq-status.html#LIBPQ-PQPORT

However this is not the case: it seems to return null when the port
specified is an empty string:

$ PSYCOPG_IMPL=python python3

import psycopg
conn = psycopg.connect("port=")
from psycopg.pq import _pq_ctypes as libpq
print(libpq.PQport(conn.pgconn._pgconn_ptr))

None

I have prepared a patch, which is attached, to fix the issue. However
the patch returns the default port in case of no info in
'conn->connhost'. Re-reading the docs, I find the behaviour confusing.

if there is an error producing the port information (perhaps if
the connection
has not been fully established or there was an error), it returns
an empty string.

If we have a successful connection but no port information I believe
that returning the default port would be the right answer from this
function (it's the port where the connection was established).

If the connection is unsuccessful, the documentation says that this
function will return an empty string. Is this useful info? If that's
the case, maybe we should finish with an if instead?

if (conn->status == CONNECTION_OK)
return DEF_PGPORT_STR;
else
return "";

Or, alternatively, maybe a better implementation would be to copy the
default port on 'conn->connhost[conn->whichhost].port' if the
connection is successful (or even before attempting connection, as to
be able to return the port we tried)?

Cheers

-- Daniele

Attachments:

0001-Fix-PQport-to-never-return-NULL-unless-the-connectio.patchtext/x-patch; charset=US-ASCII; name=0001-Fix-PQport-to-never-return-NULL-unless-the-connectio.patchDownload

From be3a051e8f39ad504cd22e22e80178d57949b7d9 Mon Sep 17 00:00:00 2001
From: Daniele Varrazzo <daniele.varrazzo@gmail.com>
Date: Thu, 8 May 2025 18:34:56 +0200
Subject: [PATCH] Fix PQport to never return NULL unless the connection is NULL

This is the documented behaviour, but, at the moment, if the port is an
empty string, the port is returned null.

Return the default port instead of an empty string if the port is not
set.
---
 src/interfaces/libpq/fe-connect.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 2f87961a71e..7f746aef94e 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -7074,10 +7074,12 @@ PQport(const PGconn *conn)
 	if (!conn)
 		return NULL;
 
-	if (conn->connhost != NULL)
+	if (conn->connhost != NULL &&
+		conn->connhost[conn->whichhost].port != NULL &&
+		conn->connhost[conn->whichhost].port[0] != '\0')
 		return conn->connhost[conn->whichhost].port;
 
-	return "";
+	return DEF_PGPORT_STR;
 }
 
 /*
-- 
2.34.1

Daniele Varrazzo

daniele.varrazzo@gmail.com

8 months ago

In reply to: Daniele Varrazzo (#1)

Re: Fix PQport to never return NULL if the connection is valid

On Thu, 8 May 2025 at 18:58, Daniele Varrazzo
<daniele.varrazzo@gmail.com> wrote:

I have prepared a patch, which is attached, to fix the issue. However
the patch returns the default port in case of no info in
'conn->connhost'.

Ah, of course this is wrong if the PGPORT env var is set and I assume
thanks to the pg_service...

If I understand better what to return and the strategy to return it I
can try and provide a better patch.

Cheers

-- Daniele

Daniele Varrazzo

daniele.varrazzo@gmail.com

8 months ago

In reply to: Daniele Varrazzo (#2)

1 attachment(s)

Re: Fix PQport to never return NULL if the connection is valid

I looked a bit more into the meaning of the port="" setting. The docs
for the port parameter / PGPORT env var
<https://www.postgresql.org/docs/current/libpq-connect.html#LIBPQ-CONNECT-PORT>
say:

An empty string, or an empty item in a comma-separated list,
specifies the default port number established when
PostgreSQL was built

Thus I understand that the return value of PQport wants to reflect
this behaviour, therefore the value "" is legitimate and it's up to
the client to figure out how the libpq was built (PQconndefaults may
tell that).

Please find attached a new patch that doesn't change the behaviour and
just makes sure to not return NULL in case no info is available in
'conn->connhost'.

Cheers

-- Daniele

Attachments:

0001-Fix-PQport-to-never-return-NULL-unless-the-connectio.patchtext/x-patch; charset=US-ASCII; name=0001-Fix-PQport-to-never-return-NULL-unless-the-connectio.patchDownload

From ecd3063613a44309583e553db4485cfa55df34a9 Mon Sep 17 00:00:00 2001
From: Daniele Varrazzo <daniele.varrazzo@gmail.com>
Date: Thu, 8 May 2025 18:34:56 +0200
Subject: [PATCH] Fix PQport to never return NULL unless the connection is NULL

This is the documented behaviour, but, at the moment, if the port in the
connection string is an empty string, the port is returned as NULL by
this function.
---
 src/interfaces/libpq/fe-connect.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 2f87961a71e..454d2ea3fb7 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -7074,7 +7074,9 @@ PQport(const PGconn *conn)
 	if (!conn)
 		return NULL;
 
-	if (conn->connhost != NULL)
+	if (conn->connhost != NULL &&
+		conn->connhost[conn->whichhost].port != NULL &&
+		conn->connhost[conn->whichhost].port[0] != '\0')
 		return conn->connhost[conn->whichhost].port;
 
 	return "";
-- 
2.34.1

Laurenz Albe

laurenz.albe@cybertec.at

6 months ago

In reply to: Daniele Varrazzo (#3)

Re: Fix PQport to never return NULL if the connection is valid

On Thu, 2025-05-08 at 22:01 +0200, Daniele Varrazzo wrote:

I looked a bit more into the meaning of the port="" setting. The docs
for the port parameter / PGPORT env var
<https://www.postgresql.org/docs/current/libpq-connect.html#LIBPQ-CONNECT-PORT>
say:

An empty string, or an empty item in a comma-separated list,
specifies the default port number established when
PostgreSQL was built

Thus I understand that the return value of PQport wants to reflect
this behaviour, therefore the value "" is legitimate and it's up to
the client to figure out how the libpq was built (PQconndefaults may
tell that).

Please find attached a new patch that doesn't change the behaviour and
just makes sure to not return NULL in case no info is available in
'conn->connhost'.

I think that it is important to fix that bug and to backpatch it.
Without the patch, I can produce the following crash:

$ psql port=
psql (18beta1)
Type "help" for help.

test=> \conninfo
Segmentation fault (core dumped)

diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -7574,7 +7574,9 @@ PQport(const PGconn *conn)
if (!conn)
return NULL;

-   if (conn->connhost != NULL)
+   if (conn->connhost != NULL &&
+       conn->connhost[conn->whichhost].port != NULL &&
+       conn->connhost[conn->whichhost].port[0] != '\0')
return conn->connhost[conn->whichhost].port;

return "";

You should omit the third test. If the first character of the port
string is 0, the string is empty, and you might as well return it
instead of a static empty string.

The patch applies (with considerable offset), builds well and passes
the regression tests. The latter is unsurprising, since nothing tests
for that. I wondered if it was worth adding a test, but could not
find a convenient place. Adding a TAP test seems a bit out of
proportion for a small fix like that.

Status set to "waiting on author".

Yours,
Laurenz Albe

Tom Lane

tgl@sss.pgh.pa.us

6 months ago

In reply to: Laurenz Albe (#4)

Re: Fix PQport to never return NULL if the connection is valid

Laurenz Albe <laurenz.albe@cybertec.at> writes:

On Thu, 2025-05-08 at 22:01 +0200, Daniele Varrazzo wrote:

Please find attached a new patch that doesn't change the behaviour and
just makes sure to not return NULL in case no info is available in
'conn->connhost'.

I think that it is important to fix that bug and to backpatch it.
Without the patch, I can produce the following crash:

$ psql port=
psql (18beta1)
Type "help" for help.

test=> \conninfo
Segmentation fault (core dumped)

This crash is actually new in v18; earlier versions produce something
like

psql (17.5)
Type "help" for help.

postgres=# \conninfo
You are connected to database "postgres" as user "postgres" via socket in "/tmp" at port "(null)".
postgres=#

because the code was basically

printf(_("You are connected to database \"%s\" as user \"%s\" via socket in \"%s\" at port \"%s\".\n"),
db, PQuser(pset.db), host, PQport(pset.db));

and fortunately our implementation of printf is guaranteed not to
crash on a null string pointer.

Having said that, I agree with back-patching Daniele's fix, because it
is making the code agree with what happened before we introduced the
connhost[] data structure in v10, and also with the documentation:

<xref linkend="libpq-PQport"/> returns <symbol>NULL</symbol> if the
<parameter>conn</parameter> argument is <symbol>NULL</symbol>.
Otherwise, if there is an error producing the port information (perhaps
if the connection has not been fully established or there was an
error), it returns an empty string.

However ... I wonder if we ought to take the opportunity to fix
\conninfo to ensure that it prints something useful rather than
just an empty string? The upgrade to make it print in tabular
format is what broke this, but it also seems like an excuse to
improve the functionality as well as the cosmetics.

However, fixing this in \conninfo seems a bit tedious: we'd have
to grovel through the output of PQconndefaults. (I don't think
it's okay for psql to assume that it can rely on having been
built with the same value of DEF_PGPORT_STR that the libpq it's
linked to is using.) So it seems attractive to stick with the
original thought of making PQport substitute the correct value
for an empty string. PQport *does* know the correct value of
DEF_PGPORT_STR, so it'd be pretty nearly a one-liner to make
this happen there.

I'd only propose doing this in v18/HEAD, and sticking to the
v2 patch in the stable branches.

regards, tom lane

Laurenz Albe

laurenz.albe@cybertec.at

6 months ago

In reply to: Tom Lane (#5)

Re: Fix PQport to never return NULL if the connection is valid

On Wed, 2025-07-16 at 15:36 -0400, Tom Lane wrote:

Laurenz Albe <laurenz.albe@cybertec.at> writes:

On Thu, 2025-05-08 at 22:01 +0200, Daniele Varrazzo wrote:

Please find attached a new patch that doesn't change the behaviour and
just makes sure to not return NULL in case no info is available in
'conn->connhost'.

I think that it is important to fix that bug and to backpatch it.
Without the patch, I can produce the following crash:

$ psql port=
psql (18beta1)
Type "help" for help.

test=> \conninfo
Segmentation fault (core dumped)

This crash is actually new in v18

That is a relief.

Having said that, I agree with back-patching Daniele's fix

However ... I wonder if we ought to take the opportunity to fix
\conninfo to ensure that it prints something useful rather than
just an empty string? The upgrade to make it print in tabular
format is what broke this, but it also seems like an excuse to
improve the functionality as well as the cosmetics.

However, fixing this in \conninfo seems a bit tedious: we'd have
to grovel through the output of PQconndefaults. (I don't think
it's okay for psql to assume that it can rely on having been
built with the same value of DEF_PGPORT_STR that the libpq it's
linked to is using.) So it seems attractive to stick with the
original thought of making PQport substitute the correct value
for an empty string. PQport *does* know the correct value of
DEF_PGPORT_STR, so it'd be pretty nearly a one-liner to make
this happen there.

I'd only propose doing this in v18/HEAD, and sticking to the
v2 patch in the stable branches.

That makes a lot of sense.

As long as the information is factually correct, it is a good
idea to have \conninfo return more complete data.

Yours,
Laurenz Albe

Tom Lane

tgl@sss.pgh.pa.us

6 months ago

In reply to: Laurenz Albe (#6)

Re: Fix PQport to never return NULL if the connection is valid

Laurenz Albe <laurenz.albe@cybertec.at> writes:

On Wed, 2025-07-16 at 15:36 -0400, Tom Lane wrote:

... So it seems attractive to stick with the
original thought of making PQport substitute the correct value
for an empty string. PQport *does* know the correct value of
DEF_PGPORT_STR, so it'd be pretty nearly a one-liner to make
this happen there.

I'd only propose doing this in v18/HEAD, and sticking to the
v2 patch in the stable branches.

That makes a lot of sense.

Cool, done that way.

I ended up using Daniele's patches verbatim -- the first one in
v18/HEAD and the second in older branches. There was some discussion
of whether v18/HEAD should return an empty string if the connection is
bad, but I don't think that'd be helpful; the other inquiry functions
don't act that way. (But I left in the documentation text claiming
that we might do so, since perhaps such behavior would become
necessary in future.)

Also I didn't bother to remove the test for whether the port string
is empty. As you noted, it's not really necessary in the older
branches, but it is needed in v18/HEAD and it seemed better to
keep the code looking as similar as possible.

I dug around and could not find any indication of similar bugs
elsewhere in libpq. I did find that the documentation of the
port field was a lie:

char *port; /* port number (always provided) */

so I fixed that. This might be archaeological evidence of how
the bug came to be: apparently the original intent was for this
field to always be accurate, and when that changed, PQport
did not get the memo.

regards, tom lane

Laurenz Albe

laurenz.albe@cybertec.at

6 months ago

In reply to: Tom Lane (#7)

Re: Fix PQport to never return NULL if the connection is valid

On Thu, 2025-07-17 at 12:58 -0400, Tom Lane wrote:

Cool, done that way.

Thanks!

Yours,
Laurenz Albe