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

Hi,

On 2023-01-21 08:16:42 -0800, Noah Misch wrote:
> 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():
> > 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.

Yea, I'm not worried about it from a security perspective and more from a
usability perspective (but even there not terribly). File descriptors that
leaked, particularly when not reserved (AcquireExternalFD() etc), can lead to
weird problems down the line. And I think it's not that rare to need a few
attempts at getting the connection string, permissions, etc right.

Thanks for looking at the fix!


> > 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.

That's a good trick I need to remember. And the errcode for an invalid
connection string luckily differs from the one for a not working one.


I think found an even easier way - port=-1 is rejected during PQconnectPoll()
and will never even open a socket. That'd make it reasonable for the test to
happen in subscription.sql, instead of a tap test, I think (faster, easier to
maintain). It may be that we'll one day move that error into the
PQconninfoParse() phase, but I don't think we need to worry about it now.

Any reason not to go for that?

If not, I'll add a test for an invalid conninfo and a non-working connection
string to subscription.sql.

Greetings,

Andres Freund