Date: Sat, 21 Jan 2023 08:16:42 -0800
From: Noah Misch <noah@leadboat.com>
To: Andres Freund <andres@anarazel.de>
Cc: pgsql-hackers@postgresql.org
Subject: Re: libpqrcv_connect() leaks PGconn
Message-ID: <20230121161642.GD1356297@rfd.leadboat.com>
References: <20230121011237.q52apbvlarfv6jm6@awork3.anarazel.de>
 <20230121025037.mildbmgylv4ybp4a@awork3.anarazel.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230121025037.mildbmgylv4ybp4a@awork3.anarazel.de>
User-Agent: Mutt/1.5.24 (2015-08-30)
Archived-At: <https://www.postgresql.org/message-id/20230121161642.GD1356297%40rfd.leadboat.com>
Precedence: bulk

On Fri, Jan 20, 2023 at 06:50:37PM -0800, Andres Freund wrote:
> On 2023-01-20 17:12:37 -0800, Andres Freund wrote:
> > We have code like this in libpqrcv_connect():
> > 
> > 	conn = palloc0(sizeof(WalReceiverConn));
> > 	conn->streamConn = PQconnectStartParams(keys, vals,
> > 											 /* expand_dbname = */ true);
> > 	if (PQstatus(conn->streamConn) == CONNECTION_BAD)
> > 	{
> > 		*err = pchomp(PQerrorMessage(conn->streamConn));
> > 		return NULL;
> > 	}
> > 
> >         [try to establish connection]
> > 
> > 	if (PQstatus(conn->streamConn) != CONNECTION_OK)
> > 	{
> > 		*err = pchomp(PQerrorMessage(conn->streamConn));
> > 		return NULL;
> > 	}
> > 
> > 
> > Am I missing something, or are we leaking the libpq connection in case of
> > errors?
> > 
> > It doesn't matter really for walreceiver, since it will exit anyway, but we
> > also use libpqwalreceiver for logical replication, where it might?
> > 
> > 
> > Seems pretty clear that we should do a PQfinish() before returning NULL? I
> > lean towards thinking that this isn't worth backpatching given the current
> > uses of libpq, but I could easily be convinced otherwise.
> > 
> 
> It's bit worse than I earlier thought: We use walrv_connect() during CREATE
> SUBSCRIPTION. One can easily exhaust file descriptors right now.  So I think
> we need to fix this.
> 
> I also noticed the following in libpqrcv_connect, added in 11da97024abb:

> The attached patch fixes both issues.

Looks good.  I'm not worried about a superuser hosing their own session via
CREATE SUBSCRIPTION failures in a loop.  At the same time, this fix is plenty
safe to back-patch.

> I seems we don't have any tests for creating a subscription that fails during
> connection establishment? That doesn't seem optimal - I guess there may have
> been concern around portability of the error messages?

Perhaps.  We have various (non-subscription) tests using "\set VERBOSITY
sqlstate" for that problem.  If even the sqlstate varies, a DO block is the
next level of error swallowing.

> I think we can control
> for that in a tap test, by failing to connect due to a non-existant database,
> then the error is under our control. Whereas e.g. an invalid hostname would
> contain an error from gai_strerror().

That sounds fine.