Re: PQHost() undefined behavior if connecting string contains both host and hostaddr types

Hello.

At Mon, 26 Mar 2018 17:49:22 +1100, Haribabu Kommi <kommi(dot)haribabu(at)gmail(dot)com> wrote in <CAJrrPGdYQ92R1hNArCByu+gN_SGsFMjGvbErjH0N9W8Ry24CxA(at)mail(dot)gmail(dot)com>
> On Mon, Mar 26, 2018 at 4:17 PM, Michael Paquier <michael(at)paquier(dot)xyz>
> wrote:
>
> > On Mon, Mar 26, 2018 at 11:28:41AM +0900, Kyotaro HORIGUCHI wrote:
> > > At Sun, 25 Mar 2018 22:27:09 +1100, Haribabu Kommi <
> > kommi(dot)haribabu(at)gmail(dot)com> wrote in <CAJrrPGeaQxWsU3RZ0yxGa=WY+
> > wnA+nvE_YPuKzBDaqRMUt9SyA(at)mail(dot)gmail(dot)com>
> > >> On Sun, Mar 25, 2018 at 12:56 AM, Michael Paquier <michael(at)paquier(dot)xyz>
> > >> wrote:
> >
>
> Thanks for the review.
>
>
> > > As the commit message of 50cb21f70, the function is intended not
> > > to return NULL in order to prevent the user functions from a
> > > crash in corner cases.
> >
> > The commit number is not correct here. You mean 40cb21f.
> >
> > > https://www.postgresql.org/message-id/19297.1493660213%40sss.pgh.pa.us
> >
> > I quite like the idea of using an empty string value in those cases.
> > This could prevent crashes at leat for applications not doing NULL-ness
> > checks.
> >
>
> I also agree to return an empty string. I did this only for the cases where
> the conn is
> not NULL but the status is not proper or the connhost is NULL.
>
>
> > > Since the
> > > purpose of PGhost is not strictly defined, especially on
> > > connection failure, returning the given host list can be another
> > > candidate (and the same can be said for returning ""). I think
> > > users shouldn't (and I believe no one does) rely on the values
> > > from the functions when CONNECTION_BAD, anyway.
> >
> > Yeah, this should really be documented and also should refer to the fact
> > that this happens when specifying multiple hosts.
> >
>
> Added.

+ The <function>PQhost</function> function returns NULL when the
+ connection is not established, or returns an empty string when status
+ of the connection is not <literal>CONNECTION_OK</literal>.

This may be wrong. NULL is only for the case conn == NULL and ""
for other connection failure. I'm not sure how to express the OOM
case in the documentation but we could reuturn "" for the
conn==NULL case if we don't want to distinguish the state in
PQhost and its family.

> > > My opinion is to add a description that is saying like "these
> > > functions return unreliable values for a failed connection".
> >
> > At the same time I don't think that this is sufficient either, because
> > for multiple hosts, PQhost() just returns the first one, which makes
> > absolutely no sense because the value is wrong. So I think that using a
> > third, separate value has some advantages:
> > - If NULL, this just means that the initialization did not happen.
> > - If using an empty string, then the value cannot be evaluated.
> > - If this returns a host or hostaddr (if host has not been specified),
> > then that's the host which is actually used for the connection.
> > Having those three states has value for applications in my opinion.
> >
> > The same can apply to PQport, or any other functions which for whatever
> > reason add support for multiple values like host, hostaddr or port.
> >
>
> I hope that I updated the documentation properly to explain all the above
> cases.

I'm not sure but I'm afraid that some authentication methods
requires that PQhost() returns a host name for the states other
than CONNECTION_OK, perhaps CONNECTION_MADE, AWAITING_RESPONSE
and so, which happens after a connection is established. Even
without considering that, we can return a sane value after raw
connection (not a PGconn) is established.

> > >> Updated patch attached with behavior of returning NULL for connections
> > of
> > >> CONNECTION_BAD status.
> > >
> > > The patch does Assert() in PQhost. I suppose that Assert() in
> > > client library is usable only when no more (library's) operation
> > > cannot continue. It would be better to return a fallback value in
> > > this criteria.
> >
> > The patch has to return a value as well. A shaped the patch causes
> > again compilation warnings because not all code paths have a return
> > value. So my previous remark has not been fixed. Hari, what do you use
> > as compiler, my gcc blows a warning and reading the patch that's
> > obviously incorrect.
> >
>
> In my assert enabled build, I didn't get any warning. Yes that patch to fix
> the
> warning is clearly wrong. I corrected in a different way of adding Assert
> checks
> for the hostaddr, because definitely host or hostaddr must be present.

As I wrote upthread, the assertion doesn't seem to be needed. I
think that a library should allow callers to decide how to handle
error cases if it is possible.

> > + PQHost returns NULL when the connection is not established
or
> > In the docs, this is wrong for two reasons:
> > - There should be a <function> markup.
> > - The name of the function is PQhost, not PGHost.
> >
>
> Corrected.
>
> Updated patch attached.

The documentation is written as the following.

- Returns the server host name of the connection.
+ Returns the server host name or host address of the active connection.
This can be a host name, an IP address, or a directory path if the

Is the replacement is required? The following line is stating the
same thing including the local-socket case.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center