Date: Sun, 6 Mar 2016 22:46:15 -0500 From: Rich Felker <dalias@...c.org> To: musl@...ts.openwall.com Subject: Re: Fun with big widths and precisions in printf 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 and precision are positive numbers and are > extracted from the format string as int by the getint function. > 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. > >  http://git.musl-libc.org/cgit/musl/tree/src/stdio/vfprintf.c#n491 >  http://git.musl-libc.org/cgit/musl/tree/src/stdio/vfprintf.c#n505 >  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 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 View attachment "printf_int_overflows.diff" of type "text/plain" (2193 bytes)
Powered by blists - more mailing lists
Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.