Обсуждение: BUG #16330: psql accesses null pointer in connect.c:do_connect

The following bug has been logged on the website:

Bug reference:      16330
Logged by:          Hugh Wang
Email address:      hghwng@gmail.com
PostgreSQL version: 12.2
Operating system:   Arch Linux
Description:

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

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

SOURCE:
https://github.com/postgres/postgres/blob/master/src/bin/psql/command.c#L3016

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