Thread: 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
Attachment
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
Attachment
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