[Toybox] [PATCH] count.c: Human readable -h option and MAYFORK

[enh]

On Tue, Oct 17, 2023 at 10:42 PM Rob Landley <rob at landley.net> wrote:
>
> On 10/17/23 16:27, Oliver Webb wrote:
> >> Let me know if I screwed stuff up this time, I've been under the weather the
> >> past few days...
> >
> > I get a -Wformat-overflow warning while compiling this command.
> > Swapping out sprintf with snprintf gives a -Wformat-truncation warning.
>
> What is the actual warning message? If it's the:
>
>     sprintf(toybuf+1024, ", %sb, %sb/s, %um%02us", toybuf+256, toybuf+512,
>       seconds/60, seconds%60);
>
> We have 3k bytes left in the 4k of toybox? Neither of the human_readable() calls
> is going to produce more than a dozen bytes of output and %u maxes out at 4
> billion which is 10 characters. It's 4 arguments for 4 % escapes, argument types
> are correct...
>
> What is it complaining about, exactly?

it has no idea what the maximum length of human_readable() output could be.

> (And what the heck is -Wformat-truncation with snprintf? Isn't that ALLOWED to
> cut the output short? What is it warning _about_?)

as long as you check the result, i don't think it cares what you do
--- you get the warning when (a) you don't check and (b) it can't
prove there's no overflow.

> > $ toybox timeout 1 /bin/yes | ./count -l > /dev/null
> > 1243979776 bytes, 1.1Gb, 237Mb/s, 0m01s
> >
> > In 1 second it copies 1.1Gb of data at 237Mb per second?
>
> My test was:
>
>   (for i in $(seq 1 200); do cat README; sleep .1; done; sleep 20) | count -l
>
> Which alas takes way too long to run for me to put in the test suite but DOES
> insulate the test from obvious variations in the host system.
>
> I've reproduced your test here, albeit 1.2Mb at 260Kb/s, but it's off by the
> same ratio.
>
> There are theoretical rounding errors with small input times (boundary condition
> where it averages in an extra zero period at the end it didn't get to write any
> data into because it advanced into a new 1/4 second but never read anything
> during it), but that's not gonna explain 4x? Hmmm...
>
> > Adding a "*4" in the human_readable call turns 237(-ish)Mb/s into 909(-ish)Mb/s,
> > Which is still inaccurate.
>
> Nope, gotta root cause it. If you don't know what went wrong, multiplying the
> observed result by a constant to try to approximate the result you expected is
> seldom helpful. I want to know WHY the behavior was observed. Causing the
> problem to go away without understanding it makes the situation WORSE.
>
> > The below patch shuts the compiler up, but doesn't fix the inaccurate rates
>
> No. The compiler is wrong here. I know how big the strings I fed into it are, I
> will -Wno-that-warning in scripts/portability.sh alongside the others if it
> can't sanely produce a helpful warning here.
>
> This is "warning: you ever used fgets() at all, I don't care you think you know
> what you're doing" level nonsense. When I'm piping data to myself I _can_ know
> it will fit. I refuse to defensively program against a broken compiler.
> (Especially since this is _exactly_ the sort of thing ASAN is supposedly good at.)

no, because -- as you said earlier -- snprintf() will truncate. it's
asking for proof that that either can't happen, or that you're going
to do something sensible if it does.

> Rob
> _______________________________________________
> Toybox mailing list
> Toybox at lists.landley.net
> http://lists.landley.net/listinfo.cgi/toybox-landley.net