Subject: Re: lib/35401
To: None <lib-bug-people@netbsd.org, gnats-admin@netbsd.org,>
From: Christian Biere <christianbiere@gmx.de>
List: netbsd-bugs
Date: 01/14/2007 23:35:02
The following reply was made to PR lib/35401; it has been noted by GNATS.

From: Christian Biere <christianbiere@gmx.de>
To: gnats-bugs@NetBSD.org
Cc: 
Subject: Re: lib/35401
Date: Mon, 15 Jan 2007 00:38:24 +0100

 Here an updated patch which should be a bit more readable. Note that it looks
 like there's an issue with '%s' too if the string is longer than INT_MAX, so
 I've added an overflow check there as well. Further, the use of memchr() is
 incorrect because the precision may legally exceed the array length, so
 that a non-trivial implementation could read out-of-bounds. strnlen() would
 be the right choice. Since NetBSD doesn't have strnlen(), I've put the equivalent
 code in place.
 
 vfwprint.c needs the same fixes albeit it already has some (unsigned) casts in
 place to avoid undefined behaviour.
 
 Should I commit or are there any objections?
 
 
 --- vfprintf.c.orig	2007-01-14 21:57:52.000000000 +0100
 +++ vfprintf.c	2007-01-15 00:32:07.000000000 +0100
 @@ -59,6 +59,7 @@
  #include <stdlib.h>
  #include <string.h>
  #include <wchar.h>
 +#include <limits.h>
  
  #include "reentrant.h"
  #include "local.h"
 @@ -176,6 +177,19 @@
  #define	ZEROPAD		0x400		/* zero (as opposed to blank) pad */
  #define FPT		0x800		/* Floating point number */
  
 +static int
 +add_digit(unsigned value, int ch)
 +{
 +	unsigned ret, d;
 +
 +	d = to_digit(ch);
 +	ret = value * 10;
 +	if (ret < value || ret > INT_MAX - d) {
 +		return -1;
 +	}
 +	return ret + d;
 +}
 +
  int
  vfprintf(fp, fmt0, ap)
  	FILE *fp;
 @@ -202,7 +216,7 @@
  {
  	const char *fmt;/* format string */
  	int ch;	/* character from fmt */
 -	int n, m;	/* handy integers (short term usage) */
 +	int n;	/* handy integer (short term usage) */
  	const char *cp;	/* handy char pointer (short term usage) */
  	char *bp;	/* handy char pointer (short term usage) */
  	struct __siov *iovp;/* for PRINT macro */
 @@ -350,7 +364,10 @@
  				}
  			}
  		}
 -		if ((m = fmt - cp) != 0) {
 +		if (fmt != cp) {
 +			ptrdiff_t m = fmt - cp;
 +			if (m < 0 || m > INT_MAX - ret)
 +				goto overflow;
  			PRINT(cp, m);
  			ret += m;
  		}
 @@ -387,6 +404,8 @@
  			 */
  			if ((width = va_arg(ap, int)) >= 0)
  				goto rflag;
 +			if (-(unsigned)width > INT_MAX)
 +				goto overflow;
  			width = -width;
  			/* FALLTHROUGH */
  		case '-':
 @@ -401,12 +420,15 @@
  				prec = n < 0 ? -1 : n;
  				goto rflag;
  			}
 -			n = 0;
 +			prec = 0;
 +			while ('0' == ch)
 +				ch = *fmt++;
  			while (is_digit(ch)) {
 -				n = 10 * n + to_digit(ch);
 +				prec = add_digit(prec, ch);
 +				if (prec < 0)
 +					goto overflow;
  				ch = *fmt++;
  			}
 -			prec = n < 0 ? -1 : n;
  			goto reswitch;
  		case '0':
  			/*
 @@ -418,12 +440,13 @@
  			goto rflag;
  		case '1': case '2': case '3': case '4':
  		case '5': case '6': case '7': case '8': case '9':
 -			n = 0;
 +			width = 0;
  			do {
 -				n = 10 * n + to_digit(ch);
 +				width = add_digit(width, ch);
 +				if (width < 0)
 +					goto overflow;
  				ch = *fmt++;
  			} while (is_digit(ch));
 -			width = n;
  			goto reswitch;
  #ifndef NO_FLOATING_POINT
  		case 'L':
 @@ -601,16 +624,16 @@
  				 * NUL in the first `prec' characters, and
  				 * strlen() will go further.
  				 */
 -				char *p = memchr(cp, 0, (size_t)prec);
 +				for (n = 0; n < prec && cp[n]; n++)
 +					continue;
 +				size = n;
 +			} else {
 +				size_t len = strlen(cp);
  
 -				if (p != NULL) {
 -					size = p - cp;
 -					if (size > prec)
 -						size = prec;
 -				} else
 -					size = prec;
 -			} else
 -				size = strlen(cp);
 +				if (len > INT_MAX)
 +					goto overflow;
 +				size = len;
 +			}
  			sign = '\0';
  			break;
  		case 'U':
 @@ -797,19 +820,29 @@
  			PAD(width - realsz, blanks);
  
  		/* finally, adjust ret */
 -		ret += width > realsz ? width : realsz;
 +		n = width > realsz ? width : realsz;
 +		if (n > INT_MAX - ret)
 +			goto overflow;
 +		ret += n;
  
  		FLUSH();	/* copy out the I/O vectors */
  	}
  done:
  	FLUSH();
  error:
 +	if (__sferror(fp))
 +		ret = -1;
 +	goto finish;
 +
 +overflow:
 +	errno = EOVERFLOW;
 +	ret = -1;
 +
 +finish:
  #ifndef NO_FLOATING_POINT
  	if (dtoaresult)
  		__freedtoa(dtoaresult);
  #endif
 -	if (__sferror(fp))
 -		ret = -1;
  	return (ret);
  }