* [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 public 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).