diff --git a/config/c-library.m4 b/config/c-library.m4 index 4446a5b719..6bcfceee6e 100644 --- a/config/c-library.m4 +++ b/config/c-library.m4 @@ -238,6 +238,33 @@ int main() AC_MSG_RESULT([$pgac_cv_snprintf_size_t_support]) ])# PGAC_FUNC_SNPRINTF_SIZE_T_SUPPORT +# PGAC_FUNC_SNPRINTF_C99_RESULT +# ----------------------------- +# Determine whether snprintf returns the desired buffer length when +# it overruns the actual buffer length. That's required by C99 and POSIX +# but ancient platforms don't behave that way, so we must test. +# +AC_DEFUN([PGAC_FUNC_SNPRINTF_C99_RESULT], +[AC_MSG_CHECKING([whether snprintf reports buffer overrun per C99]) +AC_CACHE_VAL(pgac_cv_snprintf_c99_result, +[AC_RUN_IFELSE([AC_LANG_SOURCE([[#include +#include + +int main() +{ + char buf[10]; + + if (snprintf(buf, sizeof(buf), "12345678901234567890") != 20) + return 1; + return 0; +}]])], +[pgac_cv_snprintf_c99_result=yes], +[pgac_cv_snprintf_c99_result=no], +[pgac_cv_snprintf_c99_result=cross]) +])dnl AC_CACHE_VAL +AC_MSG_RESULT([$pgac_cv_snprintf_c99_result]) +])# PGAC_FUNC_SNPRINTF_C99_RESULT + # PGAC_TYPE_LOCALE_T # ------------------ diff --git a/configure b/configure index 1aaca9ab5c..a2374a3ab6 100755 --- a/configure +++ b/configure @@ -16142,7 +16142,7 @@ fi # Run tests below here # -------------------- -# Force use of our snprintf if system's doesn't do arg control +# For NLS, force use of our snprintf if system's doesn't do arg control. # See comment above at snprintf test for details. if test "$enable_nls" = yes -a "$pgac_need_repl_snprintf" = no; then { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether snprintf supports argument control" >&5 @@ -16436,6 +16436,50 @@ $as_echo "$pgac_cv_snprintf_size_t_support" >&6; } fi fi +# Force use of our snprintf if the system's doesn't handle buffer overrun +# as specified by C99. +if test "$pgac_need_repl_snprintf" = no; then + { $as_echo "$as_me:${as_lineno-$LINENO}: checking whether snprintf reports buffer overrun per C99" >&5 +$as_echo_n "checking whether snprintf reports buffer overrun per C99... " >&6; } +if ${pgac_cv_snprintf_c99_result+:} false; then : + $as_echo_n "(cached) " >&6 +else + if test "$cross_compiling" = yes; then : + pgac_cv_snprintf_c99_result=cross +else + cat confdefs.h - <<_ACEOF >conftest.$ac_ext +/* end confdefs.h. */ +#include +#include + +int main() +{ + char buf[10]; + + if (snprintf(buf, sizeof(buf), "12345678901234567890") != 20) + return 1; + return 0; +} +_ACEOF +if ac_fn_c_try_run "$LINENO"; then : + pgac_cv_snprintf_c99_result=yes +else + pgac_cv_snprintf_c99_result=no +fi +rm -f core *.core core.conftest.* gmon.out bb.out conftest$ac_exeext \ + conftest.$ac_objext conftest.beam conftest.$ac_ext +fi + + +fi +{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $pgac_cv_snprintf_c99_result" >&5 +$as_echo "$pgac_cv_snprintf_c99_result" >&6; } + + if test "$pgac_cv_snprintf_c99_result" != yes; then + pgac_need_repl_snprintf=yes + fi +fi + # Now we have checked all the reasons to replace snprintf if test $pgac_need_repl_snprintf = yes; then diff --git a/configure.in b/configure.in index 24372b6cee..51766e630f 100644 --- a/configure.in +++ b/configure.in @@ -1806,7 +1806,7 @@ for the exact reason.]])], # Run tests below here # -------------------- -# Force use of our snprintf if system's doesn't do arg control +# For NLS, force use of our snprintf if system's doesn't do arg control. # See comment above at snprintf test for details. if test "$enable_nls" = yes -a "$pgac_need_repl_snprintf" = no; then PGAC_FUNC_SNPRINTF_ARG_CONTROL @@ -1858,6 +1858,15 @@ if test "$pgac_need_repl_snprintf" = no; then fi fi +# Force use of our snprintf if the system's doesn't handle buffer overrun +# as specified by C99. +if test "$pgac_need_repl_snprintf" = no; then + PGAC_FUNC_SNPRINTF_C99_RESULT + if test "$pgac_cv_snprintf_c99_result" != yes; then + pgac_need_repl_snprintf=yes + fi +fi + # Now we have checked all the reasons to replace snprintf if test $pgac_need_repl_snprintf = yes; then AC_DEFINE(USE_REPL_SNPRINTF, 1, [Use replacement snprintf() functions.]) diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c index f458c0eeae..9989d3a351 100644 --- a/src/backend/utils/misc/guc.c +++ b/src/backend/utils/misc/guc.c @@ -9441,26 +9441,19 @@ 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); - /* - * Cater to portability hazards in the vsnprintf() return value just like - * appendPQExpBufferVA() does. Note that this requires an extra byte of - * slack at the end of the buffer. Since serialize_variable() ends with a - * do_serialize_binary() rather than a do_serialize(), we'll always have - * that slack; estimate_variable_size() need not add a byte for it. - */ - if (n < 0 || n >= *maxbytes - 1) + if (n < 0) { - if (n < 0 && errno != 0 && errno != ENOMEM) - /* Shouldn't happen. Better show errno description. */ - elog(ERROR, "vsnprintf failed: %m"); - else - elog(ERROR, "not enough space to serialize GUC state"); + /* Shouldn't happen. Better show errno description. */ + elog(ERROR, "vsnprintf failed: %m"); + } + if (n >= *maxbytes) + { + /* This shouldn't happen either, really. */ + elog(ERROR, "not enough space to serialize GUC state"); } /* Shift the destptr ahead of the null terminator */ diff --git a/src/common/psprintf.c b/src/common/psprintf.c index b974a99be1..04465a18e6 100644 --- a/src/common/psprintf.c +++ b/src/common/psprintf.c @@ -77,7 +77,7 @@ psprintf(const char *fmt,...) * pvsnprintf * * Attempt to format text data under the control of fmt (an sprintf-style - * format string) and insert it into buf (which has length len, len > 0). + * format string) and insert it into buf (which has length len). * * If successful, return the number of bytes emitted, not counting the * trailing zero byte. This will always be strictly less than len. @@ -89,14 +89,11 @@ psprintf(const char *fmt,...) * Other error cases do not return, but exit via elog(ERROR) or exit(). * Hence, this shouldn't be used inside libpq. * - * This function exists mainly to centralize our workarounds for - * non-C99-compliant vsnprintf implementations. Generally, any call that - * pays any attention to the return value should go through here rather - * than calling snprintf or vsnprintf directly. - * * Note that the semantics of the return value are not exactly C99's. * First, we don't promise that the estimated buffer size is exactly right; * callers must be prepared to loop multiple times to get the right size. + * (Given a C99-compliant vsnprintf, that won't happen, but it is rumored + * that some implementations don't always return the same value ...) * Second, we return the recommended buffer size, not one less than that; * this lets overflow concerns be handled here rather than in the callers. */ @@ -105,28 +102,10 @@ pvsnprintf(char *buf, size_t len, const char *fmt, va_list args) { int nprinted; - Assert(len > 0); - - errno = 0; - - /* - * Assert check here is to catch buggy vsnprintf that overruns the - * specified buffer length. Solaris 7 in 64-bit mode is an example of a - * platform with such a bug. - */ -#ifdef USE_ASSERT_CHECKING - buf[len - 1] = '\0'; -#endif - nprinted = vsnprintf(buf, len, fmt, args); - Assert(buf[len - 1] == '\0'); - - /* - * If vsnprintf reports an error other than ENOMEM, fail. The possible - * causes of this are not user-facing errors, so elog should be enough. - */ - if (nprinted < 0 && errno != 0 && errno != ENOMEM) + /* We assume failure means the fmt is bogus, hence hard failure is OK */ + if (unlikely(nprinted < 0)) { #ifndef FRONTEND elog(ERROR, "vsnprintf failed: %m"); @@ -136,42 +115,21 @@ pvsnprintf(char *buf, size_t len, const char *fmt, va_list args) #endif } - /* - * Note: some versions of vsnprintf return the number of chars actually - * stored, not the total space needed as C99 specifies. And at least one - * returns -1 on failure. Be conservative about believing whether the - * print worked. - */ - if (nprinted >= 0 && (size_t) nprinted < len - 1) + if ((size_t) nprinted < len) { /* Success. Note nprinted does not include trailing null. */ return (size_t) nprinted; } - if (nprinted >= 0 && (size_t) nprinted > len) - { - /* - * This appears to be a C99-compliant vsnprintf, so believe its - * estimate of the required space. (If it's wrong, the logic will - * still work, but we may loop multiple times.) Note that the space - * needed should be only nprinted+1 bytes, but we'd better allocate - * one more than that so that the test above will succeed next time. - * - * In the corner case where the required space just barely overflows, - * fall through so that we'll error out below (possibly after - * looping). - */ - if ((size_t) nprinted <= MaxAllocSize - 2) - return nprinted + 2; - } - /* - * Buffer overrun, and we don't know how much space is needed. Estimate - * twice the previous buffer size, but not more than MaxAllocSize; if we - * are already at MaxAllocSize, choke. Note we use this palloc-oriented - * overflow limit even when in frontend. + * We assume a C99-compliant vsnprintf, so believe its estimate of the + * required space, and add one for the trailing null. (If it's wrong, the + * logic will still work, but we may loop multiple times.) + * + * Choke if the required space would exceed MaxAllocSize. Note we use + * this palloc-oriented overflow limit even when in frontend. */ - if (len >= MaxAllocSize) + if (unlikely((size_t) nprinted > MaxAllocSize - 1)) { #ifndef FRONTEND ereport(ERROR, @@ -183,8 +141,5 @@ pvsnprintf(char *buf, size_t len, const char *fmt, va_list args) #endif } - if (len >= MaxAllocSize / 2) - return MaxAllocSize; - - return len * 2; + return nprinted + 1; } diff --git a/src/interfaces/libpq/pqexpbuffer.c b/src/interfaces/libpq/pqexpbuffer.c index 86b16e60fb..0814ec6dbe 100644 --- a/src/interfaces/libpq/pqexpbuffer.c +++ b/src/interfaces/libpq/pqexpbuffer.c @@ -295,76 +295,50 @@ appendPQExpBufferVA(PQExpBuffer str, const char *fmt, va_list args) */ if (str->maxlen > str->len + 16) { - /* - * Note: we intentionally leave one byte unused, as a guard against - * old broken versions of vsnprintf. - */ - avail = str->maxlen - str->len - 1; - - errno = 0; + avail = str->maxlen - str->len; nprinted = vsnprintf(str->data + str->len, avail, fmt, args); /* - * If vsnprintf reports an error other than ENOMEM, fail. + * If vsnprintf reports an error, fail (we assume this means there's + * something wrong with the format string). */ - if (nprinted < 0 && errno != 0 && errno != ENOMEM) + if (unlikely(nprinted < 0)) { markPQExpBufferBroken(str); return true; } - /* - * Note: some versions of vsnprintf return the number of chars - * actually stored, not the total space needed as C99 specifies. And - * at least one returns -1 on failure. Be conservative about - * believing whether the print worked. - */ - if (nprinted >= 0 && (size_t) nprinted < avail - 1) + if ((size_t) nprinted < avail) { /* Success. Note nprinted does not include trailing null. */ str->len += nprinted; return true; } - if (nprinted >= 0 && (size_t) nprinted > avail) + /* + * We assume a C99-compliant vsnprintf, so believe its estimate of the + * required space, and add one for the trailing null. (If it's wrong, + * the logic will still work, but we may loop multiple times.) + * + * Choke if the required space would exceed INT_MAX, since str->maxlen + * can't represent more than that. + */ + if (unlikely(nprinted > INT_MAX - 1)) { - /* - * This appears to be a C99-compliant vsnprintf, so believe its - * estimate of the required space. (If it's wrong, the logic will - * still work, but we may loop multiple times.) Note that the - * space needed should be only nprinted+1 bytes, but we'd better - * allocate one more than that so that the test above will succeed - * next time. - * - * In the corner case where the required space just barely - * overflows, fail. - */ - if (nprinted > INT_MAX - 2) - { - markPQExpBufferBroken(str); - return true; - } - needed = nprinted + 2; - } - else - { - /* - * Buffer overrun, and we don't know how much space is needed. - * Estimate twice the previous buffer size, but not more than - * INT_MAX. - */ - if (avail >= INT_MAX / 2) - needed = INT_MAX; - else - needed = avail * 2; + markPQExpBufferBroken(str); + return true; } + needed = nprinted + 1; } else { /* * We have to guess at how much to enlarge, since we're skipping the - * formatting work. + * formatting work. Fortunately, because of enlargePQExpBuffer's + * preference for power-of-2 sizes, this number isn't very sensitive; + * the net effect is that we'll double the buffer size before trying + * to run vsnprintf, which seems sensible. */ needed = 32; }