[PATCH] [RFC] fix integer overflows and uncaught EOVERFLOW in printf

[Rich Felker <dalias@libc.org>]

[-- Attachment #1: Type: text/plain, Size: 1308 bytes --]

This patch expands on issues raised by Alexander Cherepanov earlier
this year with some concrete fixes. I'm not terribly concerned about
what happens when the format string itself contains out-of-range
numbers (although it would be nice to handle that right), but it is
important to handle correctly cases where the data being printed (e.g.
long strings) would cause the total length to overflow INT_MAX, or
where it would cause internal overflows.

This patch:

1. Returns with an error as soon as EOVERFLOW conditions are detected,
   rather than continuing to produce output.

2. Catches overflows where the format string itself is longer than
   INT_MAX (only possible on 64-bit archs).

3. Catches %s argument strings longer than INT_MAX (likewise).

4. Catches cases where adding a prefix length to a precision would
   overflow.

5. Correctly handles width specifiers that overflow (always an error)
   and precision specifiers that overflow (error for everything but
   strings; for strings they're equivalent to no precision/no limit).

Checking wide strings and floating point conversions for overflows
remains to be done.

As discussed before, none of these overflows are presently dangerous,
but they do lead to mildly wrong results in the relevant corner cases
and really should be fixed.

Rich

[-- Attachment #2: printf-int-overflows.diff --]
[-- Type: text/plain, Size: 4335 bytes --]

diff --git a/src/stdio/vfprintf.c b/src/stdio/vfprintf.c
index e439a07..64e87e2 100644
--- a/src/stdio/vfprintf.c
+++ b/src/stdio/vfprintf.c
@@ -436,9 +436,11 @@ static int fmt_fp(FILE *f, long double y, int w, int p, int fl, int t)
 }
 
 static int getint(char **s) {
-	int i;
-	for (i=0; isdigit(**s); (*s)++)
-		i = 10*i + (**s-'0');
+	unsigned i;
+	for (i=0; isdigit(**s); (*s)++) {
+		if (i > INT_MAX/10 || **s-'0' > INT_MAX-10*i) i = -1;
+		else i = 10*i + (**s-'0');
+	}
 	return i;
 }
 
@@ -446,7 +448,7 @@ static int printf_core(FILE *f, const char *fmt, va_list *ap, union arg *nl_arg,
 {
 	char *a, *z, *s=(char *)fmt;
 	unsigned l10n=0, fl;
-	int w, p;
+	int w, p, xp;
 	union arg arg;
 	int argpos;
 	unsigned st, ps;
@@ -459,18 +461,19 @@ static int printf_core(FILE *f, const char *fmt, va_list *ap, union arg *nl_arg,
 	char mb[4];
 
 	for (;;) {
+		/* This error is only specified for snprintf, but since it's
+		 * unspecified for other forms, do the same. Stop immediately
+		 * on overflow; otherwise %n could produce wrong results. */
+		if (l > INT_MAX - cnt) goto overflow;
+
 		/* Update output count, end loop when fmt is exhausted */
-		if (cnt >= 0) {
-			if (l > INT_MAX - cnt) {
-				errno = EOVERFLOW;
-				cnt = -1;
-			} else cnt += l;
-		}
+		cnt += l;
 		if (!*s) break;
 
 		/* Handle literal text and %% format specifiers */
 		for (a=s; *s && *s!='%'; s++);
 		for (z=s; s[0]=='%' && s[1]=='%'; z++, s+=2);
+		if (z-a > INT_MAX-cnt) goto overflow;
 		l = z-a;
 		if (f) out(f, a, l);
 		if (l) continue;
@@ -498,9 +501,9 @@ static int printf_core(FILE *f, const char *fmt, va_list *ap, union arg *nl_arg,
 			} else if (!l10n) {
 				w = f ? va_arg(*ap, int) : 0;
 				s++;
-			} else return -1;
+			} else goto inval;
 			if (w<0) fl|=LEFT_ADJ, w=-w;
-		} else if ((w=getint(&s))<0) return -1;
+		} else if ((w=getint(&s))<0) goto overflow;
 
 		/* Read precision */
 		if (*s=='.' && s[1]=='*') {
@@ -511,24 +514,29 @@ static int printf_core(FILE *f, const char *fmt, va_list *ap, union arg *nl_arg,
 			} else if (!l10n) {
 				p = f ? va_arg(*ap, int) : 0;
 				s+=2;
-			} else return -1;
+			} else goto inval;
+			xp = (p>=0);
 		} else if (*s=='.') {
 			s++;
 			p = getint(&s);
-		} else p = -1;
+			xp = 1;
+		} else {
+			p = -1;
+			xp = 0;
+		}
 
 		/* Format specifier state machine */
 		st=0;
 		do {
-			if (OOB(*s)) return -1;
+			if (OOB(*s)) goto inval;
 			ps=st;
 			st=states[st]S(*s++);
 		} while (st-1<STOP);
-		if (!st) return -1;
+		if (!st) goto inval;
 
 		/* Check validity of argument type (nl/normal) */
 		if (st==NOARG) {
-			if (argpos>=0) return -1;
+			if (argpos>=0) goto inval;
 		} else {
 			if (argpos>=0) nl_type[argpos]=st, arg=nl_arg[argpos];
 			else if (f) pop_arg(&arg, st, ap);
@@ -584,6 +592,7 @@ static int printf_core(FILE *f, const char *fmt, va_list *ap, union arg *nl_arg,
 		case 'u':
 			a = fmt_u(arg.i, z);
 			}
+			if (xp && p<0) goto overflow;
 			if (p>=0) fl &= ~ZERO_PAD;
 			if (!arg.i && !p) {
 				a=z;
@@ -599,9 +608,9 @@ static int printf_core(FILE *f, const char *fmt, va_list *ap, union arg *nl_arg,
 			if (1) a = strerror(errno); else
 		case 's':
 			a = arg.p ? arg.p : "(null)";
-			z = memchr(a, 0, p);
-			if (!z) z=a+p;
-			else p=z-a;
+			z = a + strnlen(a, p<0 ? INT_MAX : p);
+			if (p<0 && *z) goto overflow;
+			p = z-a;
 			fl &= ~ZERO_PAD;
 			break;
 		case 'C':
@@ -623,12 +632,15 @@ static int printf_core(FILE *f, const char *fmt, va_list *ap, union arg *nl_arg,
 			continue;
 		case 'e': case 'f': case 'g': case 'a':
 		case 'E': case 'F': case 'G': case 'A':
+			if (xp && p<0) goto overflow;
 			l = fmt_fp(f, arg.f, w, p, fl, t);
 			continue;
 		}
 
 		if (p < z-a) p = z-a;
+		if (p > INT_MAX-pl) goto overflow;
 		if (w < pl+p) w = pl+p;
+		if (w > INT_MAX-cnt) goto overflow;
 
 		pad(f, ' ', w, pl+p, fl);
 		out(f, prefix, pl);
@@ -646,8 +658,15 @@ static int printf_core(FILE *f, const char *fmt, va_list *ap, union arg *nl_arg,
 	for (i=1; i<=NL_ARGMAX && nl_type[i]; i++)
 		pop_arg(nl_arg+i, nl_type[i], ap);
 	for (; i<=NL_ARGMAX && !nl_type[i]; i++);
-	if (i<=NL_ARGMAX) return -1;
+	if (i<=NL_ARGMAX) goto inval;
 	return 1;
+
+inval:
+	errno = EINVAL;
+	return -1;
+overflow:
+	errno = EOVERFLOW;
+	return -1;
 }
 
 int vfprintf(FILE *restrict f, const char *restrict fmt, va_list ap)