Make snprintf.c follow the C99 standard for snprintf's result value.

C99 says that the result should be the number of bytes that would have
been emitted given a large enough buffer, not the number we actually
were able to put in the buffer.  It's time to make our substitute
implementation comply with that.  Not doing so results in inefficiency
in buffer-enlargement cases, and also poses a portability hazard for
third-party code that might expect C99-compliant snprintf behavior
within Postgres.

In passing, remove useless tests for str == NULL; neither C99 nor
predecessor standards ever allowed that except when count == 0,
so I see no reason to expend cycles on making that a non-crash case
for this implementation.  Also, don't waste a byte in pg_vfprintf's
local I/O buffer; this might have performance benefits by allowing
aligned writes during flushbuffer calls.

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

diff --git a/src/port/snprintf.c b/src/port/snprintf.c
index a184134ee6b02b323a78982b60a6dc740aba4fc7..851e2ae330affd209957924af36896dec81087ce 100644 (file)
--- a/src/port/snprintf.c
+++ b/src/port/snprintf.c
@@ -2,6 +2,7 @@
  * Copyright (c) 1983, 1995, 1996 Eric P. Allman
  * Copyright (c) 1988, 1993
  *     The Regents of the University of California.  All rights reserved.
+ * Portions Copyright (c) 1996-2018, PostgreSQL Global Development Group
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -49,8 +50,8 @@
  *     SNPRINTF, VSNPRINTF and friends
  *
  * These versions have been grabbed off the net.  They have been
- * cleaned up to compile properly and support for most of the Single Unix
- * Specification has been added.  Remaining unimplemented features are:
+ * cleaned up to compile properly and support for most of the C99
+ * specification has been added.  Remaining unimplemented features are:
  *
  * 1. No locale support: the radix character is always '.' and the '
  * (single quote) format flag is ignored.
@@ -64,25 +65,24 @@
  * 5. Space and '#' flags are not implemented.
  *
  *
- * The result values of these functions are not the same across different
- * platforms.  This implementation is compatible with the Single Unix Spec:
+ * Historically the result values of sprintf/snprintf varied across platforms.
+ * This implementation now follows the C99 standard:
  *
- * 1. -1 is returned only if processing is abandoned due to an invalid
- * parameter, such as incorrect format string.  (Although not required by
- * the spec, this happens only when no characters have yet been transmitted
- * to the destination.)
+ * 1. -1 is returned if an error is detected in the format string, or if
+ * a write to the target stream fails (as reported by fwrite).  Note that
+ * overrunning snprintf's target buffer is *not* an error.
  *
- * 2. For snprintf and sprintf, 0 is returned if str == NULL or count == 0;
- * no data has been stored.
+ * 2. For successful writes to streams, the actual number of bytes written
+ * to the stream is returned.
  *
- * 3. Otherwise, the number of bytes actually transmitted to the destination
- * is returned (excluding the trailing '\0' for snprintf and sprintf).
+ * 3. For successful sprintf/snprintf, the number of bytes that would have
+ * been written to an infinite-size buffer (excluding the trailing '\0')
+ * is returned.  snprintf will truncate its output to fit in the buffer
+ * (ensuring a trailing '\0' unless count == 0), but this is not reflected
+ * in the function result.
  *
- * For snprintf with nonzero count, the result cannot be more than count-1
- * (a trailing '\0' is always stored); it is not possible to distinguish
- * buffer overrun from exact fit.  This is unlike some implementations that
- * return the number of bytes that would have been needed for the complete
- * result string.
+ * snprintf buffer overrun can be detected by checking for function result
+ * greater than or equal to the supplied count.
  */
 
 /**************************************************************
@@ -101,15 +101,27 @@
 #undef fprintf
 #undef printf
 
-/* Info about where the formatted output is going */
+/*
+ * Info about where the formatted output is going.
+ *
+ * dopr and subroutines will not write at/past bufend, but snprintf
+ * reserves one byte, ensuring it may place the trailing '\0' there.
+ *
+ * In snprintf, we use nchars to count the number of bytes dropped on the
+ * floor due to buffer overrun.  The correct result of snprintf is thus
+ * (bufptr - bufstart) + nchars.  (This isn't as inconsistent as it might
+ * seem: nchars is the number of emitted bytes that are not in the buffer now,
+ * either because we sent them to the stream or because we couldn't fit them
+ * into the buffer to begin with.)
+ */
 typedef struct
 {
        char       *bufptr;                     /* next buffer output position */
        char       *bufstart;           /* first buffer element */
-       char       *bufend;                     /* last buffer element, or NULL */
+       char       *bufend;                     /* last+1 buffer element, or NULL */
        /* bufend == NULL is for sprintf, where we assume buf is big enough */
        FILE       *stream;                     /* eventual output destination, or NULL */
-       int                     nchars;                 /* # chars already sent to stream */
+       int                     nchars;                 /* # chars sent to stream, or dropped */
        bool            failed;                 /* call is a failure; errno is set */
 } PrintfTarget;
 
