Re: Broken SSL tests in master

On Fri, Nov 25, 2016 at 3:33 PM, Andreas Karlsson <andreas(at)proxel(dot)se> wrote:
> On 11/25/2016 07:19 AM, Tsunakawa, Takayuki wrote:
>>
>> Specifying multiple hosts is a new feature to be introduced in v10, so
>> that's here:
>>
>> https://www.postgresql.org/docs/devel/static/libpq-connect.html
>
>
> Thanks, I had missed that patch. If we add support for multiple hosts I
> think we should also allow for specifying the corresponding multiple
> hostaddrs.
>
> Another thought about this code: should we not check if it is a unix socket
> first before splitting the host? While I doubt that it is common to have a
> unix socket in a directory with comma in the path it is a bit annoying that
> we no longer support this.

I had a look at the proposed patch as well as the multi-host
infrastructure that Robert has introduced, and as far as I can see the
contract of PQHost() is broken because of this code in
connectOptions2:
if (conn->pghostaddr != NULL && conn->pghostaddr[0] != '\0')
{
conn->connhost[0].host = strdup(conn->pghostaddr);
if (conn->connhost[0].host == NULL)
goto oom_error;
conn->connhost[0].type = CHT_HOST_ADDRESS;
}
This fills in the array of hosts arbitrarily a host IP. And this
breaks when combined with this code in PQhost():
if (!conn)
return NULL;
if (conn->connhost != NULL)
return conn->connhost[conn->whichhost].host;
else if (conn->pghost != NULL && conn->pghost[0] != '\0')
return conn->pghost;
So this makes PQhost return an address number even if a name is
available, which is not correct per what PQhost should do. If connhost
has multiple entries, it is definitely right to return the one
whichhost is pointing to and not fallback to pghost which may be a
list of names separated by commas. But if pghostaddr is defined *and*
there is a name available, we had better return the name that
whichhost is pointing to. The most correct fix in my opinion asdasd

- conn->connhost[0].host = strdup(conn->pghostaddr);
- if (conn->connhost[0].host == NULL)
+ if (conn->pghost != NULL && conn->pghost[0] != '\0')
+ {
+ char *e = conn->pghost;
+
+ /*
+ * Search for the end of the first hostname; a comma or
+ * end-of-string acts as a terminator.
+ */
+ while (*e != '\0' && *e != ',')
+ ++e;
+
+ /* Copy the hostname whose bounds we just identified. */
+ conn->connhost[0].host =
+ (char *) malloc(sizeof(char) * (e - conn->pghost + 1));
+ if (conn->connhost[0].host == NULL)
+ goto oom_error;
+ memcpy(conn->connhost[0].host, conn->pghost, e - conn->pghost);
+ conn->connhost[0].host[e - conn->pghost] = '\0';
+ }
+ else
+ {
+ conn->connhost[0].host = strdup(conn->pghostaddr);
+ if (conn->connhost[0].host == NULL)
+ goto oom_error;
+ }
+
+ conn->connhost[0].hostaddr = strdup(conn->pghostaddr);
+ if (conn->connhost[0].hostaddr == NULL)
goto oom_error;
conn->connhost[0].type = CHT_HOST_ADDRESS

This breaks the case where users specify both host and hostaddr in a
connection string as this would force users to do an extra lookup at
which IP a host name is mapping, which is not good.

As we know that if hostaddr is specified, the number of entries in the
connhost array will be one, wouldn't the most correct fix, based on
the current implementation of multi-hosts and its limitations, be to
tweak PQhost() so as the first element in pghost is returned back to
the caller instead of an entry of type CHT_HOST_ADDRESS? And if pghost
is NULL, we should return PG_DEFAULT_HOST which is the same way of
doing things as before multi-host was implemented. We definitely need
to make PQhost() not return any host IPs if host names are available,
but not forget the case where a host can be specified as an IP itself.

Robert, others, what do you think?
--
Michael