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/11/2007 11:45: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: Thu, 11 Jan 2007 12:48:21 +0100

 --45Z9DzgjV8m4Oswq
 Content-Type: text/plain; charset=us-ascii
 Content-Disposition: inline
 
 
 Consider the attached patch as a suggestion. I have not tested, not even compiled it yet.
 I think the other patch could cause a memory leak due to not calling __freedtoa() on
 overflow. My patch also catches another issue which was mentioned in fefe's blog:
 
 	width = -width;
 
 This is seeminly meant to ensure that width is positive but this won't help if width
 is INT_MIN and -INT_MAX != INT_MIN. I suspect the latter is always the case but I
 added a check anyways.
 
 Maybe there should be another overflow check for the %hn case. I don't know why there's
 a XXX after the ssize_t case in the code.
 
 -- 
 Christian
 
 --45Z9DzgjV8m4Oswq
 Content-Type: text/plain; charset=us-ascii
 Content-Disposition: attachment; filename="vfprintf.c.udif"
 
 --- vfprintf.c.bak	2007-01-11 11:22:13.000000000 +0100
 +++ vfprintf.c	2007-01-11 12:40:19.000000000 +0100
 @@ -202,7 +202,7 @@ __vfprintf_unlocked(fp, fmt0, ap)
  {
  	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 +350,10 @@ __vfprintf_unlocked(fp, fmt0, ap)
  				}
  			}
  		}
 -		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 +390,8 @@ reswitch:	switch (ch) {
  			 */
  			if ((width = va_arg(ap, int)) >= 0)
  				goto rflag;
 +			if (-INT_MAX != INT_MIN && width <= INT_MIN)
 +				goto overflow;
  			width = -width;
  			/* FALLTHROUGH */
  		case '-':
 @@ -402,11 +407,19 @@ reswitch:	switch (ch) {
  				goto rflag;
  			}
  			n = 0;
 +			while ('0' == ch) {
 +				ch = *fmt++;
 +			}
  			while (is_digit(ch)) {
 -				n = 10 * n + to_digit(ch);
 +				if (n > INT_MAX / 10)
 +					goto overflow;
 +				n *= 10;
 +				if (to_digit(ch) > INT_MAX - n)
 +					goto overflow;
 +				n += to_digit(ch);
  				ch = *fmt++;
  			}
 -			prec = n < 0 ? -1 : n;
 +			prec = n;
  			goto reswitch;
  		case '0':
  			/*
 @@ -420,7 +433,12 @@ reswitch:	switch (ch) {
  		case '5': case '6': case '7': case '8': case '9':
  			n = 0;
  			do {
 -				n = 10 * n + to_digit(ch);
 +				if (n > INT_MAX / 10)
 +					goto overflow;
 +				n *= 10;
 +				if (to_digit(ch) > INT_MAX - n)
 +					goto overflow;
 +				n += to_digit(ch);
  				ch = *fmt++;
  			} while (is_digit(ch));
  			width = n;
 @@ -797,19 +815,28 @@ number:			if ((dprec = prec) >= 0)
  			PAD(width - realsz, blanks);
  
  		/* finally, adjust ret */
 -		ret += width > realsz ? width : realsz;
 +		if (MAX(width, realsz) > INT_MAX - ret)
 +			goto overflow;
 +		ret += MAX(width, realsz);
  
  		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);
  }
  
 
 --45Z9DzgjV8m4Oswq--