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
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index f91f0f2efe..9af830321c 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -1687,7 +1687,7 @@ useKeepalives(PGconn *conn)
/*
* Parse and try to interpret "value" as an integer value, and if successful,
* store it in *result, complaining if there is any trailing garbage or an
- * overflow.
+ * overflow. This allows any number of leading and trailing whitespaces.
*/
static bool
parse_int_param(const char *value, int *result, PGconn *conn,
@@ -1698,14 +1698,31 @@ parse_int_param(const char *value, int *result, PGconn *conn,
*result = 0;
+ /* strtol(3) skips leading whitespaces */
errno = 0;
numval = strtol(value, &end, 10);
- if (errno == 0 && *end == '\0' && numval == (int) numval)
- {
- *result = numval;
- return true;
- }
+ /*
+ * If no progress was done during the parsing or an error happened, fail.
+ * This tests properly for overflows of the result.
+ */
+ if (value == end || errno != 0 || numval != (int) numval)
+ goto error;
+
+ /*
+ * Skip any trailing whitespace; if anything but whitespace remains before
+ * the terminating character, fail
+ */
+ while (*end && *end != '\0' && isspace((unsigned char) *end))
+ end++;
+
+ if (*end && *end != '\0')
+ goto error;
+
+ *result = numval;
+ return true;
+
+error:
appendPQExpBuffer(&conn->errorMessage,
libpq_gettext("invalid integer value \"%s\" for connection option \"%s\"\n"),
value, context);
@@ -2008,7 +2025,11 @@ connectDBComplete(PGconn *conn)
{
if (!parse_int_param(conn->connect_timeout, &timeout, conn,
"connect_timeout"))
+ {
+ /* mark the connection as bad to report the parsing failure */
+ conn->status = CONNECTION_BAD;
return 0;
+ }
if (timeout > 0)
{
signature.asc
Description: PGP signature
