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

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.

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

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

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

Regards,
Hari Babu
Fujitsu Australia