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 signature.asc Description: PGP signature
Re: libpq stricter integer parsing
+ ... 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(>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.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, , 10); + if (errno == 0 && *end == '\0' && numval == (int) numval) + { + *result = numval; + return true; + } + + appendPQExpBuffer(>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, , 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, , 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, , 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, , 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, , 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, , 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, , conn, "port")) +goto error_return; + if (thisport < 1 || thisport > 65535) { appendPQExpBuffer(>errorMessage,
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 > > > > + > + > +Integer values expected for keywords port, > +connect_timeout, keepalives_idle, > +keepalives_interval and > +keepalives_timeout are parsed more strictly as > +of PostgreSQL 12, i.e. values including trailing > garbage > +or overflowing are rejected. > + > > 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(>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
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 libpq before > PostgreSQL 12 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=50 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 + + +Integer values expected for keywords port, +connect_timeout, keepalives_idle, +keepalives_interval and +keepalives_timeout are parsed more strictly as +of PostgreSQL 12, i.e. values including trailing garbage +or overflowing are rejected. + 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, , 10); + if (errno == 0 && *end == '\0' && numval == (int) numval) + { + *result = numval; + return true; + } + + appendPQExpBuffer(>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, , 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, , 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, , 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, , 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, , 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, , 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, , conn, "port")) +goto error_return; + if (thisport < 1 || thisport > 65535) { appendPQExpBuffer(>errorMessage, signature.asc Description: PGP signature
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 port, +connect_timeout, +keepalives_idle, +keepalives_interval and +keepalives_timeout 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 libpq before PostgreSQL 12 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.
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 port, +connect_timeout, +keepalives_idle, +keepalives_interval and +keepalives_timeout 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 signature.asc Description: PGP signature
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. -- Fabiendiff --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 + + +Integer values expected for keywords port, +connect_timeout, +keepalives_idle, +keepalives_interval and +keepalives_timeout are parsed strictly. +Versions of libpq before +PostgreSQL 12 accepted trailing garbage or overflows. + 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, , 10); + + if (errno != 0) + { + appendPQExpBuffer(>errorMessage, + libpq_gettext("invalid integer \"%s\" for keyword \"%s\": %s\n"), + str, context, strerror(errno)); + return false; + } + else if (*endptr != '\0') + { + appendPQExpBuffer(>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, , 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, , 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, , 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, , conn, "keepalives_idle")) + return 0; + + if (conn->keepalives_interval && + !strict_atoi(conn->keepalives_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, , 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, , conn, "port")) +goto error_return; + if (thisport < 1 || thisport > 65535) { appendPQExpBuffer(>errorMessage,
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 signature.asc Description: PGP signature
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.
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
libpq stricter integer parsing
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.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 + + +Integer values expected for keywords port, +connect_timeout, +keepalives_idle, +keepalives_interval and +keepalives_timeout are parsed strictly. +Versions of libpq before +PostgreSQL 12 accepted trailing garbage or overflows. + 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, , 10); + + if (errno != 0) + { + appendPQExpBuffer(>errorMessage, + libpq_gettext("invalid integer \"%s\" for keyword \"%s\": %s\n"), + str, context, strerror(errno)); + return false; + } + else if (*endptr != '\0') + { + appendPQExpBuffer(>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, , 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, , 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, , 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, , conn, "keepalives_idle")) + return 0; + + if (conn->keepalives_interval && + !strict_atoi(conn->keepalives_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, , conn, "port")) +goto connect_errReturn; + if (thisport < 1 || thisport > 65535) { appendPQExpBuffer(>errorMessage, @@ -1916,7 +1957,9 @@ connectDBComplete(PGconn *conn) */ if (conn->connect_timeout != NULL) { - timeout = atoi(conn->connect_timeout); + if (!strict_atoi(conn->connect_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 (;;)