Skip to content

Commit 6101bc2

Browse files
tglsfdctglsfdc
committed
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
1 parent efc4b48 commit 6101bc2

File tree

8 files changed

+47
-21
lines changed

8 files changed

+47
-21
lines changed

‎src/backend/postmaster/pgstat.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4748,7 +4748,7 @@ get_dbstat_filename(bool permanent, bool tempname, Oid databaseid,
47484748
pgstat_stat_directory,
47494749
databaseid,
47504750
tempname ? "tmp" : "stat");
4751-
if (printed > len)
4751+
if (printed >= len)
47524752
elog(ERROR, "overlength pgstat path");
47534753
}
47544754

‎src/backend/utils/misc/guc.c

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9161,6 +9161,8 @@ do_serialize(char **destptr, Size *maxbytes, const char *fmt,...)
91619161
if (*maxbytes <= 0)
91629162
elog(ERROR, "not enough space to serialize GUC state");
91639163

9164+
errno = 0;
9165+
91649166
va_start(vargs, fmt);
91659167
n = vsnprintf(*destptr, *maxbytes, fmt, vargs);
91669168
va_end(vargs);

‎src/common/ip.c

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -233,7 +233,7 @@ getnameinfo_unix(const struct sockaddr_un *sa, int salen,
233233
char *service, int servicelen,
234234
int flags)
235235
{
236-
int ret = -1;
236+
int ret;
237237

238238
/* Invalid arguments. */
239239
if (sa == NULL || sa->sun_family != AF_UNIX ||
@@ -243,14 +243,14 @@ getnameinfo_unix(const struct sockaddr_un *sa, int salen,
243243
if (node)
244244
{
245245
ret = snprintf(node, nodelen, "%s", "[local]");
246-
if (ret == -1 || ret > nodelen)
246+
if (ret < 0 || ret >= nodelen)
247247
return EAI_MEMORY;
248248
}
249249

250250
if (service)
251251
{
252252
ret = snprintf(service, servicelen, "%s", sa->sun_path);
253-
if (ret == -1 || ret > servicelen)
253+
if (ret < 0 || ret >= servicelen)
254254
return EAI_MEMORY;
255255
}
256256

‎src/interfaces/ecpg/pgtypeslib/common.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,7 @@ pgtypes_fmt_replace(union un_fmt_comb replace_val, int replace_type, char **outp
110110
break;
111111
}
112112

113-
if (i < 0)
113+
if (i < 0 || i >= PGTYPES_FMT_NUM_MAX_DIGITS)
114114
{
115115
free(t);
116116
return -1;

‎src/port/getaddrinfo.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -404,7 +404,7 @@ getnameinfo(const struct sockaddr *sa, int salen,
404404
ret = snprintf(service, servicelen, "%d",
405405
ntohs(((struct sockaddr_in *) sa)->sin_port));
406406
}
407-
if (ret == -1 || ret >= servicelen)
407+
if (ret < 0 || ret >= servicelen)
408408
return EAI_MEMORY;
409409
}
410410

‎src/test/isolation/isolation_main.c

Lines changed: 18 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -75,15 +75,27 @@ isolation_start_test(const char *testname,
7575
add_stringlist_item(expectfiles, expectfile);
7676

7777
if (launcher)
78+
{
7879
offset += snprintf(psql_cmd + offset, sizeof(psql_cmd) - offset,
7980
"%s ", launcher);
81+
if (offset >= sizeof(psql_cmd))
82+
{
83+
fprintf(stderr, _("command too long\n"));
84+
exit(2);
85+
}
86+
}
8087

81-
snprintf(psql_cmd + offset, sizeof(psql_cmd) - offset,
82-
"\"%s\" \"dbname=%s\" < \"%s\" > \"%s\" 2>&1",
83-
isolation_exec,
84-
dblist->str,
85-
infile,
86-
outfile);
88+
offset += snprintf(psql_cmd + offset, sizeof(psql_cmd) - offset,
89+
"\"%s\" \"dbname=%s\" < \"%s\" > \"%s\" 2>&1",
90+
isolation_exec,
91+
dblist->str,
92+
infile,
93+
outfile);
94+
if (offset >= sizeof(psql_cmd))
95+
{
96+
fprintf(stderr, _("command too long\n"));
97+
exit(2);
98+
}
8799

88100
pid = spawn_process(psql_cmd);
89101

‎src/test/regress/pg_regress.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1023,7 +1023,7 @@ config_sspi_auth(const char *pgdata)
10231023
} while (0)
10241024

10251025
res = snprintf(fname, sizeof(fname), "%s/pg_hba.conf", pgdata);
1026-
if (res < 0 || res >= sizeof(fname) - 1)
1026+
if (res < 0 || res >= sizeof(fname))
10271027
{
10281028
/*
10291029
* Truncating this name is a fatal error, because we must not fail to

‎src/test/regress/pg_regress_main.c

Lines changed: 20 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -63,20 +63,32 @@ psql_start_test(const char *testname,
6363
add_stringlist_item(expectfiles, expectfile);
6464

6565
if (launcher)
66+
{
6667
offset += snprintf(psql_cmd + offset, sizeof(psql_cmd) - offset,
6768
"%s ", launcher);
69+
if (offset >= sizeof(psql_cmd))
70+
{
71+
fprintf(stderr, _("command too long\n"));
72+
exit(2);
73+
}
74+
}
75+
76+
offset += snprintf(psql_cmd + offset, sizeof(psql_cmd) - offset,
77+
"\"%s%spsql\" -X -a -q -d \"%s\" < \"%s\" > \"%s\" 2>&1",
78+
bindir ? bindir : "",
79+
bindir ? "/" : "",
80+
dblist->str,
81+
infile,
82+
outfile);
83+
if (offset >= sizeof(psql_cmd))
84+
{
85+
fprintf(stderr, _("command too long\n"));
86+
exit(2);
87+
}
6888

6989
appnameenv = psprintf("PGAPPNAME=pg_regress/%s", testname);
7090
putenv(appnameenv);
7191

72-
snprintf(psql_cmd + offset, sizeof(psql_cmd) - offset,
73-
"\"%s%spsql\" -X -a -q -d \"%s\" < \"%s\" > \"%s\" 2>&1",
74-
bindir ? bindir : "",
75-
bindir ? "/" : "",
76-
dblist->str,
77-
infile,
78-
outfile);
79-
8092
pid = spawn_process(psql_cmd);
8193

8294
if (pid == INVALID_PID)

0 commit comments

Comments
 (0)