[Toybox] [landley/toybox] xargs: exec ... Argument list too long (#40)

[enh]

On Sun, Oct 1, 2017 at 3:31 PM, Rob Landley <rob at landley.net> wrote:
> On 09/28/2017 10:18 AM, enh wrote:
>> Did you look at my xargs patch in this thread?
>
> Not closely enough. (Description implied it was the same as last time.)
>
>> It handles all that. I think the find patch is pretty similar, I just
>> haven't had time to set up a working test case. (Or rather I fell into
>> ratholes while doing so.)
>
> I think this year has been one big rathole for me. :)
>
> Ok, first I should have invoked my "I took too long, apply the submitted
> patch as-is, clean it up later" rule a week ago. (It conflicted with
> stuff in lib.h and lib.c I was doing, but I've reverted that and can
> re-apply after.)
>
> Done and pushed, BUT: I think it's still the wrong fix. I think my
> fundamental objection is:
>
>   $ seq 1 200000 | tr '\n' x | xargs
>   xargs: argument line too long
>   $ echo $(seq 1 200000 | tr '\n' x) | wc
>       1       1 1288896
>
> Why does the first _not_ work when the second can? (I note that's the
> Ubuntu xargs failing; I think toybox should do better than that.)
>
> You're calling _SC_ARG_MAX which at least on musl returns hardwired
> 131072, when it should return 1/4 of the stack size limit. (cc'd Rich to
> see what he thinks. See the URLs at the end for earlier discussion of
> digging into the kernel, the real limit the kernel enforces these days
> is 1/4 the stack size ulimit, which defaults to 8388608 on ubuntu 14.04,
> 1/4 of which is 2 megabytes. I dunno if that's because this netbook has
> 8 gigs though? It might scale, haven't checked...) Fixing libc to probe
> the real size is an acceptable fix, otherwise toybox should do it.

this seems to be a clear case of a musl bug, though, not something
toybox should be trying to reimplement itself.

> The guaranteed 2k space left over makes sense, although the "can a
> single file still advance" issue should be able to cut into that as
> necessary. Which would argue for it being PATH_MAX (I.E. the historical
> 4k) space reserved. But reality can still blow through that, filenames
> go arbitrarily deep (or non-filename arguments, as in the above
> example). So 2k's no sillier than anything else. :)
>
> And tests/xargs.test needs size tests making sure we calculate the
> corner cases exactly. "printf '%0*d' $LEN 0" is probably useful here.
> And yes, it works in toybox printf. :)
>
> Rob
>
> P.S. One more FUN corner case is that ulimit -s is a soft limit, the
> hard limit is unlimited. So a normal user can ulimit -s 999999999 and
> it's accepted. So xargs _could_ finagle this itself if it wanted, at
> least for the "one big argument that doesn't fit by itself" case,
> although here there probably be dragons. xargs -f anyone? :)
>
>> On Sep 28, 2017 04:41, "Rob Landley" <rob at landley.net
>> <mailto:rob at landley.net>> wrote:
>>
>>     Sorry, yes I did get time to look at this, I just haven't _fixed_ it
>>     yet.
>>
>>     The problem is that environment variables and arguments both fit in the
>>     same space, which is dynamic at 1/4 the stack size limit as mentioned at
>>     http://lists.landley.net/pipermail/toybox-landley.net/2016-August/008592.html
>>     <http://lists.landley.net/pipermail/toybox-landley.net/2016-August/008592.html>
>>     last time around.
>>
>>     Which means that _any_ static size we enforce isn't guaranteed to work,
>>     if you have a lot of environment variables defined (or just one really
>>     big one). Environment varaibles can even consume the space to the point
>>     you can't launch a _single_ argument. (If it's less than PATH_MAX, we
>>     have a problem.)
>>
>>     So I think it has to probe, and be dynamic, and at least not _loop_ when
>>     find produces a single long name that's more than the remaining
>>     environment space to launch a child. (I.E. test the corner case that the
>>     maximum number of args we can fit is zero.)
>>
>>     Space consumption should be exactly sizeof(char *)*(argc+1) +
>>     sizeof(char *)+(envc+1) + the strlen+1 of all the argv[] + the strlen+1
>>     of all the envp[]. I need to test that's still the case (it used to be
>>     when the limit was hardwired 32*4096).
>>
>>     Sorry for the delay, still juggling way more things than I have energy
>>     for. :(
>>
>>     Rob
>>



-- 
Elliott Hughes - http://who/enh - http://jessies.org/~enh/
Android native code/tools questions? Mail me/drop by/add me as a reviewer.