Require a C99-compliant snprintf(), and remove related workarounds.

Since our substitute snprintf now returns a C99-compliant result,
there's no need anymore to have complicated code to cope with pre-C99
behavior.  We can just make configure substitute snprintf.c if it finds
that the system snprintf() is pre-C99.  (Note: I do not believe that
there are any platforms where this test will trigger that weren't
already being rejected due to our other C99-ish feature requirements for
snprintf.  But let's add the check for paranoia's sake.)  Then, simplify
the call sites that had logic to cope with the pre-C99 definition.

I also dropped some stuff that was being paranoid about the possibility
of snprintf overrunning the given buffer.  The only reports we've ever
heard of that being a problem were for Solaris 7, which is long dead,
and we've sure not heard any reports of these assertions triggering in
a long time.  So let's drop that complexity too.

Likewise, drop some code that wasn't trusting snprintf to set errno
when it returns -1.  That would be not-per-spec, and again there's
no real reason to believe it is a live issue, especially not for
snprintfs that pass all of configure's feature checks.

Discussion: https://postgr.es/m/17245.1534289329@sss.pgh.pa.us

Author: tglsfdc

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 <stdio.h>
+#include <string.h>
+
+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
 # ------------------

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 <stdio.h>
+#include <string.h>
+
+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
 

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.])

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 */

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;
 }

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)
-		{
-			/*
-			 * 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
+		/*
+		 * 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))
 		{
-			/*
-			 * 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;
 	}