From mboxrd@z Thu Jan 1 00:00:00 1970 X-Msuck: nntp://news.gmane.org/gmane.linux.lib.musl.general/9518 Path: news.gmane.org!not-for-mail From: Rich Felker Newsgroups: gmane.linux.lib.musl.general Subject: Re: Fun with big widths and precisions in printf Date: Sun, 6 Mar 2016 22:46:15 -0500 Message-ID: <20160307034615.GY9349@brightrain.aerifal.cx> References: <568EE6A2.50704@openwall.com> Reply-To: musl@lists.openwall.com NNTP-Posting-Host: plane.gmane.org Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="kORqDWCi7qDJ0mEj" X-Trace: ger.gmane.org 1457322393 12874 80.91.229.3 (7 Mar 2016 03:46:33 GMT) X-Complaints-To: usenet@ger.gmane.org NNTP-Posting-Date: Mon, 7 Mar 2016 03:46:33 +0000 (UTC) To: musl@lists.openwall.com Original-X-From: musl-return-9531-gllmg-musl=m.gmane.org@lists.openwall.com Mon Mar 07 04:46:33 2016 Return-path: Envelope-to: gllmg-musl@m.gmane.org Original-Received: from mother.openwall.net ([195.42.179.200]) by plane.gmane.org with smtp (Exim 4.69) (envelope-from ) id 1acm7r-0000MF-Sd for gllmg-musl@m.gmane.org; Mon, 07 Mar 2016 04:46:32 +0100 Original-Received: (qmail 23975 invoked by uid 550); 7 Mar 2016 03:46:28 -0000 Mailing-List: contact musl-help@lists.openwall.com; run by ezmlm Precedence: bulk List-Post: List-Help: List-Unsubscribe: List-Subscribe: List-ID: Original-Received: (qmail 23951 invoked from network); 7 Mar 2016 03:46:28 -0000 Content-Disposition: inline In-Reply-To: <568EE6A2.50704@openwall.com> User-Agent: Mutt/1.5.21 (2010-09-15) Original-Sender: Rich Felker Xref: news.gmane.org gmane.linux.lib.musl.general:9518 Archived-At: --kORqDWCi7qDJ0mEj Content-Type: text/plain; charset=us-ascii Content-Disposition: inline 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 --kORqDWCi7qDJ0mEj Content-Type: text/plain; charset=us-ascii Content-Disposition: attachment; filename="printf_int_overflows.diff" 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=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) --kORqDWCi7qDJ0mEj--