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

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.

> 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?

> 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.

Regards,
Hari Babu
Fujitsu Australia