Re: [musl] [PATCH] clone: Return EINVAL for null stack

[Tudor Cretu <tudor.cretu@arm.com>]

On 02-08-2022 19:53, Rich Felker wrote:
> On Tue, Aug 02, 2022 at 12:30:36PM +0100, Tudor Cretu wrote:
>> This change aligns the clone wrapper with the man page. If the stack is
>> null, clone sets errno to EINVAL, instead of throwing a segmentation fault.
>> ---
>>   src/linux/clone.c | 5 +++++
>>   1 file changed, 5 insertions(+)
>>
>> diff --git a/src/linux/clone.c b/src/linux/clone.c
>> index 8c1af7d3..43a6803b 100644
>> --- a/src/linux/clone.c
>> +++ b/src/linux/clone.c
>> @@ -1,4 +1,5 @@
>>   #define _GNU_SOURCE
>> +#include <errno.h>
>>   #include <stdarg.h>
>>   #include <unistd.h>
>>   #include <sched.h>
>> @@ -11,6 +12,10 @@ int clone(int (*func)(void *), void *stack, int flags, void *arg, ...)
>>   	pid_t *ptid, *ctid;
>>   	void  *tls;
>>   
>> +	if (!stack) {
>> +		return __syscall_ret(-EINVAL);
>> +	}
>> +
>>   	va_start(ap, arg);
>>   	ptid = va_arg(ap, pid_t *);
>>   	tls  = va_arg(ap, void *);
>> -- 
>> 2.25.1
> 
> This is probably okay, but there's also a bigger discussion to be had
> here about what to do about clone() -- deciding what the contract is
> for what usage can be supported, and possibly making the rest produce
> errors like the above. There's also a matter of the current very-wrong
> use of va_arg for variadic arguments that might not exist, and which
> probably *can't* exist in any valid application usage. This came up
> before as part of the mt-fork work, but was basically deferred
> indefinitely...
> 
> Rich

Hi Rich,

Thank you for your reply. This is definitely a discussion to be had and 
I appreciate you sharing your thoughts. Just wanted to point out that 
the missing EINVAL issue popped up while running the LTP tests for 
clone. So, maybe this small change is still worth adding before having 
the bigger discussion.

Thanks,
Tudor