Parse more strictly integer parameters from connection strings in libpq

The following parameters have been parsed in lossy ways when specified
in a connection string processed by libpq:
- connect_timeout
- keepalives
- keepalives_count
- keepalives_idle
- keepalives_interval
- port

Overflowing values or the presence of incorrect characters were not
properly checked, leading to libpq trying to use such values and fail
with unhelpful error messages.  This commit hardens the parsing of those
parameters so as it is possible to find easily incorrect values.

Author: Fabien Coelho
Reviewed-by: Peter Eisentraut, Michael Paquier
Discussion: https://postgr.es/m/alpine.DEB.2.21.1808171206180.20841@lancre

diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 42cdb971a37e63413ec9898a8990bbd00cb13b1f..45bc067921c807aa1a9abb3895ee5e9795351324 100644 (file)
--- 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 integer value \"%s\" for keyword \"%s\"\n"),
+                     value, context);
+   return false;
+}
+
 #ifndef WIN32
 /*
  * Set the keepalive idle timer.
@@ -1599,7 +1627,9 @@ 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 +1661,9 @@ 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 +1696,9 @@ 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 +1732,17 @@ 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 +1869,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 +1883,8 @@ connectDBComplete(PGconn *conn)
            if (timeout < 2)
                timeout = 2;
        }
+       else                    /* negative means 0 */
+           timeout = 0;
    }
 
    for (;;)
@@ -2108,7 +2151,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,