mailing list of musl libc
 help / color / mirror / code / Atom feed
* [musl] [PATCH] clone: Return EINVAL for null stack
@ 2022-08-02 11:30 Tudor Cretu
  2022-08-02 18:53 ` Rich Felker
  0 siblings, 1 reply; 4+ messages in thread
From: Tudor Cretu @ 2022-08-02 11:30 UTC (permalink / raw)
  To: musl; +Cc: Tudor Cretu

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


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [musl] [PATCH] clone: Return EINVAL for null stack
  2022-08-02 11:30 [musl] [PATCH] clone: Return EINVAL for null stack Tudor Cretu
@ 2022-08-02 18:53 ` Rich Felker
  2022-08-03 14:42   ` Tudor Cretu
  0 siblings, 1 reply; 4+ messages in thread
From: Rich Felker @ 2022-08-02 18:53 UTC (permalink / raw)
  To: Tudor Cretu; +Cc: musl

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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [musl] [PATCH] clone: Return EINVAL for null stack
  2022-08-02 18:53 ` Rich Felker
@ 2022-08-03 14:42   ` Tudor Cretu
  2022-08-03 16:42     ` Rich Felker
  0 siblings, 1 reply; 4+ messages in thread
From: Tudor Cretu @ 2022-08-03 14:42 UTC (permalink / raw)
  To: Rich Felker; +Cc: musl



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

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [musl] [PATCH] clone: Return EINVAL for null stack
  2022-08-03 14:42   ` Tudor Cretu
@ 2022-08-03 16:42     ` Rich Felker
  0 siblings, 0 replies; 4+ messages in thread
From: Rich Felker @ 2022-08-03 16:42 UTC (permalink / raw)
  To: Tudor Cretu; +Cc: musl

On Wed, Aug 03, 2022 at 03:42:48PM +0100, Tudor Cretu wrote:
> 
> 
> 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.

Yeah, I'm inclined to agree.

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2022-08-03 16:42 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-08-02 11:30 [musl] [PATCH] clone: Return EINVAL for null stack Tudor Cretu
2022-08-02 18:53 ` Rich Felker
2022-08-03 14:42   ` Tudor Cretu
2022-08-03 16:42     ` Rich Felker

Code repositories for project(s) associated with this inbox:

	https://git.vuxu.org/mirror/musl/

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).