Re: Fun with big widths and precisions in printf

[Rich Felker <dalias@libc.org>]

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

On Fri, Jan 08, 2016 at 01:28:50AM +0300, Alexander Cherepanov wrote:
> Hi!
> 
> Some fun with big values for field widths and precisions in *printf
> functions.

I've finally got around to looking at these in more detail and I have
some ideas which I'm including as an attached patch.

> 1. POSIX requires *printf functions to set errno in case of errors.
> It's not always done in musl.

This is easily fixed.

> 2. Field width[1] and precision[2] are positive numbers and are
> extracted from the format string as int by the getint function[3].
> It doesn't check for overflows while neither C11 nor POSIX set any

It's easy to make it check for overflow and return -1 which cannot
happen on valid input (since it only parses non-negative number
strings).

> limits for these numbers. Overflows in getint are bad as it's UB
> itself plus it gives wrong results -- either negative numbers or
> positive but smaller than original. Both cases lead to wrong
> behavior in printf.
> 
> Negative widths are caught while positive overflown values are still
> a problem. For precisions both cases are a problem.

Yes. For width we can just fail the operation with EOVERFLOW when
getint returns a negative value. And overflows in precision are not
consistently treated as "unlimited"/"default", which yields the right
behavior for %s, %g, and misc stuff, but not for %[diouxXeEfF] where
EOVERFLOW "should" occur. I think we could simply put a sentinel value
in p and check at the right points for those formats and throw an
EOVERFLOW error, but it's mildly ugly so I might look to do that as a
second patch for this (non-serious, rather trivial) issue.

> From C11 POV most of these problems cannot occur. E.g., a field with
> width greater than INT_MAX leads to a return value greater than
> INT_MAX. printf cannot return such values because it has int as its
> return type. Thus, this is UB in user code and implementations don't
> have to care about it. But there is one case where this is still a
> problem -- overflown positive precision for %s:
> 
>   printf("%.4294967296s", "abc");
> 
> In musl it prints empty string while having to print "abc".

Agreed. This is fixed by the attached patch.

> OTOH POSIX requires *printf functions to fail with errno=EOVERFLOW
> if the value to be returned is greater than INT_MAX. This makes the
> problem wider.
> 
> [1] http://git.musl-libc.org/cgit/musl/tree/src/stdio/vfprintf.c#n491
> [2] http://git.musl-libc.org/cgit/musl/tree/src/stdio/vfprintf.c#n505
> [3] http://git.musl-libc.org/cgit/musl/tree/src/stdio/vfprintf.c#n438
> 
> 3. When width is an asterisk and its value is taken from an
> argument, the value could initially be negative. Processing of this
> case[4] overflows on INT_MIN with the effect similar to the previous
> case (UB plus wrong result).

This is easy to fix and is handled in the patch.

One thing I noticed while reading the code to fix it is that,
currently, printf does not stop on hitting EOVERFLOW but keeps
producing output. I don't remember my motivation for doing it that
way, and noticed that the %n (even %jn!) output is wrong past the
point of overflow, making this behavior rather undesirable.

Also the output is not consistent due to further overflow (actually
conversion-to-signed-int) issues: printing a string longer than
INT_MAX with %s will lead to nonsense values from p=z-a, w=pl+p, and
l=w, which totally break the output character counting. (Note: this
has no safety impact on snprintf, whose buffer space counting has
nothing to do with vfprintf.c's; that decoupling was intentional.)
Even if we capped z-a to INT_MAX though, we'd end up producing wrong
output.

If we don't care about producing output on EOVERFLOW, the easy fix is
just bailing out immediately on overflow. Alternatively the current
apparent intent (which is broken) could be fixed by using 64-bit
counts internally for everything and just producing an overflow error
at the end if the count exceeded INT_MAX. I think failing early is
preferable, but I've left this issue alone for the purpose of this
first patch.

Rich

[-- Attachment #2: printf_int_overflows.diff --]
[-- Type: text/plain, Size: 2192 bytes --]

diff --git a/src/stdio/vfprintf.c b/src/stdio/vfprintf.c
index 2ecf769..c123ade 100644
--- a/src/stdio/vfprintf.c
+++ b/src/stdio/vfprintf.c
@@ -437,8 +437,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)++)
+	for (i=0; isdigit(**s); (*s)++) {
+		if (i > INT_MAX/10 || **s-'0' > INT_MAX-10*i)
+			return -1;
 		i = 10*i + (**s-'0');
+	}
 	return i;
 }
 
@@ -498,9 +501,10 @@ 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==INT_MIN) goto overflow;
 			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,7 +515,7 @@ 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;
 		} else if (*s=='.') {
 			s++;
 			p = getint(&s);
@@ -520,15 +524,15 @@ static int printf_core(FILE *f, const char *fmt, va_list *ap, union arg *nl_arg,
 		/* 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);
@@ -646,8 +650,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)