[v2,1/3] posix: Remove dynamic memory allocation from execl{e,p}

On 02/08/2016 08:28 AM, Rasmus Villemoes wrote:
> On Tue, Feb 02 2016, Rich Felker <dalias@libc.org> wrote:
>
>> On Mon, Feb 01, 2016 at 04:52:15PM +0000, Joseph Myers wrote:
>>> On Mon, 1 Feb 2016, Adhemerval Zanella wrote:
>>>
>>>> +  char *argv[argc+1];
>>>> +  va_start (ap, arg);
>>>> +  argv[0] = (char*) arg;
>>>> +  for (i = 1; i < argc; i++)
>>>> +     argv[i] = va_arg (ap, char *);
>>>> +  argv[i] = NULL;
>>>
>>> I don't see how you're ensuring this stack allocation is safe (i.e. if
>>> it's too big, it doesn't corrupt memory that's in use by other threads).
>>
>> There's no obligation to. If you pass something like a million
>> arguments to a variadic function, the compiler will generate code in
>> the caller that overflows the stack before the callee is even reached.
>> The size of the vla used in execl is exactly the same size as the
>> argument block on the stack used for passing arguments to execl from
>> its caller, and it's nobody's fault but the programmer's if this is
>> way too big. It's not a runtime variable.
>
> This is true, and maybe it's not worth the extra complication, but if
> we're willing to make arch-specific versions of execl and execle we can
> avoid the double stack use and the time spent copying the argv
> array. That won't remove the possible stack overflow, of course, but
> then it'll in all likelihood happen in the user code and not glibc.

I like the idea.  It's a quality of implementation issue, wherein by re-using 
the data that's (mostly) on the stack already we don't need twice again the 
amount of stack space for any given call.

I think that Adhemerval's patch should go in as a default implementation, and 
various targets can implement the assembly as desired.

I've tested the following on x86_64 and i686.  I've compile-tested for x32 (but 
need a more complete x32 installation for testing), and alpha (testing is just 
slow).

Thoughts?


r~

On 09/02/16 11:35, Richard Henderson wrote:
> On 02/08/2016 08:28 AM, Rasmus Villemoes wrote:
>> On Tue, Feb 02 2016, Rich Felker <dalias@libc.org> wrote:
>>
>>> On Mon, Feb 01, 2016 at 04:52:15PM +0000, Joseph Myers wrote:
>>>> On Mon, 1 Feb 2016, Adhemerval Zanella wrote:
>>>>
>>>>> +  char *argv[argc+1];
>>>>> +  va_start (ap, arg);
>>>>> +  argv[0] = (char*) arg;
>>>>> +  for (i = 1; i < argc; i++)
>>>>> +     argv[i] = va_arg (ap, char *);
>>>>> +  argv[i] = NULL;
>>>>
>>>> I don't see how you're ensuring this stack allocation is safe (i.e. if
>>>> it's too big, it doesn't corrupt memory that's in use by other threads).
>>>
>>> There's no obligation to. If you pass something like a million
>>> arguments to a variadic function, the compiler will generate code in
>>> the caller that overflows the stack before the callee is even reached.
>>> The size of the vla used in execl is exactly the same size as the
>>> argument block on the stack used for passing arguments to execl from
>>> its caller, and it's nobody's fault but the programmer's if this is
>>> way too big. It's not a runtime variable.
>>
>> This is true, and maybe it's not worth the extra complication, but if
>> we're willing to make arch-specific versions of execl and execle we can
>> avoid the double stack use and the time spent copying the argv
>> array. That won't remove the possible stack overflow, of course, but
>> then it'll in all likelihood happen in the user code and not glibc.
> 
> I like the idea.  It's a quality of implementation issue, wherein by re-using the data that's (mostly) on the
> stack already we don't need twice again the amount of stack space for any given call.
> 
> I think that Adhemerval's patch should go in as a default implementation, and various targets can implement the
> assembly as desired.
> 
> I've tested the following on x86_64 and i686.  I've compile-tested for x32 (but need a more complete x32
> installation for testing), and alpha (testing is just slow).
> 
> Thoughts?
> 

i think it is a hard to maintain, nasty hack

is execl stack usage the bottleneck somewhere?

to improve the stack usage in glibc, the extreme
wasteful cases should be fixed first.
(crypt uses >100k, strtod uses ???, etc)

e.g. musl guarantees hard limits on libc stack usage
and execl is one of the (two) exceptions where it just
seems useless to do so (you cannot do it portably anyway).

On 09-02-2016 11:30, Szabolcs Nagy wrote:
> On 09/02/16 11:35, Richard Henderson wrote:
>> On 02/08/2016 08:28 AM, Rasmus Villemoes wrote:
>>> On Tue, Feb 02 2016, Rich Felker <dalias@libc.org> wrote:
>>>
>>>> On Mon, Feb 01, 2016 at 04:52:15PM +0000, Joseph Myers wrote:
>>>>> On Mon, 1 Feb 2016, Adhemerval Zanella wrote:
>>>>>
>>>>>> +  char *argv[argc+1];
>>>>>> +  va_start (ap, arg);
>>>>>> +  argv[0] = (char*) arg;
>>>>>> +  for (i = 1; i < argc; i++)
>>>>>> +     argv[i] = va_arg (ap, char *);
>>>>>> +  argv[i] = NULL;
>>>>>
>>>>> I don't see how you're ensuring this stack allocation is safe (i.e. if
>>>>> it's too big, it doesn't corrupt memory that's in use by other threads).
>>>>
>>>> There's no obligation to. If you pass something like a million
>>>> arguments to a variadic function, the compiler will generate code in
>>>> the caller that overflows the stack before the callee is even reached.
>>>> The size of the vla used in execl is exactly the same size as the
>>>> argument block on the stack used for passing arguments to execl from
>>>> its caller, and it's nobody's fault but the programmer's if this is
>>>> way too big. It's not a runtime variable.
>>>
>>> This is true, and maybe it's not worth the extra complication, but if
>>> we're willing to make arch-specific versions of execl and execle we can
>>> avoid the double stack use and the time spent copying the argv
>>> array. That won't remove the possible stack overflow, of course, but
>>> then it'll in all likelihood happen in the user code and not glibc.
>>
>> I like the idea.  It's a quality of implementation issue, wherein by re-using the data that's (mostly) on the
>> stack already we don't need twice again the amount of stack space for any given call.
>>
>> I think that Adhemerval's patch should go in as a default implementation, and various targets can implement the
>> assembly as desired.
>>
>> I've tested the following on x86_64 and i686.  I've compile-tested for x32 (but need a more complete x32
>> installation for testing), and alpha (testing is just slow).
>>
>> Thoughts?
>>
> 
> i think it is a hard to maintain, nasty hack
> 
> is execl stack usage the bottleneck somewhere?
> 
> to improve the stack usage in glibc, the extreme
> wasteful cases should be fixed first.
> (crypt uses >100k, strtod uses ???, etc)
> 
> e.g. musl guarantees hard limits on libc stack usage
> and execl is one of the (two) exceptions where it just
> seems useless to do so (you cannot do it portably anyway).
> 


I agree with Szabolcs and I also see these kind of asm specific
implementation also adds platform different behaviour which I also
think we should avoid to add within GLIBC.

The only doubt with my patch I have now is if it is worth to add
the -fstack-check or not. Florian has raised some questioning about
its efficacy and I am not sure how well it is supported on all 
architectures.