From d2b0b60e71931997455afd5da72ca29148f1ca51 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Thu, 6 Dec 2018 15:08:44 -0500 Subject: [PATCH] Improve our response to invalid format strings, and detect more cases. MIME-Version: 1.0 Content-Type: text/plain; charset=utf8 Content-Transfer-Encoding: 8bit Places that are testing for *printf failure ought to include the format string in their error reports, since bad-format-string is one of the more likely causes of such failure. This both makes it easier to find and repair the mistake, and provides at least some useful info to the user who stumbles across such a problem. Also, tighten snprintf.c to report EINVAL for an invalid flag or final character in a format %-spec (including the case where the %-spec is missing a final character altogether). This seems like better project policy, and it also allows removing an instruction or two from the hot code path. Back-patch the error reporting change in pvsnprintf, since it should be harmless and may be helpful; but not the snprintf.c change. Per discussion of bug #15511 from Ertuğrul Kahveci, which reported an invalid translated format string. These changes don't fix that error, but they should improve matters next time we make such a mistake. Discussion: https://postgr.es/m/15511-1d8b6a0bc874112f@postgresql.org --- src/backend/utils/misc/guc.c | 4 ++-- src/common/psprintf.c | 4 ++-- src/port/snprintf.c | 13 +++++++++---- 3 files changed, 13 insertions(+), 8 deletions(-) diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index 03594e77fe..380741e648 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -4387,7 +4387,7 @@ static struct config_enum ConfigureNamesEnum[] = }, &ssl_min_protocol_version, PG_TLS1_VERSION, - ssl_protocol_versions_info + 1 /* don't allow PG_TLS_ANY */, + ssl_protocol_versions_info + 1, /* don't allow PG_TLS_ANY */ NULL, NULL, NULL }, @@ -9666,7 +9666,7 @@ do_serialize(char **destptr, Size *maxbytes, const char *fmt,...) if (n < 0) { /* Shouldn't happen. Better show errno description. */ - elog(ERROR, "vsnprintf failed: %m"); + elog(ERROR, "vsnprintf failed: %m with format string \"%s\"", fmt); } if (n >= *maxbytes) { diff --git a/src/common/psprintf.c b/src/common/psprintf.c index 2cf100f095..411713bac8 100644 --- a/src/common/psprintf.c +++ b/src/common/psprintf.c @@ -113,9 +113,9 @@ pvsnprintf(char *buf, size_t len, const char *fmt, va_list args) if (unlikely(nprinted < 0)) { #ifndef FRONTEND - elog(ERROR, "vsnprintf failed: %m"); + elog(ERROR, "vsnprintf failed: %m with format string \"%s\"", fmt); #else - fprintf(stderr, "vsnprintf failed: %s\n", strerror(errno)); + fprintf(stderr, "vsnprintf failed: %m with format string \"%s\"\n", fmt); exit(EXIT_FAILURE); #endif } diff --git a/src/port/snprintf.c b/src/port/snprintf.c index c79cb88497..a773381617 100644 --- a/src/port/snprintf.c +++ b/src/port/snprintf.c @@ -452,8 +452,6 @@ dopr(PrintfTarget *target, const char *format, va_list args) have_star = afterstar = false; nextch2: ch = *format++; - if (ch == '\0') - break; /* illegal, but we don't complain */ switch (ch) { case '-': @@ -718,6 +716,13 @@ nextch2: case '%': dopr_outch('%', target); break; + default: + + /* + * Anything else --- in particular, '\0' indicating end of + * format string --- is bogus. + */ + goto bad_format; } /* Check for failure after each conversion spec */ @@ -782,8 +787,6 @@ find_arguments(const char *format, va_list args, afterstar = false; nextch1: ch = *format++; - if (ch == '\0') - break; /* illegal, but we don't complain */ switch (ch) { case '-': @@ -918,6 +921,8 @@ nextch1: case 'm': case '%': break; + default: + return false; /* bogus format string */ } /* -- 2.40.0