Clean up assorted misuses of snprintf()'s result value.

Fix a small number of places that were testing the result of snprintf()
but doing so incorrectly.  The right test for buffer overrun, per C99,
is "result >= bufsize" not "result > bufsize".  Some places were also
checking for failure with "result == -1", but the standard only says
that a negative value is delivered on failure.

(Note that this only makes these places correct if snprintf() delivers
C99-compliant results.  But at least now these places are consistent
with all the other places where we assume that.)

Also, make psql_start_test() and isolation_start_test() check for
buffer overrun while constructing their shell commands.  There seems
like a higher risk of overrun, with more severe consequences, here
than there is for the individual file paths that are made elsewhere
in the same functions, so this seemed like a worthwhile change.

Also fix guc.c's do_serialize() to initialize errno = 0 before
calling vsnprintf.  In principle, this should be unnecessary because
vsnprintf should have set errno if it returns a failure indication ...
but the other two places this coding pattern is cribbed from don't
assume that, so let's be consistent.

These errors are all very old, so back-patch as appropriate.  I think
that only the shell command overrun cases are even theoretically
reachable in practice, but there's not much point in erroneous error
checks.

Discussion: https://postgr.es/m/17245.1534289329@sss.pgh.pa.us

diff --git a/src/backend/libpq/ip.c b/src/backend/libpq/ip.c
index 9c3e85bbf7fd29074a8fe1d19fa70958319bf8e1..b6499529c75db751e56290df4b2cd78a3df9494b 100644 (file)
--- a/src/backend/libpq/ip.c
+++ b/src/backend/libpq/ip.c
@@ -240,7 +240,7 @@ getnameinfo_unix(const struct sockaddr_un * sa, int salen,
                                 char *service, int servicelen,
                                 int flags)
 {
-       int                     ret = -1;
+       int                     ret;
 
        /* Invalid arguments. */
        if (sa == NULL || sa->sun_family != AF_UNIX ||
@@ -250,14 +250,14 @@ getnameinfo_unix(const struct sockaddr_un * sa, int salen,
        if (node)
        {
                ret = snprintf(node, nodelen, "%s", "[local]");
-               if (ret == -1 || ret > nodelen)
+               if (ret < 0 || ret >= nodelen)
                        return EAI_MEMORY;
        }
 
        if (service)
        {
                ret = snprintf(service, servicelen, "%s", sa->sun_path);
-               if (ret == -1 || ret > servicelen)
+               if (ret < 0 || ret >= servicelen)
                        return EAI_MEMORY;
        }
 

diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index e872bf6a313fb0cf241a4a51749dee750b3ac176..8a2b69bad450845c31b5c900d6023efab9e7493e 100644 (file)
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -3676,7 +3676,7 @@ get_dbstat_filename(bool permanent, bool tempname, Oid databaseid,
                                           pgstat_stat_directory,
                                           databaseid,
                                           tempname ? "tmp" : "stat");
-       if (printed > len)
+       if (printed >= len)
                elog(ERROR, "overlength pgstat path");
 }
 

diff --git a/src/interfaces/ecpg/pgtypeslib/common.c b/src/interfaces/ecpg/pgtypeslib/common.c
index 7adca66618d8d9dc8c2cea90a922acc045f89cd1..998764d6627de4a39be130ffb457afe00546e6b2 100644 (file)
--- a/src/interfaces/ecpg/pgtypeslib/common.c
+++ b/src/interfaces/ecpg/pgtypeslib/common.c
@@ -110,7 +110,7 @@ pgtypes_fmt_replace(union un_fmt_comb replace_val, int replace_type, char **outp
                                                break;
                                }
 
-                               if (i < 0)
+                               if (i < 0 || i >= PGTYPES_FMT_NUM_MAX_DIGITS)
                                {
                                        free(t);
                                        return -1;

diff --git a/src/port/getaddrinfo.c b/src/port/getaddrinfo.c
index a35354e3728949c35f9e4937db2c07d926b4c26f..45cfe5585296b5f17fee0da5887dc506928f01b2 100644 (file)
--- a/src/port/getaddrinfo.c
+++ b/src/port/getaddrinfo.c
@@ -404,7 +404,7 @@ getnameinfo(const struct sockaddr * sa, int salen,
                        ret = snprintf(service, servicelen, "%d",
                                                   ntohs(((struct sockaddr_in *) sa)->sin_port));
                }
-               if (ret == -1 || ret >= servicelen)
+               if (ret < 0 || ret >= servicelen)
                        return EAI_MEMORY;
        }
 

diff --git a/src/test/isolation/isolation_main.c b/src/test/isolation/isolation_main.c
index 48a0e1235a2370e0b3540a2cc207aa16f0c44fe1..f692c2f8dc924c58f61e4541ec6468ed0eafd8fc 100644 (file)
--- a/src/test/isolation/isolation_main.c
+++ b/src/test/isolation/isolation_main.c
@@ -54,14 +54,26 @@ isolation_start_test(const char *testname,
        add_stringlist_item(expectfiles, expectfile);
 
        if (launcher)
+       {
                offset += snprintf(psql_cmd + offset, sizeof(psql_cmd) - offset,
                                                   "%s ", launcher);
+               if (offset >= sizeof(psql_cmd))
+               {
+                       fprintf(stderr, _("command too long\n"));
+                       exit(2);
+               }
+       }
 
-       snprintf(psql_cmd + offset, sizeof(psql_cmd) - offset,
-                        SYSTEMQUOTE "\"./isolationtester\" \"dbname=%s\" < \"%s\" > \"%s\" 2>&1" SYSTEMQUOTE,
-                        dblist->str,
-                        infile,
-                        outfile);
+       offset += snprintf(psql_cmd + offset, sizeof(psql_cmd) - offset,
+                                          SYSTEMQUOTE "\"./isolationtester\" \"dbname=%s\" < \"%s\" > \"%s\" 2>&1" SYSTEMQUOTE,
+                                          dblist->str,
+                                          infile,
+                                          outfile);
+       if (offset >= sizeof(psql_cmd))
+       {
+               fprintf(stderr, _("command too long\n"));
+               exit(2);
+       }
 
        pid = spawn_process(psql_cmd);
 

diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index f1111d9247644a6a4d7a43ffe6e2837cd6a28a7c..a074433ede27bea47bd784f32f88070b2a436004 100644 (file)
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -1102,7 +1102,7 @@ config_sspi_auth(const char *pgdata)
        } while (0)
 
        res = snprintf(fname, sizeof(fname), "%s/pg_hba.conf", pgdata);
-       if (res < 0 || res >= sizeof(fname) - 1)
+       if (res < 0 || res >= sizeof(fname))
        {
                /*
                 * Truncating this name is a fatal error, because we must not fail to

diff --git a/src/test/regress/pg_regress_main.c b/src/test/regress/pg_regress_main.c
index f13e27ff2bc7be9aa1509e186f84cbe20fef9629..c6758e3241284fcf1da03d4d05daa01bedb65f55 100644 (file)
--- a/src/test/regress/pg_regress_main.c
+++ b/src/test/regress/pg_regress_main.c
@@ -60,16 +60,28 @@ psql_start_test(const char *testname,
        add_stringlist_item(expectfiles, expectfile);
 
        if (launcher)
+       {
                offset += snprintf(psql_cmd + offset, sizeof(psql_cmd) - offset,
                                                   "%s ", launcher);
+               if (offset >= sizeof(psql_cmd))
+               {
+                       fprintf(stderr, _("command too long\n"));
+                       exit(2);
+               }
+       }
 
-       snprintf(psql_cmd + offset, sizeof(psql_cmd) - offset,
-                        SYSTEMQUOTE "\"%s%spsql\" -X -a -q -d \"%s\" < \"%s\" > \"%s\" 2>&1" SYSTEMQUOTE,
-                        psqldir ? psqldir : "",
-                        psqldir ? "/" : "",
-                        dblist->str,
-                        infile,
-                        outfile);
+       offset += snprintf(psql_cmd + offset, sizeof(psql_cmd) - offset,
+                                          SYSTEMQUOTE "\"%s%spsql\" -X -a -q -d \"%s\" < \"%s\" > \"%s\" 2>&1" SYSTEMQUOTE,
+                                          psqldir ? psqldir : "",
+                                          psqldir ? "/" : "",
+                                          dblist->str,
+                                          infile,
+                                          outfile);
+       if (offset >= sizeof(psql_cmd))
+       {
+               fprintf(stderr, _("command too long\n"));
+               exit(2);
+       }
 
        pid = spawn_process(psql_cmd);