libpq stricter integer parsing

Started by Fabien COELHOover 7 years ago11 messages

coelho@cri.ensmp.fr

over 7 years ago

1 attachment(s)

Follow up on a patch and discussion with Tom, currently integer parsing on
keywords in libpq is quite loose, resulting in trailing garbage being
ignored and allowing to hide bugs, eg:

sh> psql "connect_timeout=2,port=5433"

The timeout is set to 2, and the port directive is silently ignored.
However, URL parsing is stricter, eg on "port".

The attached patch checks integer syntax errors and overflows, and report
errors.

The pros is that it helps detect bugs. The cons is that some people may
not want to know about these if it works in the end.

--
Fabien.

Attachments:

libpq-strict-atoi-1.patchtext/x-diff; name=libpq-strict-atoi-1.patchDownload

diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 49de975e7f..94d114562a 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -1579,6 +1579,16 @@ 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 strictly.
+    Versions of <application>libpq</application> before
+    <product>PostgreSQL 12</product> accepted trailing garbage or overflows.
+   </para>
   </sect2>
  </sect1>
 
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 9315a27561..15280dc208 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -1586,6 +1586,35 @@ useKeepalives(PGconn *conn)
 	return val != 0 ? 1 : 0;
 }
 
+/*
+ * Parse integer, report any syntax error, and tell if all is well.
+ */
+static bool
+strict_atoi(const char *str, int *val, PGconn *conn, const char *context)
+{
+	char 	*endptr;
+
+	errno = 0;
+	*val = strtol(str, &endptr, 10);
+
+	if (errno != 0)
+	{
+		appendPQExpBuffer(&conn->errorMessage,
+						  libpq_gettext("invalid integer \"%s\" for keyword \"%s\": %s\n"),
+						  str, context, strerror(errno));
+		return false;
+	}
+	else if (*endptr != '\0')
+	{
+		appendPQExpBuffer(&conn->errorMessage,
+						  libpq_gettext("invalid integer \"%s\" for keyword \"%s\"\n"),
+						  str, context);
+		return false;
+	}
+
+	return true;
+}
+
 #ifndef WIN32
 /*
  * Set the keepalive idle timer.
@@ -1598,7 +1627,9 @@ setKeepalivesIdle(PGconn *conn)
 	if (conn->keepalives_idle == NULL)
 		return 1;
 
-	idle = atoi(conn->keepalives_idle);
+	if (!strict_atoi(conn->keepalives_idle, &idle, conn, "keepalives_idle"))
+		return 0;
+
 	if (idle < 0)
 		idle = 0;
 
@@ -1630,7 +1661,9 @@ setKeepalivesInterval(PGconn *conn)
 	if (conn->keepalives_interval == NULL)
 		return 1;
 
-	interval = atoi(conn->keepalives_interval);
+	if (!strict_atoi(conn->keepalives_interval, &interval, conn, "keepalives_interval"))
+		return 0;
+
 	if (interval < 0)
 		interval = 0;
 
@@ -1663,7 +1696,9 @@ setKeepalivesCount(PGconn *conn)
 	if (conn->keepalives_count == NULL)
 		return 1;
 
-	count = atoi(conn->keepalives_count);
+	if (!strict_atoi(conn->keepalives_count, &count, conn, "keepalives_count"))
+		return 0;
+
 	if (count < 0)
 		count = 0;
 
@@ -1697,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 &&
+		!strict_atoi(conn->keepalives_idle, &idle, conn, "keepalives_idle"))
+		return 0;
+
+	if (conn->keepalives_interval &&
+		!strict_atoi(conn->keepalives_interval, &interval, conn, "keepalives_interval"))
+		return 0;
+
 	if (idle <= 0)
 		idle = 2 * 60 * 60;		/* 2 hours = default */
 
-	if (conn->keepalives_interval)
-		interval = atoi(conn->keepalives_interval);
 	if (interval <= 0)
 		interval = 1;			/* 1 second = default */
 
