Horton

Thread

Commits

  1. Fix PQport to never return NULL unless the connection is NULL.

  1. Fix PQport to never return NULL if the connection is valid

    Daniele Varrazzo <daniele.varrazzo@gmail.com> — 2025-05-08T16:58:25Z

    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

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

    Daniele Varrazzo <daniele.varrazzo@gmail.com> — 2025-05-08T17:38:36Z

    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

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

    Daniele Varrazzo <daniele.varrazzo@gmail.com> — 2025-05-08T20:01:18Z

    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

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

    Laurenz Albe <laurenz.albe@cybertec.at> — 2025-07-16T15:32:59Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-07-16T19:36:35Z

    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

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

    Laurenz Albe <laurenz.albe@cybertec.at> — 2025-07-17T06:16:43Z

    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

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

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-07-17T16:58:36Z

    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

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

    Laurenz Albe <laurenz.albe@cybertec.at> — 2025-07-17T18:53:13Z

    On Thu, 2025-07-17 at 12:58 -0400, Tom Lane wrote:
> Cool, done that way.

Thanks!

Yours,
Laurenz Albe