Thread: libpq: Fix wrong connection status on invalid "connect_timeout"
Greetings, libpq since PostgreSQL-12 has stricter checks for integer values in connection parameters. They were introduced by commit https://github.com/postgres/postgres/commit/e7a2217978d9cbb2149bfcb4ef1e45716cfcbefb . However in case of "connect_timeout" such an invalid integer value leads to a connection status other than CONNECTION_OK or CONNECTION_BAD. The wrong parameter is therefore not properly reported to user space. This patch fixes this by explicit setting CONNECTION_BAD. The issue was raised on ruby-pg: https://github.com/ged/ruby-pg/issues/302 It originally came up at Heroku: https://github.com/heroku/stack-images/issues/147 -- Kind Regards, Lars Kanis
Attachment
I verified that all other integer parameters properly set CONNECTION_BAD in case of invalid values. These are: * port * keepalives_idle * keepalives_interval * keepalives_count * tcp_user_timeout That's why I changed connectDBComplete() only, instead of setting the status directly in parse_int_param(). -- Kind Regards, Lars Kanis Am 17.10.19 um 20:04 schrieb Lars Kanis: > Greetings, > > libpq since PostgreSQL-12 has stricter checks for integer values in > connection parameters. They were introduced by commit > https://github.com/postgres/postgres/commit/e7a2217978d9cbb2149bfcb4ef1e45716cfcbefb > . > > However in case of "connect_timeout" such an invalid integer value leads > to a connection status other than CONNECTION_OK or CONNECTION_BAD. The > wrong parameter is therefore not properly reported to user space. This > patch fixes this by explicit setting CONNECTION_BAD. > > The issue was raised on ruby-pg: https://github.com/ged/ruby-pg/issues/302 > > It originally came up at Heroku: > https://github.com/heroku/stack-images/issues/147 > -- -- Kind Regards, Lars Kanis
On Thu, Oct 17, 2019 at 10:10:17PM +0200, Lars Kanis wrote: > That's why I changed connectDBComplete() only, instead of setting the > status directly in parse_int_param(). Yes, you shouldn't do that as the keepalive parameters and tcp_user_timeout have some specific handling when it comes to defaults depending on the platform and we have some retry logic when specifying multiple hosts. Now, there is actually more to it than it looks at first glance. Your patch is pointing out at a failure within the regression tests of the ECPG driver, as any parameters part of a connection string may have trailing spaces which are considered as incorrect by the patch, causing the connection to fail. In short, on HEAD this succeeds but would fail with your patch: $ psql 'postgresql:///postgres?host=/tmp&connect_timeout=14 &port=5432' psql: error: could not connect to server: invalid integer value "14 " for connection option "connect_timeout" Parameter names are more restrictive, as URLs don't allow leading or trailing spaces for them. On HEAD, we allow leading spaces for integer parameters as the parsing uses strtol(3), but not for the trailing spaces, which is a bit crazy and I think that we had better not break that if the parameter value correctly defines a proper integer. So attached is a patch to skip trailing whitespaces as well, which also fixes the issue with ECPG. I have refactored the parsing logic a bit while on it. The comment at the top of parse_int_param() needs to be reworked a bit more. We could add some TAP tests for that, but I don't see a good area to check after connection parameters. We have tests for multi-host strings in 001_stream_rep.pl but that already feels misplaced as those tests are for recovery. Perhaps we could add directly regression tests for libpq. I'll start a new thread about that once we are done here, the topic is larger. (Note to self: Ed Morley needs to be credited for the report as well.) -- Michael
Attachment
Am 18.10.19 um 05:06 schrieb Michael Paquier: > So attached is a patch to skip trailing whitespaces as well, > which also fixes the issue with ECPG. I have refactored the parsing > logic a bit while on it. The comment at the top of parse_int_param() > needs to be reworked a bit more. I tested this and it looks good to me. Maybe you could omit some redundant 'end' checks, as in the attached patch. Or was your intention to verify non-NULL 'end'? > Perhaps we could add directly regression > tests for libpq. I'll start a new thread about that once we are done > here, the topic is larger. We have around 650 tests on ruby-pg to ensure everything runs as expected and I always wondered how the API of libpq is being verified. -- Kind Regards, Lars Kanis
Attachment
Am 18.10.19 um 05:06 schrieb Michael Paquier: > So attached is a patch to skip trailing whitespaces as well, > which also fixes the issue with ECPG. I have refactored the parsing > logic a bit while on it. The comment at the top of parse_int_param() > needs to be reworked a bit more. I tested this and it looks good to me. Maybe you could omit some redundant 'end' checks, as in the attached patch. Or was your intention to verify non-NULL 'end'? > Perhaps we could add directly regression > tests for libpq. I'll start a new thread about that once we are done > here, the topic is larger. We have around 650 tests on ruby-pg to ensure everything runs as expected and I always wondered how the API of libpq is being verified. -- Kind Regards, Lars Kanis
Attachment
Am 18.10.19 um 05:06 schrieb Michael Paquier: > So attached is a patch to skip trailing whitespaces as well, > which also fixes the issue with ECPG. I have refactored the parsing > logic a bit while on it. The comment at the top of parse_int_param() > needs to be reworked a bit more. I tested this and it looks good to me. Maybe you could omit some redundant 'end' checks, as in the attached patch. Or was your intention to verify non-NULL 'end'? > Perhaps we could add directly regression > tests for libpq. I'll start a new thread about that once we are done > here, the topic is larger. We have around 650 tests on ruby-pg to ensure everything runs as expected and I always wondered how the API of libpq is being verified. -- Kind Regards, Lars Kanis
Attachment
On Fri, Oct 18, 2019 at 02:01:23PM +0200, Lars Kanis wrote: > Am 18.10.19 um 05:06 schrieb Michael Paquier: >> So attached is a patch to skip trailing whitespaces as well, >> which also fixes the issue with ECPG. I have refactored the parsing >> logic a bit while on it. The comment at the top of parse_int_param() >> needs to be reworked a bit more. > > I tested this and it looks good to me. Maybe you could omit some > redundant 'end' checks, as in the attached patch. Or was your intention > to verify non-NULL 'end'? Yes. Here are the connection patterns I have tested. These now pass: 'postgresql:///postgres?host=/tmp&port=5432 &user=postgres' 'postgresql:///postgres?host=/tmp&port= 5432&user=postgres' And these fail (overflow on third one): 'postgresql:///postgres?host=/tmp&port=5432 s &user=postgres' 'postgresql:///postgres?host=/tmp&port= s 5432&user=postgres' 'postgresql:///postgres?host=/tmp&port= 5000000000&user=postgres' Before the patch any trailing characters caused a failures even if there were just whitespaces as trailing characters (first case listed). >> Perhaps we could add directly regression >> tests for libpq. I'll start a new thread about that once we are done >> here, the topic is larger. > > We have around 650 tests on ruby-pg to ensure everything runs as > expected and I always wondered how the API of libpq is being verified. For advanced test scenarios like connection handling, we use perl's TAP tests. The situation regarding libpq-related testing is a bit messy though. We have some tests in src/test/recovery/ for a couple of things, and we should have more things to stress anything related to the protocol (say message injection, etc.). I'll try to start a new thread about that with a patch adding some basics for discussion. I have applied the parsing fix and your fix as two separate commits as these are at the end two separate bugs, then back-patched down to v12. Ed has been credited for the report, and I have marked the author as you, Lars. Thanks! -- Michael