From cc4f6b77861803be99dfc17a38052035a0af5ae6 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Wed, 15 Aug 2018 16:29:31 -0400 Subject: [PATCH] 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 --- src/backend/postmaster/pgstat.c | 2 +- src/backend/utils/misc/guc.c | 2 ++ src/common/ip.c | 6 +++--- src/interfaces/ecpg/pgtypeslib/common.c | 2 +- src/port/getaddrinfo.c | 2 +- src/test/isolation/isolation_main.c | 24 +++++++++++++++------ src/test/regress/pg_regress.c | 2 +- src/test/regress/pg_regress_main.c | 28 ++++++++++++++++++------- 8 files changed, 47 insertions(+), 21 deletions(-) diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c index a5d1291296..8a5b2b3b42 100644 --- a/src/backend/postmaster/pgstat.c +++ b/src/backend/postmaster/pgstat.c @@ -4810,7 +4810,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 c5ba149996..f458c0eeae 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -9441,6 +9441,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 caca7be9e5..002260ed5a 100644 --- 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 c5d1621ff1..a8a7e02be0 100644 --- 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 21f1f1b94b..1054d857b3 100644 --- a/src/port/getaddrinfo.c +++ b/src/port/getaddrinfo.c @@ -405,7 +405,7 @@ getnameinfo(const struct sockaddr *sa, int salen, ret = snprintf(service, servicelen, "%d", pg_ntoh16(((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 58402b74d8..d3ada6d2bc 100644 --- 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 2ff2acc641..6890678fa8 100644 --- a/src/test/regress/pg_regress.c +++ b/src/test/regress/pg_regress.c @@ -1024,7 +1024,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 a2bd6a2cd5..bd613e4fda 100644 --- 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)