BUG #16330: psql accesses null pointer in connect.c:do_connect

On Mon, Mar 30, 2020 at 02:20:48AM +0000, PG Bug reporting form wrote:

If the connection to postmaster is closed, then trying to re-connect to
another one leads to SIGSEGV.

REPRODUCE:
$ psql
-> \conninfo
You are connected to database "hugh" as user "hugh" via socket in
"/run/postgresql" at port "5432".
*shut down server with commands like "systemctl stop postgresql"*
-> \conninfo
You are currently not connected to a database.
-> \c a b c d
[1] 984978 segmentation fault (core dumped) psql

Indeed. Reproduced the problem here on HEAD and REL_12_STABLE, though
you need to execute a command after stopping the server to let psql
know that you are not connected to a database.

ANALYSIS:
PQhost(o_conn) returns NULL, and strcmp(host, NULL) raises SIGSEGV.

Yes, the problem comes from this commit (added Alvaro in CC):
commit: 6e5f8d489acccdc50a35a1b7db8e72b5ad579253
author: Alvaro Herrera <alvherre@alvh.no-ip.org>
date: Mon, 19 Nov 2018 14:34:12 -0300
psql: Show IP address in \conninfo

And more particularly this bit that ignores the possibility of
PQhost() being NULL:
-   if (!user && reuse_previous)
-       user = PQuser(o_conn);
-   if (!host && reuse_previous)
-       host = PQhost(o_conn);
-   if (!port && reuse_previous)
-       port = PQport(o_conn);
+   if (reuse_previous)
+   {
+       if (!user)
+           user = PQuser(o_conn);
+       if (host && strcmp(host, PQhost(o_conn)) == 0)
+       {
+           /*
+            * if we are targetting the same host, reuse its hostaddr for
+            * consistency
+            */
+           hostaddr = PQhostaddr(o_conn);

A fix like the attached should be sufficient as if we know that
PQhost() is NULL for the old connection we cannot use the old
hostaddr. Alvaro, what do you think?
--
Michael

Michael Paquier <michael@paquier.xyz> writes:

On Mon, Mar 30, 2020 at 02:20:48AM +0000, PG Bug reporting form wrote:

If the connection to postmaster is closed, then trying to re-connect to
another one leads to SIGSEGV.

A fix like the attached should be sufficient as if we know that
PQhost() is NULL for the old connection we cannot use the old
hostaddr. Alvaro, what do you think?

It looks to me like there's a similar hazard a bit further down
(line 3029):

appendConnStrVal(&connstr, PQdb(o_conn));

I wonder if we should force reuse_previous to false if there's
no o_conn, rather than fixing this stuff piecemeal.

regards, tom lane

On 2020-Mar-30, Tom Lane wrote:

Michael Paquier <michael@paquier.xyz> writes:

On Mon, Mar 30, 2020 at 02:20:48AM +0000, PG Bug reporting form wrote:

If the connection to postmaster is closed, then trying to re-connect to
another one leads to SIGSEGV.

A fix like the attached should be sufficient as if we know that
PQhost() is NULL for the old connection we cannot use the old
hostaddr. Alvaro, what do you think?

It looks to me like there's a similar hazard a bit further down
(line 3029):

appendConnStrVal(&connstr, PQdb(o_conn));

I wonder if we should force reuse_previous to false if there's
no o_conn, rather than fixing this stuff piecemeal.

That was my impression too.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Mon, Mar 30, 2020 at 12:48:51PM -0300, Alvaro Herrera wrote:

On 2020-Mar-30, Tom Lane wrote:

It looks to me like there's a similar hazard a bit further down
(line 3029):

appendConnStrVal(&connstr, PQdb(o_conn));

I wonder if we should force reuse_previous to false if there's
no o_conn, rather than fixing this stuff piecemeal.

That was my impression too.

Good point. What do you think about the attached then?
--
Michael

Michael Paquier <michael@paquier.xyz> writes:

On Mon, Mar 30, 2020 at 12:48:51PM -0300, Alvaro Herrera wrote:

On 2020-Mar-30, Tom Lane wrote:

I wonder if we should force reuse_previous to false if there's
no o_conn, rather than fixing this stuff piecemeal.

That was my impression too.

Good point. What do you think about the attached then?

WFM.

regards, tom lane

On 2020-Mar-31, Michael Paquier wrote:

On Mon, Mar 30, 2020 at 12:48:51PM -0300, Alvaro Herrera wrote:

On 2020-Mar-30, Tom Lane wrote:

It looks to me like there's a similar hazard a bit further down
(line 3029):

appendConnStrVal(&connstr, PQdb(o_conn));

I wonder if we should force reuse_previous to false if there's
no o_conn, rather than fixing this stuff piecemeal.

That was my impression too.

Good point. What do you think about the attached then?

Looks better :-)

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Mar 31, 2020 at 10:52:59AM -0300, Alvaro Herrera wrote:

Looks better :-)

Thanks to both of you for the suggestions and the reviews. Fix this
way then.
--
Michael