Skip to content

Commit c182c1e

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 54db0e5 commit c182c1e

File tree

8 files changed

+46
-20
lines changed

8 files changed

+46
-20
lines changed

‎src/backend/libpq/ip.c

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -240,7 +240,7 @@ getnameinfo_unix(const struct sockaddr_un * sa, int salen,
240240
char *service, int servicelen,
241241
int flags)
242242
{
243-
int ret = -1;
243+
int ret;
244244

245245
/* Invalid arguments. */
246246
if (sa == NULL || sa->sun_family != AF_UNIX ||
@@ -250,14 +250,14 @@ getnameinfo_unix(const struct sockaddr_un * sa, int salen,
250250
if (node)
251251
{
252252
ret = snprintf(node, nodelen, "%s", "[local]");
253-
if (ret == -1 || ret > nodelen)
253+
if (ret < 0 || ret >= nodelen)
254254
return EAI_MEMORY;
255255
}
256256

257257
if (service)
258258
{
259259
ret = snprintf(service, servicelen, "%s", sa->sun_path);
260-
if (ret == -1 || ret > servicelen)
260+
if (ret < 0 || ret >= servicelen)
261261
return EAI_MEMORY;
262262
}
263263

‎src/backend/postmaster/pgstat.c

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -4070,7 +4070,7 @@ get_dbstat_filename(bool permanent, bool tempname, Oid databaseid,
40704070
pgstat_stat_directory,
40714071
databaseid,
40724072
tempname ? "tmp" : "stat");
4073-
if (printed > len)
4073+
if (printed >= len)
40744074
elog(ERROR, "overlength pgstat path");
40754075
}
40764076

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

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

9050+
errno = 0;
9051+
90509052
va_start(vargs, fmt);
90519053
n = vsnprintf(*destptr, *maxbytes, fmt, vargs);
90529054
va_end(vargs);

‎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
@@ -73,15 +73,27 @@ isolation_start_test(const char *testname,
7373
add_stringlist_item(expectfiles, expectfile);
7474

7575
if (launcher)
76+
{
7677
offset += snprintf(psql_cmd + offset, sizeof(psql_cmd) - offset,
7778
"%s ", launcher);
79+
if (offset >= sizeof(psql_cmd))
80+
{
81+
fprintf(stderr, _("command too long\n"));
82+
exit(2);
83+
}
84+
}
7885

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

8698
pid = spawn_process(psql_cmd);
8799

‎src/test/regress/pg_regress.c

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

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

‎src/test/regress/pg_regress_main.c

Lines changed: 19 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -60,16 +60,28 @@ psql_start_test(const char *testname,
6060
add_stringlist_item(expectfiles, expectfile);
6161

6262
if (launcher)
63+
{
6364
offset += snprintf(psql_cmd + offset, sizeof(psql_cmd) - offset,
6465
"%s ", launcher);
66+
if (offset >= sizeof(psql_cmd))
67+
{
68+
fprintf(stderr, _("command too long\n"));
69+
exit(2);
70+
}
71+
}
6572

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

7486
pid = spawn_process(psql_cmd);
7587

0 commit comments

Comments
 (0)