Re: libpq stricter integer parsing

[Michael Paquier]

On Sun, Sep 09, 2018 at 09:01:15AM +0200, Fabien COELHO wrote:
> Hmmm. This is what the sentence following the above tries to explain
> implicitely:
> 
>   Versions of <application>libpq</application> before
>   <product>PostgreSQL 12</product> accepted trailing garbage or overflows.
> 
> Maybe I can rephrase it in one sentence, eg:
> 
> "From PostgreSQL 12, integer values for keywords ... are parsed strictly,
> i.e. trailing garbage and errors on overflows are not accepted
> anymore."

Okay, I am including that formulation.  I have not put yet much thoughts
into locating this in another place of the docs.  Or perhaps we could
just discard it from the final commit.

I have been reviewing your patch a bit more, and I have found an issue:
overflows are not correctly detected.  For example by specifying
something like port=5000000000 I would have expected an error but the
parsing code failed to detect that.  Values like -1 need to be accepted
though are equivalent to an unknown state when it comes to keepalive_*.

In conclusion, I finish with the simplified patch attached.  Fabien, is
that acceptable to you?
--
Michael

diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 5e7931ba90..bc7836d103 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -1591,6 +1591,15 @@ postgresql://%2Fvar%2Flib%2Fpostgresql/dbname
     </varlistentry>
     </variablelist>
    </para>
+
+   <para>
+    Integer values expected for keywords <literal>port</literal>,
+    <literal>connect_timeout</literal>, <literal>keepalives_idle</literal>,
+    <literal>keepalives_interval</literal> and
+    <literal>keepalives_timeout</literal> are parsed more strictly as
+    of <product>PostgreSQL<product> 12, i.e. values including trailing garbage
+    or overflowing are rejected.
+   </para>
   </sect2>
  </sect1>
 
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 42cdb971a3..c7a4814e8e 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -1587,6 +1587,34 @@ useKeepalives(PGconn *conn)
 	return val != 0 ? 1 : 0;
 }
 
+/*
+ * 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.
+ */
+static bool
+parse_int_param(const char *value, int *result, PGconn *conn,
+				const char *context)
+{
+	char   *end;
+	long	numval;
+
+	*result = 0;
+
+	errno = 0;
+	numval = strtol(value, &end, 10);
+	if (errno == 0 && *end == '\0' && numval == (int) numval)
+	{
+		*result = numval;
+		return true;
+	}
+
+	appendPQExpBuffer(&conn->errorMessage,
+					  libpq_gettext("invalid value for keyword \"%s\"\n"),
+					  context);
+	return false;
+}
+
 #ifndef WIN32
 /*
  * Set the keepalive idle timer.
@@ -1599,7 +1627,8 @@ setKeepalivesIdle(PGconn *conn)
 	if (conn->keepalives_idle == NULL)
 		return 1;
 
-	idle = atoi(conn->keepalives_idle);
+	if (!parse_int_param(conn->keepalives_idle, &idle, conn, "keepalives_idle"))
+		return 0;
 	if (idle < 0)
 		idle = 0;
 
@@ -1631,7 +1660,8 @@ setKeepalivesInterval(PGconn *conn)
 	if (conn->keepalives_interval == NULL)
 		return 1;
 
-	interval = atoi(conn->keepalives_interval);
+	if (!parse_int_param(conn->keepalives_interval, &interval, conn, "keepalives_interval"))
+		return 0;
 	if (interval < 0)
 		interval = 0;
 
@@ -1664,7 +1694,8 @@ setKeepalivesCount(PGconn *conn)
 	if (conn->keepalives_count == NULL)
 		return 1;
 
-	count = atoi(conn->keepalives_count);
+	if (!parse_int_param(conn->keepalives_count, &count, conn, "keepalives_count"))
+		return 0;
 	if (count < 0)
 		count = 0;
 
@@ -1698,13 +1729,15 @@ setKeepalivesWin32(PGconn *conn)
 	int			idle = 0;
 	int			interval = 0;
 
-	if (conn->keepalives_idle)
-		idle = atoi(conn->keepalives_idle);
+	if (conn->keepalives_idle &&
+		!parse_int_param(conn->keepalives_idle, &idle, conn, "keepalives_idle"))
+		return 0;
 	if (idle <= 0)
 		idle = 2 * 60 * 60;		/* 2 hours = default */
 
-	if (conn->keepalives_interval)
-		interval = atoi(conn->keepalives_interval);
+	if (conn->keepalives_interval &&
+		!parse_int_param(conn->keepalives_interval, &interval, conn, "keepalives_interval"))
+		return 0;
 	if (interval <= 0)
 		interval = 1;			/* 1 second = default */
 
@@ -1831,7 +1864,10 @@ connectDBComplete(PGconn *conn)
 	 */
 	if (conn->connect_timeout != NULL)
 	{
-		timeout = atoi(conn->connect_timeout);
+		if (!parse_int_param(conn->connect_timeout, &timeout, conn,
+							 "connect_timeout"))
+			return 0;
+
 		if (timeout > 0)
 		{
 			/*
@@ -1842,6 +1878,8 @@ connectDBComplete(PGconn *conn)
 			if (timeout < 2)
 				timeout = 2;
 		}
+		else /* negative means 0 */
+			timeout = 0;
 	}
 
 	for (;;)
@@ -2108,7 +2146,9 @@ keep_going:						/* We will come back to here until there is
 			thisport = DEF_PGPORT;
 		else
 		{
-			thisport = atoi(ch->port);
+			if (!parse_int_param(ch->port, &thisport, conn, "port"))
+				goto error_return;
+
 			if (thisport < 1 || thisport > 65535)
 			{
 				appendPQExpBuffer(&conn->errorMessage,

signature.asc
Description: PGP signature