Re: C99 compliance for src/port/snprintf.c - Mailing list pgsql-hackers

I wrote:
> BTW, independently of whether to back-patch, it strikes me that what
> we ought to do in HEAD (after applying this) is to just assume we have
> C99-compliant behavior, and rip out the baroque logic in psnprintf
> and appendPQExpBufferVA that tries to deal with pre-C99 snprintf.

Here's a proposed patch for that.  It also gets rid of some ancient
code that tried to deal with snprintfs that were outright broken,
such as writing past the end of the specified buffer.  Even if anyone
is still using platforms where that's a problem, I'd expect that we'd
have rejected the system snprintf thanks to configure's feature checks.

            regards, tom lane

diff --git a/config/c-library.m4 b/config/c-library.m4
index 4446a5b..6bcfcee 100644
*** a/config/c-library.m4
--- b/config/c-library.m4
*************** int main()
*** 238,243 ****
--- 238,270 ----
  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
  # ------------------
diff --git a/configure b/configure
index 067fc43..863d07f 100755
*** a/configure
--- b/configure
*************** fi
*** 15965,15971 ****
  # Run tests below here
  # --------------------

! # 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
--- 15965,15971 ----
  # Run tests below here
  # --------------------

! # 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
*************** $as_echo "$pgac_cv_snprintf_size_t_suppo
*** 16259,16264 ****
--- 16259,16308 ----
    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

diff --git a/configure.in b/configure.in
index 49257e5..1c529e7 100644
*** a/configure.in
--- b/configure.in
*************** for the exact reason.]])],
*** 1805,1811 ****
  # Run tests below here
  # --------------------

! # 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
--- 1805,1811 ----
  # Run tests below here
  # --------------------

! # 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
*************** if test "$pgac_need_repl_snprintf" = no;
*** 1857,1862 ****
--- 1857,1871 ----
    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 f458c0e..9989d3a 100644
*** a/src/backend/utils/misc/guc.c
--- b/src/backend/utils/misc/guc.c
*************** do_serialize(char **destptr, Size *maxby
*** 9441,9466 ****
      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 && 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");
      }

      /* Shift the destptr ahead of the null terminator */
--- 9441,9459 ----
      if (*maxbytes <= 0)
          elog(ERROR, "not enough space to serialize GUC state");

      va_start(vargs, fmt);
      n = vsnprintf(*destptr, *maxbytes, fmt, vargs);
      va_end(vargs);

!     if (n < 0)
      {
!         /* 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 b974a99..04465a1 100644
*** a/src/common/psprintf.c
--- b/src/common/psprintf.c
*************** psprintf(const char *fmt,...)
*** 77,83 ****
   * 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).
   *
   * If successful, return the number of bytes emitted, not counting the
   * trailing zero byte.  This will always be strictly less than len.
--- 77,83 ----
   * 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).
   *
   * If successful, return the number of bytes emitted, not counting the
   * trailing zero byte.  This will always be strictly less than len.
*************** psprintf(const char *fmt,...)
*** 89,102 ****
   * 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.
   * Second, we return the recommended buffer size, not one less than that;
   * this lets overflow concerns be handled here rather than in the callers.
   */
--- 89,99 ----
   * Other error cases do not return, but exit via elog(ERROR) or exit().
   * Hence, this shouldn't be used inside libpq.
   *
   * 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.
   */
*************** pvsnprintf(char *buf, size_t len, const
*** 105,132 ****
  {
      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)
      {
  #ifndef FRONTEND
          elog(ERROR, "vsnprintf failed: %m");
--- 102,111 ----
  {
      int            nprinted;

      nprinted = vsnprintf(buf, len, fmt, args);

!     /* We assume failure means the fmt is bogus, hence hard failure is OK */
!     if (unlikely(nprinted < 0))
      {
  #ifndef FRONTEND
          elog(ERROR, "vsnprintf failed: %m");
*************** pvsnprintf(char *buf, size_t len, const
*** 136,177 ****
  #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)
      {
          /* 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.
       */
!     if (len >= MaxAllocSize)
      {
  #ifndef FRONTEND
          ereport(ERROR,
--- 115,135 ----
  #endif
      }

!     if ((size_t) nprinted < len)
      {
          /* Success.  Note nprinted does not include trailing null. */
          return (size_t) nprinted;
      }

      /*
!      * 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 (unlikely((size_t) nprinted > MaxAllocSize - 1))
      {
  #ifndef FRONTEND
          ereport(ERROR,
*************** pvsnprintf(char *buf, size_t len, const
*** 183,190 ****
  #endif
      }

!     if (len >= MaxAllocSize / 2)
!         return MaxAllocSize;
!
!     return len * 2;
  }
--- 141,145 ----
  #endif
      }

!     return nprinted + 1;
  }
diff --git a/src/interfaces/libpq/pqexpbuffer.c b/src/interfaces/libpq/pqexpbuffer.c
index 86b16e6..0814ec6 100644
*** a/src/interfaces/libpq/pqexpbuffer.c
--- b/src/interfaces/libpq/pqexpbuffer.c
*************** appendPQExpBufferVA(PQExpBuffer str, con
*** 295,370 ****
       */
      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;

          nprinted = vsnprintf(str->data + str->len, avail, fmt, args);

          /*
!          * If vsnprintf reports an error other than ENOMEM, fail.
           */
!         if (nprinted < 0 && errno != 0 && errno != ENOMEM)
          {
              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)
          {
              /* 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
          {
!             /*
!              * 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;
          }
      }
      else
      {
          /*
           * We have to guess at how much to enlarge, since we're skipping the
!          * formatting work.
           */
          needed = 32;
      }
--- 295,344 ----
       */
      if (str->maxlen > str->len + 16)
      {
!         avail = str->maxlen - str->len;

          nprinted = vsnprintf(str->data + str->len, avail, fmt, args);

          /*
!          * If vsnprintf reports an error, fail (we assume this means there's
!          * something wrong with the format string).
           */
!         if (unlikely(nprinted < 0))
          {
              markPQExpBufferBroken(str);
              return true;
          }

!         if ((size_t) nprinted < avail)
          {
              /* Success.  Note nprinted does not include trailing null. */
              str->len += nprinted;
              return true;
          }

!         /*
!          * 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))
          {
!             markPQExpBufferBroken(str);
!             return true;
          }
+         needed = nprinted + 1;
      }
      else
      {
          /*
           * We have to guess at how much to enlarge, since we're skipping the
!          * 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;
      }