[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date: Sun, 3 Dec 2023 16:38:55 -0500
From: Rich Felker <dalias@...c.org>
To: Morten Welinder <mwelinder@...il.com>
Cc: musl@...ts.openwall.com
Subject: Re: stdio/vfprintf.c
On Sun, Dec 03, 2023 at 11:28:43AM -0500, Morten Welinder wrote:
> Looking at https://git.musl-libc.org/cgit/musl/tree/src/stdio/vfprintf.c
> I see a few issues:
>
>
> 1. If "i=-1" in getint on line 424 is reached and there are more
> digits then the next overflow check will itself overflow in
> "INT_MAX-10*i"
No, the evaluation of INT_MAX-10*i is not reached because i >
INT_MAX/10U is true.
> 2. The getint call on line 504 doesn't check for overflow. If it did,
> getint could just return -1 right away on overflow.
I'm unclear what you think it should check. Precisions too large to be
representable become -1 (no limit), as intended. They will not be
exceeded because attempting to output more than INT_MAX bytes is
already an error which will be caught when reached, before the (lost)
gigantic precision is exceeded.
> 3. The "w=-w;" on line 488 doesn't check for overflow which will
> happen for INT_MIN.
This is probably a bug that should be handled with an explicit check
for INT_MIN and goto overflow.
> 4. The length calculation for "%s" on line 600 implies that strings
> longer than 2G cannot be printed. It looks deliberate, but is it
> reasonable?
It's an unfortunate contract of the printf family. The return value
must be the number of bytes produced, which is not satisfiable if more
than INT_MAX would be produced. snprintf is explicit that this is a
reportable error, but for the rest, this is probably really undefined
behavior.
> 5. And speaking of plain "%s" with no width or precision, why is the
> string length even calculated first? Walking the string twice seems
> inefficient.
Because the code is written as one common path for everything rather
than two special cases for "%s with no width or precision" and "%s
with width or precision".
> 6. This comment and check seems out of date:
> /* 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;
>
> Since %n allows size modifiers it can already produce wrong results.
> Right right place to check would be at %n handling.
I think you're misreading what "wrong results" means. %n is specified
to store the number into an object of a particular type according to
the modifiers prefixes; this may be a truncated result for %hn or
%hhn, but it's not a wrong result. It's the correct result as
specified.
The issue that comment is dealing with is that, in the absence of %n,
it would be perfectly fine for snprintf to "keep going on overflow",
discarding output, as long as the final return value reflects the
overflow error. However, with %n, this "keep going" can have side
effects that are not permitted: an object whose value should not have
been modified will have had a new value stored into it.
Rich
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.
