[musl] stdio/vfprintf.c

[musl] stdio/vfprintf.c

[Morten Welinder]

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"

2. The getint call on line 504 doesn't check for overflow.  If it did,
getint could just return -1 right away on overflow.

3. The "w=-w;" on line 488 doesn't check for overflow which will
happen for INT_MIN.

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?

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.

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.


M.

Re: [musl] stdio/vfprintf.c

[Rich Felker]

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