From 805889d7d23fbecf5925443deb334aaeb6beaeb0 Mon Sep 17 00:00:00 2001 From: Tom Lane Date: Wed, 15 Aug 2018 13:21:05 -0400 Subject: [PATCH] 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 --- src/port/snprintf.c | 94 ++++++++++++++++++++++++++++++--------------- 1 file changed, 63 insertions(+), 31 deletions(-) diff --git a/src/port/snprintf.c b/src/port/snprintf.c index a184134ee6..851e2ae330 100644 --- 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; -- 2.40.0