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