@@ -1784,7 +1823,9 @@ connectDBStart(PGconn *conn)
 			thisport = DEF_PGPORT;
 		else
 		{
-			thisport = atoi(ch->port);
+			if (!strict_atoi(ch->port, &thisport, conn, "port"))
+				goto connect_errReturn;
+
 			if (thisport < 1 || thisport > 65535)
 			{
 				appendPQExpBuffer(&conn->errorMessage,
@@ -1916,7 +1957,9 @@ connectDBComplete(PGconn *conn)
 	 */
 	if (conn->connect_timeout != NULL)
 	{
-		timeout = atoi(conn->connect_timeout);
+		if (!strict_atoi(conn->connect_timeout, &timeout, conn, "connect_timeout"))
+			return 0;
+
 		if (timeout > 0)
 		{
 			/*
@@ -1927,6 +1970,8 @@ connectDBComplete(PGconn *conn)
 			if (timeout < 2)
 				timeout = 2;
 		}
+		else /* negative means 0 */
+			timeout = 0;
 	}
 
 	for (;;)

Peter Eisentraut

peter.eisentraut@2ndquadrant.com

over 7 years ago

In reply to: Fabien COELHO (#1)

Re: libpq stricter integer parsing

On 17/08/2018 12:13, Fabien COELHO wrote:

sh> psql "connect_timeout=2,port=5433"

The timeout is set to 2, and the port directive is silently ignored.
However, URL parsing is stricter, eg on "port".

The attached patch checks integer syntax errors and overflows, and report
errors.

This looks useful and the patch looks reasonable, but it doesn't apply
anymore. Can you send in an update?

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Fabien COELHO

coelho@cri.ensmp.fr

over 7 years ago

In reply to: Peter Eisentraut (#2)

Re: libpq stricter integer parsing

Hello Peter,

The timeout is set to 2, and the port directive is silently ignored.
However, URL parsing is stricter, eg on "port".

The attached patch checks integer syntax errors and overflows, and report
errors.

This looks useful and the patch looks reasonable, but it doesn't apply
anymore. Can you send in an update?

Hmmm. It does apply on a test I just did right know...

Usually it does not apply with "git apply" if your MUA does not know that
MIME requires CR LF eol on text files, and that it is its responsability
to switch to something else if desired. Pg automatic patch checker does
not know about that so complains unduly. Some MUA send attachements which
violates MIME (text attachements with LF only eol), thus hidding the
issue.

Otherwise, the "patch" command should work?

--
Fabien.

Michael Paquier

michael@paquier.xyz

over 7 years ago

In reply to: Fabien COELHO (#3)

Re: libpq stricter integer parsing

On Fri, Sep 07, 2018 at 05:13:17PM +0200, Fabien COELHO wrote:

Hmmm. It does apply on a test I just did right know...

That's weird, it does not apply for me either with patch -p1 and there
is on conflict in fe-connect.c, which looks easy enough to fix by the
way.
--
Michael

Fabien COELHO

coelho@cri.ensmp.fr

over 7 years ago

In reply to: Michael Paquier (#4)

1 attachment(s)

Re: libpq stricter integer parsing

Hello Michael,

Hmmm. It does apply on a test I just did right know...

That's weird, it does not apply for me either with patch -p1 and there
is on conflict in fe-connect.c, which looks easy enough to fix by the
way.

Weird indeed. However, even if the patch applied cleanly for me, it was
not compiling anymore. Attached an updated version, in which I also tried
to improve the error message on trailing garbage.

--
Fabien

Attachments:

libpq-strict-atoi-2.patchtext/x-diff; charset=us-ascii; name=libpq-strict-atoi-2.patchDownload

diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 5e7931ba90..a6cbbea655 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -1591,6 +1591,16 @@ 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 strictly.
+    Versions of <application>libpq</application> before
+    <product>PostgreSQL 12</product> accepted trailing garbage or overflows.
+   </para>
   </sect2>
  </sect1>
 
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index db5aacd0e9..5ea51d5c55 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -1586,6 +1586,35 @@ useKeepalives(PGconn *conn)
 	return val != 0 ? 1 : 0;
 }
 
+/*
+ * Parse integer, report any syntax error, and tell if all is well.
+ */
+static bool
+strict_atoi(const char *str, int *val, PGconn *conn, const char *context)
+{
+	char 	*endptr;
+
+	errno = 0;
+	*val = strtol(str, &endptr, 10);
+
+	if (errno != 0)
+	{
+		appendPQExpBuffer(&conn->errorMessage,
+						  libpq_gettext("invalid integer \"%s\" for keyword \"%s\": %s\n"),
+						  str, context, strerror(errno));
+		return false;
+	}
+	else if (*endptr != '\0')
+	{
+		appendPQExpBuffer(&conn->errorMessage,
+						  libpq_gettext("invalid integer \"%s\" for keyword \"%s\": trailing garbage\n"),
+						  str, context);
+		return false;
+	}
+
+	return true;
+}
+
 #ifndef WIN32
 /*
  * Set the keepalive idle timer.
@@ -1598,7 +1627,9 @@ setKeepalivesIdle(PGconn *conn)
 	if (conn->keepalives_idle == NULL)
 		return 1;
 
-	idle = atoi(conn->keepalives_idle);
+	if (!strict_atoi(conn->keepalives_idle, &idle, conn, "keepalives_idle"))
+		return 0;
+
 	if (idle < 0)
 		idle = 0;
 
@@ -1630,7 +1661,9 @@ setKeepalivesInterval(PGconn *conn)
 	if (conn->keepalives_interval == NULL)
 		return 1;
 
-	interval = atoi(conn->keepalives_interval);
+	if (!strict_atoi(conn->keepalives_interval, &interval, conn, "keepalives_interval"))
+		return 0;
+
 	if (interval < 0)
 		interval = 0;
 
@@ -1663,7 +1696,9 @@ setKeepalivesCount(PGconn *conn)
 	if (conn->keepalives_count == NULL)
 		return 1;
 
-	count = atoi(conn->keepalives_count);
+	if (!strict_atoi(conn->keepalives_count, &count, conn, "keepalives_count"))
+		return 0;
+
 	if (count < 0)
 		count = 0;
 
@@ -1697,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 &&
+		!strict_atoi(conn->keepalives_idle, &idle, conn, "keepalives_idle"))
+		return 0;
+
+	if (conn->keepalives_interval &&
+		!strict_atoi(conn->keepalives_interval, &interval, conn, "keepalives_interval"))
+		return 0;
+
 	if (idle <= 0)
 		idle = 2 * 60 * 60;		/* 2 hours = default */
 
-	if (conn->keepalives_interval)
-		interval = atoi(conn->keepalives_interval);
 	if (interval <= 0)
 		interval = 1;			/* 1 second = default */
 
@@ -1817,7 +1856,9 @@ connectDBComplete(PGconn *conn)
 	 */
 	if (conn->connect_timeout != NULL)
 	{
-		timeout = atoi(conn->connect_timeout);
+		if (!strict_atoi(conn->connect_timeout, &timeout, conn, "connect_timeout"))
+			return 0;
+
 		if (timeout > 0)
 		{
 			/*
@@ -1828,6 +1869,8 @@ connectDBComplete(PGconn *conn)
 			if (timeout < 2)
 				timeout = 2;
 		}
+		else /* negative means 0 */
+			timeout = 0;
 	}
 
 	for (;;)
@@ -2094,7 +2137,9 @@ keep_going:						/* We will come back to here until there is
 			thisport = DEF_PGPORT;
 		else
 		{
-			thisport = atoi(ch->port);
+			if (!strict_atoi(ch->port, &thisport, conn, "port"))
+				goto error_return;
+
 			if (thisport < 1 || thisport > 65535)
 			{
 				appendPQExpBuffer(&conn->errorMessage,

Michael Paquier

michael@paquier.xyz

over 7 years ago

In reply to: Fabien COELHO (#5)

Re: libpq stricter integer parsing

On Fri, Sep 07, 2018 at 11:22:14PM +0200, Fabien COELHO wrote:

Weird indeed. However, even if the patch applied cleanly for me, it was not
compiling anymore. Attached an updated version, in which I also tried to
improve the error message on trailing garbage.

At least now it applies for me, thanks.

+    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 strictly.
Wouldn't it be better to actually document what "parsed strictly" means?

A wild though: we already have things like pg_strtoint32 or pg_atoi
which do similar error handling in smarter ways. Wouldn't we want to
refactor the code so as libpq makes use of such routines? This would
mean that the error string should be moved around, and allowing
frontends to use those utilities requires some extra engineering.

Not that this patch should reinvent the world...
--
Michael

Fabien COELHO

coelho@cri.ensmp.fr

over 7 years ago

In reply to: Michael Paquier (#6)

Re: libpq stricter integer parsing

Hello Michael,

On Fri, Sep 07, 2018 at 11:22:14PM +0200, Fabien COELHO wrote:

Weird indeed. However, even if the patch applied cleanly for me, it was not
compiling anymore. Attached an updated version, in which I also tried to
improve the error message on trailing garbage.

At least now it applies for me, thanks.
+    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 strictly.

Wouldn't it be better to actually document what "parsed strictly" means?

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."

A wild though: we already have things like pg_strtoint32 or pg_atoi
which do similar error handling in smarter ways. Wouldn't we want to
refactor the code so as libpq makes use of such routines? This would
mean that the error string should be moved around, and allowing
frontends to use those utilities requires some extra engineering.

I thought of that but rejected it.

The pg_* function you mention are in the backend code, where error
handling is managed differently, and this function is really only about
error handling. Moreover "strtol" is already used "libpq/fe-connect.c" for
the same purpose of parsing int and detecting errors (URL port, keep
alives).

So the implied changes to try to factor out the functionnality looked like
a bad idea (where should I put it [probably pgport?]? how should I manage
errors in a client/server compatible way [no simple idea]?), hence the
local re-inventation, also to avoid any impact on the backend code.

Not that this patch should reinvent the world...

Sure.

--
Fabien.

Michael Paquier

michael@paquier.xyz

over 7 years ago

In reply to: Fabien COELHO (#7)

1 attachment(s)

Re: libpq stricter integer parsing

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

Attachments:

libpq-int-parse.patchtext/x-diff; charset=us-asciiDownload

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,

Peter Eisentraut

peter.eisentraut@2ndquadrant.com

over 7 years ago

In reply to: Michael Paquier (#8)

Re: libpq stricter integer parsing

On 11/09/2018 11:00, Michael Paquier wrote:

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>

I would leave this out. We don't need to document every single
refinement of parsing rules. This might better belong in the release notes.

+	appendPQExpBuffer(&conn->errorMessage,
+					  libpq_gettext("invalid value for keyword \"%s\"\n"),
+					  context);

Add the actual invalid value to the error message.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#10

Fabien COELHO

coelho@cri.ensmp.fr

over 7 years ago

In reply to: Peter Eisentraut (#9)

1 attachment(s)

Re: libpq stricter integer parsing

+ <para>...</para>

I would leave this out. We don't need to document every single
refinement of parsing rules. This might better belong in the release notes.

Why not, I'd be fine with that. The fact that the libpq parser was quite
fuzzy was not documented, the behavior was really a side effect of the
implementation which never suggested that it would work.

+	appendPQExpBuffer(&conn->errorMessage,
+					  libpq_gettext("invalid value for keyword \"%s\"\n"),
+					  context);

Add the actual invalid value to the error message.

Indeed.

Attached Michael's simplified version updated with your remarks.

--
Fabien.

Attachments:

libpq-strict-atoi-4.patchtext/x-diff; name=libpq-strict-atoi-4.patchDownload

diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 42cdb971a3..d79f311241 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 integer value \"%s\" for keyword \"%s\"\n"),
+					  value, 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,

#11

Michael Paquier

michael@paquier.xyz

over 7 years ago

In reply to: Fabien COELHO (#10)

Re: libpq stricter integer parsing

On Tue, Sep 11, 2018 at 07:03:41PM +0200, Fabien COELHO wrote:

Attached Michael's simplified version updated with your remarks.

Okay, this version looks good to me so pushed. Thanks Fabien and
Peter.
--
Michael