[Toybox] [PATCH] xargs: avoid "Argument list too long".

[enh]

i haven't tested my other cases, but your patch doesn't address the -0 issues:

/tmp/toybox$ ./toybox find / -print0 2>/dev/null | ./toybox xargs -0 > /dev/nul
l ; echo $?
xargs: exec echo: Argument list too long
126
/tmp/toybox$

On Thu, Oct 24, 2019 at 5:50 PM enh <enh at google.com> wrote:
>
> On Thu, Oct 24, 2019 at 5:27 PM Rob Landley <rob at landley.net> wrote:
> >
> > On 10/22/19 3:31 PM, enh via Toybox wrote:
> > > okay, i've also sent out the alternative patch (search your inbox for
> > > "proper")
> >
> > I'm looking, I'm trying to remember what failure it was trying to fix. (When I
> > have a test, coming up with a fix for it is generally easy. When I have a fix
> > without a test I have to try to figure out what _might_ be going wrong which
> > takes forever and is error prone.)
> >
> > Ah, there's a test in the patch. Ok, what's going on here...
> >
> > We're currently doing:
> >
> >   // POSIX requires that we never hit the ARG_MAX limit, even if we try to
> >   // with -s. POSIX also says we have to reserve 2048 bytes "to guarantee
> >   // that the invoked utility has room to modify its environment variables
> >   // and command line arguments and still be able to invoke another utility",
> >   // though obviously that's not really something you can guarantee.
> >   bytes = sysconf(_SC_ARG_MAX) - environ_bytes() - 2048;
> >   if (!TT.s || TT.s > bytes) TT.s = bytes;
> >
> > That sysconf is, in theory, 128k. (We had that argument on the kernel list with
> > Linus, I think I yanked Rich Felker into it too. It _should_ have an #if/else
> > test on kernel version and currently say 10 megs but I'd rather not do that in
> > toybox). Meaning if you don't specify a -s, you get 128k, minus however much
> > environment space you've currently used, minus 2048. (Which could easily make it
> > negative? I have no idea why we're doing that.)
> >
> > The actual limit on current kernels is 10 megabytes. The limit on kernels before
> > that (going back to 2007) was "unlimited", so 10 megabytes works for them too.
> > The pathological case is single character arguments, each of which takes 10
> > bytes on a 64 bit system (char+null+sizeof(pointer)), so 1 million of those
> > would fill your 10 megs, so you'd want your default -s value to be at most 1
> > megabyte if you've removed the sizeof(pointer) accounting.
> >
> > If sysconf(_SC_ARG_MAX) is returning 128k, that's comfortably under 1 megabyte
> > and all modern kernels should be able to handle it?
> >
> > (Goes to check what the sysconf is returning...)
> >
> > And glibc raised it to 2 megabytes. Of course they did. But that still shouldn't
> > cause a failure on find /usr unless the average filename length returned is
> > tiny? (Average file length of 9 would double to 4 megabytes, and we'd still have
> > 6 left?)
> >
> > Aha! The kernel changed behavior again. (4 seperate sets of semantics, why not?)
> > They're enforcing exec size at 1/4 ulimit -s, so the amount glibc is returning
> > is probed and accurate... modulo the 10 meg limit on top of that. :P
> >
> > So we do need to account for all the bytes.)
> >
> > Ok, new rule: if FLAG(s) isn't set, we accurately count the bytes. And when it
> > is, it's #%*(#&% pilot error. (And I'll let the silly posix -2048 cover the
> > sizeof(char *) for the trailing NULL on argv[] because adding it to the
> > initialization would drive the for loop over 80 chars).
>
> (please see the patch with "proper" in the title. it does this, only
> it's a bit trickier than it seems, and you definitely can hit those
> edge cases unless you increase the 2048. oh, and -0 is also not quite
> right. the "proper" patch fixes all of the combinations for me.)
>
> > > if you want to compare. i don't personally have a strong
> > > opinion between the two. macOS seems to inherit BSD's "proper"
> > > implementation, and findutils seems to just assume the 128KiB limit.
> >
> > I don't want to check in a test with external dependencies like that, which
> > basically eliminate reproducibility. (I don't even use toybox source files for
> > this because version skew.)
> >
> > If you want an arbitrarily long run of characters here's a trick:
> >
> >   head -c 100000 /dev/zero | tr '\0' x
>
> yeah, now i'm not thinking solely in terms of find(1), seq(1) would
> probably be a good source of input at least for the non `-0` case.
>
> > > a few new things i learned though:
> > >
> > > * the 128KiB limit is actually motivated somewhat by being the
> > > kernel's minimum limit; even if you set the stack so that you'd get a
> > > smaller limit, you still get 128KiB (strictly, 32 pages, but all my
> > > hardware has 4KiB pages, so...).
> >
> > The behavior before 2.6.22 was hardwired 128k, lowering it would have been a
> > compatibility break.
> >
> > > * the findutils implementation is a bit cleverer than i realized. if
> > > you dick about with strace and `xargs -s`, you'll see that if you run
> > > it over the kernel limit it catches the E2BIG and retries with fewer
> > > arguments!
> >
> > I explicitly asked Linus if there was a better way to do it than that, and he
> > said no. Dealing with this area gives me a headache now.
> >
> > > * the kernel also has separate limits on the maximum length of any
> > > individual string,
> >
> > last I checked it was 128k.
> >
> > > and on the count of strings (separate from the
> > > space taken by those strings, or their pointers).
> >
> > last I checked it was 10 megs.
>
> (kind of irrelevant, but, no, i'm talking about an independent limit
> of the *number* of strings, not the space taken by them.)
>
> > > but i'm assuming YAGNI.
> >
> > You can't query either limit except by checking your kernel version, and they've
> > changed before. I'm calling it libc's responsibility to feed me a sane
> > sysconf(_SC_ARG_MAX), which means libc should have those boundary checks after
> > the getrlimit().
> >
> > > we can add those limits if anyone ever manages to hit them in
> > > real life. (i don't think there's anything we can do in the former
> > > case anyway, so the default E2BIG error message isn't terrible.)
> >
> > I asked the kernel guys and they confirmed it's Underpants Gnomes Step 2. They
> > did the smile and everything. Since then libc updated to give the _actual_ value
> > requiring us to do the accounting. My response is "wheee", with the slow dour
> > single finger twirl.
> >
> > > i do wonder who uses -s in practice, and whether they really should be
> > > doing so. -n, sure, that totally makes sense. but -s seems like a
> > > mistake to me.
> >
> > You get to keep the pieces.
> >
> > > (note that unlike the 128KiB hack, i haven't tested this patch in the
> > > actual mac SDK or kernel build scenarios that have been failing. let
> > > me know if you'd rather go this way, and i'll explicitly test those
> > > cases too. but this patch doesn't break existing tests and does fix my
> > > /usr proxy test.)
> >
> > I'm trying to add a test suite entry now and...
> >
> > $ head -c $((131071)) /dev/zero | tr '\0' x | ./xargs | wc
> >       1       1  131072
> > $ head -c $((131071)) /dev/zero | tr '\0' x | xargs | wc
> > xargs: argument line too long
> >       0       0       0
> >
> > I hate the FSF and everything it stands for. Andrew Fluegelman coined the term
> > "Freeware" a year before the GNU manifesto shipped, and that's only because
> > calling it "public domain software" was going out of fashion as proprietary FUD
> > scaled up. (I'd tweet about it if twitter hadn't deleted my account.)
> >
> > The kernel can handle arguments of size 131072 minus the null terminator, but
> > the biggest one fileutils xargs can handle is 131066 because they're doing the
> > math wrong. If I want it to reliably pass TEST_HOST, I need to use the broken
> > value. Wheee.
> >
> > Let's see...
> >
> > $ X=$(head -c 131066 /dev/zero | tr '\0' x)
> > $ for i in $(seq 1 200); do echo $X; done | xargs | wc
> >     200     200 26213400
> > $ for i in $(seq 1 200); do echo $X; done | ./toybox xargs | wc
> >      14     200 26213400
> >
> > Sigh, what does success look like here... I guess "didn't error out" not any
> > specific number of lines?
> >
> > Rob