Обсуждение: 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

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

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

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

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

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