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);
}