From: Tom Lane <tgl@sss.pgh.pa.us>
To: pgsql-hackers@lists.postgresql.org
Subject: C99 compliance for src/port/snprintf.c
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <17005.1534288994.0@sss.pgh.pa.us>
Date: Tue, 14 Aug 2018 19:28:49 -0400
Message-ID: <17245.1534289329@sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <17005.1534288994.1@sss.pgh.pa.us>

I noticed that src/port/snprintf.c still follows the old (pre-C99)
semantics for what to return from snprintf() after buffer overrun.
This seems bad for a couple of reasons:

* It results in potential performance loss in psnprintf and
appendPQExpBufferVA, since those have to fly blind about how much
to enlarge the buffer before retrying.

* Given that we override the system-supplied *printf if it doesn't
support the 'z' width modifier, it seems highly unlikely that we are
still using any snprintf's with pre-C99 semantics, except when this
code is used.  So there's not much point in keeping this behavior
as a test case to ensure compatibility with such libraries.

* When we install snprintf.c, port.h forces it to be used everywhere
that includes c.h, including extensions.  It seems quite possible that
there is extension code out there that assumes C99 snprintf behavior.
Such code would work today everywhere except Windows, since that's the
only platform made in this century that requires snprintf.c.  Between
the existing Windows porting hazard and our nearby discussion about
using snprintf.c on more platforms, I'd say this is a gotcha waiting
to bite somebody.

Hence, PFA a patch that changes snprintf.c to follow C99 by counting
dropped bytes in the result of snprintf(), including in the case where
the passed count is zero.

(I also dropped the tests for str == NULL when count isn't zero; that's
not permitted by either C99 or SUSv2, so I see no reason for this code
to support it.  Also, avoid wasting one byte in the local buffer for
*fprintf.)

I'm almost tempted to think that the reasons above make this a
back-patchable bug fix.  Comments?

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="c99-compliant-snprintf-result-1.patch";
	charset="us-ascii"
Content-ID: <17005.1534288994.2@sss.pgh.pa.us>
Content-Description: c99-compliant-snprintf-result-1.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/port/snprintf.c b/src/port/snprintf.c
index a184134..851e2ae 100644
*** a/src/port/snprintf.c
--- b/src/port/snprintf.c
***************
*** 2,7 ****
--- 2,8 ----
   * 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,56 ****
   *	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 Uni=
x
!  * 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.
--- 50,57 ----
   *	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 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,88 ****
   * 5. Space and '#' flags are not implemented.
   *
   *
!  * The result values of these functions are not the same across differen=
t
!  * platforms.  This implementation is compatible with the Single Unix Sp=
ec:
   *
!  * 1. -1 is returned only if processing is abandoned due to an invalid
!  * parameter, such as incorrect format string.  (Although not required b=
y
!  * the spec, this happens only when no characters have yet been transmit=
ted
!  * to the destination.)
   *
!  * 2. For snprintf and sprintf, 0 is returned if str =3D=3D NULL or coun=
t =3D=3D 0;
!  * no data has been stored.
   *
!  * 3. Otherwise, the number of bytes actually transmitted to the destina=
tion
!  * is returned (excluding the trailing '\0' for snprintf and sprintf).
   *
!  * 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 t=
hat
!  * return the number of bytes that would have been needed for the comple=
te
!  * result string.
   */
  =

  /**************************************************************
--- 65,88 ----
   * 5. Space and '#' flags are not implemented.
   *
   *
!  * Historically the result values of sprintf/snprintf varied across plat=
forms.
!  * This implementation now follows the C99 standard:
   *
!  * 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 tha=
t
!  * overrunning snprintf's target buffer is *not* an error.
   *
!  * 2. For successful writes to streams, the actual number of bytes writt=
en
!  * to the stream is returned.
   *
!  * 3. For successful sprintf/snprintf, the number of bytes that would ha=
ve
!  * 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 =3D=3D 0), but this is not ref=
lected
!  * in the function result.
   *
!  * snprintf buffer overrun can be detected by checking for function resu=
lt
!  * greater than or equal to the supplied count.
   */
  =

  /**************************************************************
***************
*** 101,115 ****
  #undef	fprintf
  #undef	printf
  =

! /* Info about where the formatted output is going */
  typedef struct
  {
  	char	   *bufptr;			/* next buffer output position */
  	char	   *bufstart;		/* first buffer element */
! 	char	   *bufend;			/* last buffer element, or NULL */
  	/* bufend =3D=3D 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 */
  	bool		failed;			/* call is a failure; errno is set */
  } PrintfTarget;
  =

--- 101,127 ----
  #undef	fprintf
  #undef	printf
  =

! /*
!  * 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 th=
e
!  * floor due to buffer overrun.  The correct result of snprintf is thus
!  * (bufptr - bufstart) + nchars.  (This isn't as inconsistent as it migh=
t
!  * seem: nchars is the number of emitted bytes that are not in the buffe=
r 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+1 buffer element, or NULL */
  	/* bufend =3D=3D NULL is for sprintf, where we assume buf is big enough=
 */
  	FILE	   *stream;			/* eventual output destination, or NULL */