@@ -147,17 +159,28 @@ int
 pg_vsnprintf(char *str, size_t count, const char *fmt, va_list args)
 {
        PrintfTarget target;
+       char            onebyte[1];
 
-       if (str == NULL || count == 0)
-               return 0;
+       /*
+        * C99 allows the case str == NULL when count == 0.  Rather than
+        * special-casing this situation further down, we substitute a one-byte
+        * local buffer.  Callers cannot tell, since the function result doesn't
+        * depend on count.
+        */
+       if (count == 0)
+       {
+               str = onebyte;
+               count = 1;
+       }
        target.bufstart = target.bufptr = str;
        target.bufend = str + count - 1;
        target.stream = NULL;
-       /* target.nchars is unused in this case */
+       target.nchars = 0;
        target.failed = false;
        dopr(&target, fmt, args);
        *(target.bufptr) = '\0';
-       return target.failed ? -1 : (target.bufptr - target.bufstart);
+       return target.failed ? -1 : (target.bufptr - target.bufstart
+                                                                + target.nchars);
 }
 
 int
@@ -177,16 +200,15 @@ pg_vsprintf(char *str, const char *fmt, va_list args)
 {
        PrintfTarget target;
 
-       if (str == NULL)
-               return 0;
        target.bufstart = target.bufptr = str;
        target.bufend = NULL;
        target.stream = NULL;
-       /* target.nchars is unused in this case */
+       target.nchars = 0;                      /* not really used in this case */
        target.failed = false;
        dopr(&target, fmt, args);
        *(target.bufptr) = '\0';
-       return target.failed ? -1 : (target.bufptr - target.bufstart);
+       return target.failed ? -1 : (target.bufptr - target.bufstart
+                                                                + target.nchars);
 }
 
 int
@@ -213,7 +235,7 @@ pg_vfprintf(FILE *stream, const char *fmt, va_list args)
                return -1;
        }
        target.bufstart = target.bufptr = buffer;
-       target.bufend = buffer + sizeof(buffer) - 1;
+       target.bufend = buffer + sizeof(buffer);        /* use the whole buffer */
        target.stream = stream;
        target.nchars = 0;
        target.failed = false;
@@ -256,6 +278,10 @@ flushbuffer(PrintfTarget *target)
 {
        size_t          nc = target->bufptr - target->bufstart;
 
+       /*
+        * Don't write anything if we already failed; this is to ensure we
+        * preserve the original failure's errno.
+        */
        if (!target->failed && nc > 0)
        {
                size_t          written;
@@ -1035,7 +1061,10 @@ dostr(const char *str, int slen, PrintfTarget *target)
                {
                        /* buffer full, can we dump to stream? */
                        if (target->stream == NULL)
-                               return;                 /* no, lose the data */
+                       {
+                               target->nchars += slen; /* no, lose the data */
+                               return;
+                       }
                        flushbuffer(target);
                        continue;
                }
@@ -1054,7 +1083,10 @@ dopr_outch(int c, PrintfTarget *target)
        {
                /* buffer full, can we dump to stream? */
                if (target->stream == NULL)
-                       return;                         /* no, lose the data */
+               {
+                       target->nchars++;       /* no, lose the data */
+                       return;
+               }
                flushbuffer(target);
        }
        *(target->bufptr++) = c;