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/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index 50960f738e4a672e56029d2b9d94143279e2e3b3..302c331c49399ae35ecb394f48c60a99a75ee368 100644 (file)
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -4748,7 +4748,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/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 03c1d99f217bef044ca71720da1cd02f08c53935..e71300b5e2732b41859cb77e78ded0bf6e65130f 100644 (file)
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -9161,6 +9161,8 @@ do_serialize(char **destptr, Size *maxbytes, const char *fmt,...)
    if (*maxbytes <= 0)
        elog(ERROR, "not enough space to serialize GUC state");
 
+   errno = 0;
+
    va_start(vargs, fmt);
    n = vsnprintf(*destptr, *maxbytes, fmt, vargs);
    va_end(vargs);

diff --git a/src/common/ip.c b/src/common/ip.c
index bb536d3e86c235a3ea7ca8fb0d3e161a4b25dbcc..dc964163a171e1ba0f7b2fdebe481d49fdbde1db 100644 (file)
--- a/src/common/ip.c
+++ b/src/common/ip.c
@@ -233,7 +233,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 ||
@@ -243,14 +243,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/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 e5b5702c79975a5370b247c1b3dbcbc3341757ba..617ce7f6f6b932d93e0e34ce13941d332a585303 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 8a3d7f51b39fc0f4210da6587bfd335df000002c..dcddfd8f6356c3c0a4779e79e01a456a3f8f842b 100644 (file)
--- a/src/test/isolation/isolation_main.c
+++ b/src/test/isolation/isolation_main.c
@@ -75,15 +75,27 @@ 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,
-            "\"%s\" \"dbname=%s\" < \"%s\" > \"%s\" 2>&1",
-            isolation_exec,
-            dblist->str,
-            infile,
-            outfile);
+   offset += snprintf(psql_cmd + offset, sizeof(psql_cmd) - offset,
+                      "\"%s\" \"dbname=%s\" < \"%s\" > \"%s\" 2>&1",
+                      isolation_exec,
+                      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 17e982edca9ef5569a977826665719b44aec6979..3dde4a2b609fd5ca355b5fe61fb8ab8b4521b2c4 100644 (file)
--- a/src/test/regress/pg_regress.c
+++ b/src/test/regress/pg_regress.c
@@ -1023,7 +1023,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 298ed758eebbe40fe052ea4b16b28a99163be013..01dd8b9cf3bd6d090b8dc01f7ed7195717692d40 100644 (file)
--- a/src/test/regress/pg_regress_main.c
+++ b/src/test/regress/pg_regress_main.c
@@ -63,20 +63,32 @@ 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);
+       }
+   }
+
+   offset += snprintf(psql_cmd + offset, sizeof(psql_cmd) - offset,
+                      "\"%s%spsql\" -X -a -q -d \"%s\" < \"%s\" > \"%s\" 2>&1",
+                      bindir ? bindir : "",
+                      bindir ? "/" : "",
+                      dblist->str,
+                      infile,
+                      outfile);
+   if (offset >= sizeof(psql_cmd))
+   {
+       fprintf(stderr, _("command too long\n"));
+       exit(2);
+   }
 
    appnameenv = psprintf("PGAPPNAME=pg_regress/%s", testname);
    putenv(appnameenv);
 
-   snprintf(psql_cmd + offset, sizeof(psql_cmd) - offset,
-            "\"%s%spsql\" -X -a -q -d \"%s\" < \"%s\" > \"%s\" 2>&1",
-            bindir ? bindir : "",
-            bindir ? "/" : "",
-            dblist->str,
-            infile,
-            outfile);
-
    pid = spawn_process(psql_cmd);
 
    if (pid == INVALID_PID)