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

[Rich Felker <dalias@libc.org>]

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.