* [musl] stdio/vfprintf.c
@ 2023-12-03 16:28 Morten Welinder
2023-12-03 21:38 ` Rich Felker
0 siblings, 1 reply; 2+ messages in thread
From: Morten Welinder @ 2023-12-03 16:28 UTC (permalink / raw)
To: musl
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.
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [musl] stdio/vfprintf.c
2023-12-03 16:28 [musl] stdio/vfprintf.c Morten Welinder
@ 2023-12-03 21:38 ` Rich Felker
0 siblings, 0 replies; 2+ messages in thread
From: Rich Felker @ 2023-12-03 21:38 UTC (permalink / raw)
To: Morten Welinder; +Cc: musl
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
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2023-12-03 21:38 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-12-03 16:28 [musl] stdio/vfprintf.c Morten Welinder
2023-12-03 21:38 ` Rich Felker
Code repositories for project(s) associated with this public inbox
https://git.vuxu.org/mirror/musl/
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).