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

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:
>
> > On Sat, Mar 24, 2018 at 01:49:28AM +1100, Haribabu Kommi wrote:
> > > Here I attached the updated patch that returns either the connected
> > > host/hostaddr
> > > or NULL in case if the connection is not established.
> > >
> > > I removed the returning default host details, because the default host
> > > details are also available with the connhost member itself.
> >
> >
> Thanks for the review.
>
>
> > As shaped, PQhost generates complains from gcc with -Wreturn-type. So I
> > would suggest to return NULL for the default code path. As far as I can
> > see from the code, PGconn->connhost cannot be NULL and it should have
> > at least one "host" or "hostaddr" defined, so I think that we could
> > consider adding an assertion about that and comment out this cannot
> > normally be reached.
> >
>
> Ok. Added an assert with an explanation comment.

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.

https://www.postgresql.org/message-id/19297.1493660213%40sss.pgh.pa.us

> > If the connection is bad, then whichhost points to 0, which would cause
> > PQhost to return the first host or hostaddr value. I think that we
> > should document properly to not trust the value of PQhost if the status
> > is CONNECTION_BAD, or to return NULL if the connection is bad as this
> > would have no real value for multiple hosts. I am a bit afraid of
> > potential breakages if we do that, so the first method may make the most
> > sense.
>
>
> The existing behavior is currently returning wrong value when the connection
> status is CONNECTION_BAD. As we are changing the behavior of the function,
> it may be better to handle the CONNECTION_BAD scenario also instead of
> providing note in the manual?

There's no reason to behave so only for the functions. PQdb() and
PQuser() returns names that is not actually connected. 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.

My opninon is to add a description that is saying like "these
functions return unreliable values for a failed connection".

> > The same things apply to PQport as multiple ports can be
> > defined. Thoughts?
> >
>
> Yes, I changed PQport also to return the connected port or NULL,
> Removed the returning of all the ports specified in the connection string.
>
>
> > I have quickly looked at the callers of PQhost in the core code and
> > those seem safe. Something to keep in mind.
> >
> > More details in the documentation would be nice. Let's detail the
> > following:
> > - PQhost returns NULL if the connection is not established yet.
> > - PQhost may return an incorrect value when with CONNECTION_BAD as
> > status.
> > - If both hostaddr and host are precised in the conneciton string, then
> > host has the priority.
> > - If only hostaddr is precised, then this is the value returned.
> >
>
> Docs are updated with the new behavior of the functions.
>
> 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.

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center