! 	int			nchars;			/* # chars sent to stream, or dropped */
  	bool		failed;			/* call is a failure; errno is set */
  } PrintfTarget;
  =

*************** int
*** 147,163 ****
  pg_vsnprintf(char *str, size_t count, const char *fmt, va_list args)
  {
  	PrintfTarget target;
  =

! 	if (str =3D=3D NULL || count =3D=3D 0)
! 		return 0;
  	target.bufstart =3D target.bufptr =3D str;
  	target.bufend =3D str + count - 1;
  	target.stream =3D NULL;
! 	/* target.nchars is unused in this case */
  	target.failed =3D false;
  	dopr(&target, fmt, args);
  	*(target.bufptr) =3D '\0';
! 	return target.failed ? -1 : (target.bufptr - target.bufstart);
  }
  =

  int
--- 159,186 ----
  pg_vsnprintf(char *str, size_t count, const char *fmt, va_list args)
  {
  	PrintfTarget target;
+ 	char		onebyte[1];
  =

! 	/*
! 	 * C99 allows the case str =3D=3D NULL when count =3D=3D 0.  Rather tha=
n
! 	 * 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 =3D=3D 0)
! 	{
! 		str =3D onebyte;
! 		count =3D 1;
! 	}
  	target.bufstart =3D target.bufptr =3D str;
  	target.bufend =3D str + count - 1;
  	target.stream =3D NULL;
! 	target.nchars =3D 0;
  	target.failed =3D false;
  	dopr(&target, fmt, args);
  	*(target.bufptr) =3D '\0';
! 	return target.failed ? -1 : (target.bufptr - target.bufstart
! 								 + target.nchars);
  }
  =

  int
*************** pg_vsprintf(char *str, const char *fmt, =

*** 177,192 ****
  {
  	PrintfTarget target;
  =

- 	if (str =3D=3D NULL)
- 		return 0;
  	target.bufstart =3D target.bufptr =3D str;
  	target.bufend =3D NULL;
  	target.stream =3D NULL;
! 	/* target.nchars is unused in this case */
  	target.failed =3D false;
  	dopr(&target, fmt, args);
  	*(target.bufptr) =3D '\0';
! 	return target.failed ? -1 : (target.bufptr - target.bufstart);
  }
  =

  int
--- 200,214 ----
  {
  	PrintfTarget target;
  =

  	target.bufstart =3D target.bufptr =3D str;
  	target.bufend =3D NULL;
  	target.stream =3D NULL;
! 	target.nchars =3D 0;			/* not really used in this case */
  	target.failed =3D false;
  	dopr(&target, fmt, args);
  	*(target.bufptr) =3D '\0';
! 	return target.failed ? -1 : (target.bufptr - target.bufstart
! 								 + target.nchars);
  }
  =

  int
*************** pg_vfprintf(FILE *stream, const char *fm
*** 213,219 ****
  		return -1;
  	}
  	target.bufstart =3D target.bufptr =3D buffer;
! 	target.bufend =3D buffer + sizeof(buffer) - 1;
  	target.stream =3D stream;
  	target.nchars =3D 0;
  	target.failed =3D false;
--- 235,241 ----
  		return -1;
  	}
  	target.bufstart =3D target.bufptr =3D buffer;
! 	target.bufend =3D buffer + sizeof(buffer);	/* use the whole buffer */
  	target.stream =3D stream;
  	target.nchars =3D 0;
  	target.failed =3D false;
*************** flushbuffer(PrintfTarget *target)
*** 256,261 ****
--- 278,287 ----
  {
  	size_t		nc =3D 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;
*************** dostr(const char *str, int slen, PrintfT
*** 1035,1041 ****
  		{
  			/* buffer full, can we dump to stream? */
  			if (target->stream =3D=3D NULL)
! 				return;			/* no, lose the data */
  			flushbuffer(target);
  			continue;
  		}
--- 1061,1070 ----
  		{
  			/* buffer full, can we dump to stream? */
  			if (target->stream =3D=3D NULL)
! 			{
! 				target->nchars +=3D slen; /* no, lose the data */
! 				return;
! 			}
  			flushbuffer(target);
  			continue;
  		}
*************** dopr_outch(int c, PrintfTarget *target)
*** 1054,1060 ****
  	{
  		/* buffer full, can we dump to stream? */
  		if (target->stream =3D=3D NULL)
! 			return;				/* no, lose the data */
  		flushbuffer(target);
  	}
  	*(target->bufptr++) =3D c;
--- 1083,1092 ----
  	{
  		/* buffer full, can we dump to stream? */
  		if (target->stream =3D=3D NULL)
! 		{
! 			target->nchars++;	/* no, lose the data */
! 			return;
! 		}
  		flushbuffer(target);
  	}
  	*(target->bufptr++) =3D c;

------- =_aaaaaaaaaa0--