mailing list of musl libc
 help / color / mirror / code / Atom feed
* [musl] [PATCH] accept4: don't fall back to accept if we got unknown flags
@ 2023-02-27 19:46 Alexey Izbyshev
  2023-02-27 22:38 ` Rich Felker
  0 siblings, 1 reply; 9+ messages in thread
From: Alexey Izbyshev @ 2023-02-27 19:46 UTC (permalink / raw)
  To: musl

accept4 emulation via accept ignores unknown flags, so it can spuriously
succeed instead of failing (or succeed without doing the action implied
by an unknown flag if it's added in a future kernel). Worse, unknown
flags trigger the fallback code even on modern kernels if the real
accept4 syscall returns EINVAL, because this is indistinguishable from
socketcall returning EINVAL due to lack of accept4 support. Fix this by
always propagating the syscall attempt failure if unknown flags are
present.

The behavior is still not ideal on old kernels lacking accept4 on arches
with socketcall, where failing with ENOSYS instead of EINVAL returned by
socketcall would be preferable, but at least modern kernels are now
fine.
---
 src/network/accept4.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/network/accept4.c b/src/network/accept4.c
index 59ab1726..bbb79507 100644
--- a/src/network/accept4.c
+++ b/src/network/accept4.c
@@ -9,6 +9,7 @@ int accept4(int fd, struct sockaddr *restrict addr, socklen_t *restrict len, int
 	if (!flg) return accept(fd, addr, len);
 	int ret = socketcall_cp(accept4, fd, addr, len, flg, 0, 0);
 	if (ret>=0 || (errno != ENOSYS && errno != EINVAL)) return ret;
+	if (flg & ~(SOCK_CLOEXEC|SOCK_NONBLOCK)) return ret;
 	ret = accept(fd, addr, len);
 	if (ret<0) return ret;
 	if (flg & SOCK_CLOEXEC)
-- 
2.39.1


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

* Re: [musl] [PATCH] accept4: don't fall back to accept if we got unknown flags
  2023-02-27 19:46 [musl] [PATCH] accept4: don't fall back to accept if we got unknown flags Alexey Izbyshev
@ 2023-02-27 22:38 ` Rich Felker
  2023-02-27 23:42   ` Alexey Izbyshev
  0 siblings, 1 reply; 9+ messages in thread
From: Rich Felker @ 2023-02-27 22:38 UTC (permalink / raw)
  To: musl

On Mon, Feb 27, 2023 at 10:46:54PM +0300, Alexey Izbyshev wrote:
> accept4 emulation via accept ignores unknown flags, so it can spuriously
> succeed instead of failing (or succeed without doing the action implied
> by an unknown flag if it's added in a future kernel). Worse, unknown
> flags trigger the fallback code even on modern kernels if the real
> accept4 syscall returns EINVAL, because this is indistinguishable from
> socketcall returning EINVAL due to lack of accept4 support. Fix this by
> always propagating the syscall attempt failure if unknown flags are
> present.
> 
> The behavior is still not ideal on old kernels lacking accept4 on arches
> with socketcall, where failing with ENOSYS instead of EINVAL returned by
> socketcall would be preferable, but at least modern kernels are now
> fine.

Can you clarify what you mean about ENOSYS vs EINVAL here? I'm not
following.

Rich

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

* Re: [musl] [PATCH] accept4: don't fall back to accept if we got unknown flags
  2023-02-27 22:38 ` Rich Felker
@ 2023-02-27 23:42   ` Alexey Izbyshev
  2023-02-27 23:51     ` Alexey Izbyshev
  2023-02-28 17:21     ` Rich Felker
  0 siblings, 2 replies; 9+ messages in thread
From: Alexey Izbyshev @ 2023-02-27 23:42 UTC (permalink / raw)
  To: musl

On 2023-02-28 01:38, Rich Felker wrote:
> On Mon, Feb 27, 2023 at 10:46:54PM +0300, Alexey Izbyshev wrote:
>> accept4 emulation via accept ignores unknown flags, so it can 
>> spuriously
>> succeed instead of failing (or succeed without doing the action 
>> implied
>> by an unknown flag if it's added in a future kernel). Worse, unknown
>> flags trigger the fallback code even on modern kernels if the real
>> accept4 syscall returns EINVAL, because this is indistinguishable from
>> socketcall returning EINVAL due to lack of accept4 support. Fix this 
>> by
>> always propagating the syscall attempt failure if unknown flags are
>> present.
>> 
>> The behavior is still not ideal on old kernels lacking accept4 on 
>> arches
>> with socketcall, where failing with ENOSYS instead of EINVAL returned 
>> by
>> socketcall would be preferable, but at least modern kernels are now
>> fine.
> 
> Can you clarify what you mean about ENOSYS vs EINVAL here? I'm not
> following.
> 
Sorry for confusion, I meant the following. On arches with socketcall, 
if a program running on an old kernel that doesn't support accept4 in 
any form calls accept4 with unknown flags, musl's accept4 will fail with 
EINVAL after this patch. But the reason of failure remains unclear to 
the programmer: is it because some flag is not supported or because 
accept4 is not supported at all? So I thought it'd be better to fail 
with ENOSYS in this case instead, although I don't know a good way to do 
that: the EINVAL ambiguity exists at socketcall level too, so testing 
whether the kernel's socketcall supports __SC_accept4 or not would 
probably involve calling it with known-good arguments on a separately 
created socket, and I certainly don't propose to do that.

On the other hand, it could be argued that a function that can emulate a 
certain baseline feature set of another function shouldn't fail with 
ENOSYS at all because the real function would never do that. The two 
cleanest options for possibly-not-supported functions seem to be either 
always failing with ENOSYS if the kernel doesn't support the syscall or 
failing with a reasonable error if the caller requests something 
unsupported by the emulation. And I think accept4 satisfies the latter 
with this patch.

As an aside, note that dup3 and pipe2 currently also ignore unknown 
flags on old kernels, and for pipe2 there is a valid flag (O_DIRECT) 
that could be silently ignored because of that. But there is no issue on 
newer kernels supporting the syscalls, unlike for accept4.

Alexey

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

* Re: [musl] [PATCH] accept4: don't fall back to accept if we got unknown flags
  2023-02-27 23:42   ` Alexey Izbyshev
@ 2023-02-27 23:51     ` Alexey Izbyshev
  2023-02-27 23:53       ` Rich Felker
  2023-02-28 17:21     ` Rich Felker
  1 sibling, 1 reply; 9+ messages in thread
From: Alexey Izbyshev @ 2023-02-27 23:51 UTC (permalink / raw)
  To: musl

On 2023-02-28 02:42, Alexey Izbyshev wrote:
> On 2023-02-28 01:38, Rich Felker wrote:
>> On Mon, Feb 27, 2023 at 10:46:54PM +0300, Alexey Izbyshev wrote:
>>> accept4 emulation via accept ignores unknown flags, so it can 
>>> spuriously
>>> succeed instead of failing (or succeed without doing the action 
>>> implied
>>> by an unknown flag if it's added in a future kernel). Worse, unknown
>>> flags trigger the fallback code even on modern kernels if the real
>>> accept4 syscall returns EINVAL, because this is indistinguishable 
>>> from
>>> socketcall returning EINVAL due to lack of accept4 support. Fix this 
>>> by
>>> always propagating the syscall attempt failure if unknown flags are
>>> present.
>>> 
>>> The behavior is still not ideal on old kernels lacking accept4 on 
>>> arches
>>> with socketcall, where failing with ENOSYS instead of EINVAL returned 
>>> by
>>> socketcall would be preferable, but at least modern kernels are now
>>> fine.
>> 
>> Can you clarify what you mean about ENOSYS vs EINVAL here? I'm not
>> following.
>> 
> Sorry for confusion, I meant the following. On arches with socketcall,
> if a program running on an old kernel that doesn't support accept4 in
> any form calls accept4 with unknown flags, musl's accept4 will fail
> with EINVAL after this patch. But the reason of failure remains
> unclear to the programmer: is it because some flag is not supported or
> because accept4 is not supported at all? So I thought it'd be better
> to fail with ENOSYS in this case instead, although I don't know a good
> way to do that: the EINVAL ambiguity exists at socketcall level too,
> so testing whether the kernel's socketcall supports __SC_accept4 or
> not would probably involve calling it with known-good arguments on a
> separately created socket, and I certainly don't propose to do that.
> 
> On the other hand, it could be argued that a function that can emulate
> a certain baseline feature set of another function shouldn't fail with
> ENOSYS at all because the real function would never do that. The two
> cleanest options for possibly-not-supported functions seem to be
> either always failing with ENOSYS if the kernel doesn't support the
> syscall or failing with a reasonable error if the caller requests
> something unsupported by the emulation. And I think accept4 satisfies
> the latter with this patch.

But actually it doesn't. The error should always be EINVAL in case of 
unknown flags for that, but the patch propagates ENOSYS on arches 
without socketcall.

Alexey

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

* Re: [musl] [PATCH] accept4: don't fall back to accept if we got unknown flags
  2023-02-27 23:51     ` Alexey Izbyshev
@ 2023-02-27 23:53       ` Rich Felker
  0 siblings, 0 replies; 9+ messages in thread
From: Rich Felker @ 2023-02-27 23:53 UTC (permalink / raw)
  To: musl

On Tue, Feb 28, 2023 at 02:51:42AM +0300, Alexey Izbyshev wrote:
> On 2023-02-28 02:42, Alexey Izbyshev wrote:
> >On 2023-02-28 01:38, Rich Felker wrote:
> >>On Mon, Feb 27, 2023 at 10:46:54PM +0300, Alexey Izbyshev wrote:
> >>>accept4 emulation via accept ignores unknown flags, so it can
> >>>spuriously
> >>>succeed instead of failing (or succeed without doing the
> >>>action implied
> >>>by an unknown flag if it's added in a future kernel). Worse, unknown
> >>>flags trigger the fallback code even on modern kernels if the real
> >>>accept4 syscall returns EINVAL, because this is
> >>>indistinguishable from
> >>>socketcall returning EINVAL due to lack of accept4 support.
> >>>Fix this by
> >>>always propagating the syscall attempt failure if unknown flags are
> >>>present.
> >>>
> >>>The behavior is still not ideal on old kernels lacking accept4
> >>>on arches
> >>>with socketcall, where failing with ENOSYS instead of EINVAL
> >>>returned by
> >>>socketcall would be preferable, but at least modern kernels are now
> >>>fine.
> >>
> >>Can you clarify what you mean about ENOSYS vs EINVAL here? I'm not
> >>following.
> >>
> >Sorry for confusion, I meant the following. On arches with socketcall,
> >if a program running on an old kernel that doesn't support accept4 in
> >any form calls accept4 with unknown flags, musl's accept4 will fail
> >with EINVAL after this patch. But the reason of failure remains
> >unclear to the programmer: is it because some flag is not supported or
> >because accept4 is not supported at all? So I thought it'd be better
> >to fail with ENOSYS in this case instead, although I don't know a good
> >way to do that: the EINVAL ambiguity exists at socketcall level too,
> >so testing whether the kernel's socketcall supports __SC_accept4 or
> >not would probably involve calling it with known-good arguments on a
> >separately created socket, and I certainly don't propose to do that.
> >
> >On the other hand, it could be argued that a function that can emulate
> >a certain baseline feature set of another function shouldn't fail with
> >ENOSYS at all because the real function would never do that. The two
> >cleanest options for possibly-not-supported functions seem to be
> >either always failing with ENOSYS if the kernel doesn't support the
> >syscall or failing with a reasonable error if the caller requests
> >something unsupported by the emulation. And I think accept4 satisfies
> >the latter with this patch.
> 
> But actually it doesn't. The error should always be EINVAL in case
> of unknown flags for that, but the patch propagates ENOSYS on arches
> without socketcall.

Yeah, I was just about to say that. I think that's wrong. The patch
should be something like:

+       if (flg & ~(SOCK_CLOEXEC|SOCK_NONBLOCK)) return __syscall_ret(-EINVAL);

or explicitly assigning to errno.

Rich

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

* Re: [musl] [PATCH] accept4: don't fall back to accept if we got unknown flags
  2023-02-27 23:42   ` Alexey Izbyshev
  2023-02-27 23:51     ` Alexey Izbyshev
@ 2023-02-28 17:21     ` Rich Felker
  2023-02-28 17:25       ` Rich Felker
  1 sibling, 1 reply; 9+ messages in thread
From: Rich Felker @ 2023-02-28 17:21 UTC (permalink / raw)
  To: musl

On Tue, Feb 28, 2023 at 02:42:39AM +0300, Alexey Izbyshev wrote:
> On 2023-02-28 01:38, Rich Felker wrote:
> >On Mon, Feb 27, 2023 at 10:46:54PM +0300, Alexey Izbyshev wrote:
> >>accept4 emulation via accept ignores unknown flags, so it can
> >>spuriously
> >>succeed instead of failing (or succeed without doing the action
> >>implied
> >>by an unknown flag if it's added in a future kernel). Worse, unknown
> >>flags trigger the fallback code even on modern kernels if the real
> >>accept4 syscall returns EINVAL, because this is indistinguishable from
> >>socketcall returning EINVAL due to lack of accept4 support. Fix
> >>this by
> >>always propagating the syscall attempt failure if unknown flags are
> >>present.
> >>
> >>The behavior is still not ideal on old kernels lacking accept4
> >>on arches
> >>with socketcall, where failing with ENOSYS instead of EINVAL
> >>returned by
> >>socketcall would be preferable, but at least modern kernels are now
> >>fine.
> >
> >Can you clarify what you mean about ENOSYS vs EINVAL here? I'm not
> >following.
> >
> Sorry for confusion, I meant the following. On arches with
> socketcall, if a program running on an old kernel that doesn't
> support accept4 in any form calls accept4 with unknown flags, musl's
> accept4 will fail with EINVAL after this patch. But the reason of
> failure remains unclear to the programmer: is it because some flag
> is not supported or because accept4 is not supported at all? So I
> thought it'd be better to fail with ENOSYS in this case instead,
> although I don't know a good way to do that: the EINVAL ambiguity
> exists at socketcall level too, so testing whether the kernel's
> socketcall supports __SC_accept4 or not would probably involve
> calling it with known-good arguments on a separately created socket,
> and I certainly don't propose to do that.
> 
> On the other hand, it could be argued that a function that can
> emulate a certain baseline feature set of another function shouldn't
> fail with ENOSYS at all because the real function would never do
> that. The two cleanest options for possibly-not-supported functions
> seem to be either always failing with ENOSYS if the kernel doesn't
> support the syscall or failing with a reasonable error if the caller
> requests something unsupported by the emulation. And I think accept4
> satisfies the latter with this patch.
> 
> As an aside, note that dup3 and pipe2 currently also ignore unknown
> flags on old kernels, and for pipe2 there is a valid flag (O_DIRECT)
> that could be silently ignored because of that. But there is no
> issue on newer kernels supporting the syscalls, unlike for accept4.

The dup3 situation is even worse than you thought. The dup3 syscall is
only attempted if O_CLOEXEC is set in flags. If not, the rest of flags
are ignored and the dup2 syscall is made. I'll make a fix.

Rich

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

* Re: [musl] [PATCH] accept4: don't fall back to accept if we got unknown flags
  2023-02-28 17:21     ` Rich Felker
@ 2023-02-28 17:25       ` Rich Felker
  2023-02-28 20:15         ` Alexey Izbyshev
  0 siblings, 1 reply; 9+ messages in thread
From: Rich Felker @ 2023-02-28 17:25 UTC (permalink / raw)
  To: musl

[-- Attachment #1: Type: text/plain, Size: 3050 bytes --]

On Tue, Feb 28, 2023 at 12:21:01PM -0500, Rich Felker wrote:
> On Tue, Feb 28, 2023 at 02:42:39AM +0300, Alexey Izbyshev wrote:
> > On 2023-02-28 01:38, Rich Felker wrote:
> > >On Mon, Feb 27, 2023 at 10:46:54PM +0300, Alexey Izbyshev wrote:
> > >>accept4 emulation via accept ignores unknown flags, so it can
> > >>spuriously
> > >>succeed instead of failing (or succeed without doing the action
> > >>implied
> > >>by an unknown flag if it's added in a future kernel). Worse, unknown
> > >>flags trigger the fallback code even on modern kernels if the real
> > >>accept4 syscall returns EINVAL, because this is indistinguishable from
> > >>socketcall returning EINVAL due to lack of accept4 support. Fix
> > >>this by
> > >>always propagating the syscall attempt failure if unknown flags are
> > >>present.
> > >>
> > >>The behavior is still not ideal on old kernels lacking accept4
> > >>on arches
> > >>with socketcall, where failing with ENOSYS instead of EINVAL
> > >>returned by
> > >>socketcall would be preferable, but at least modern kernels are now
> > >>fine.
> > >
> > >Can you clarify what you mean about ENOSYS vs EINVAL here? I'm not
> > >following.
> > >
> > Sorry for confusion, I meant the following. On arches with
> > socketcall, if a program running on an old kernel that doesn't
> > support accept4 in any form calls accept4 with unknown flags, musl's
> > accept4 will fail with EINVAL after this patch. But the reason of
> > failure remains unclear to the programmer: is it because some flag
> > is not supported or because accept4 is not supported at all? So I
> > thought it'd be better to fail with ENOSYS in this case instead,
> > although I don't know a good way to do that: the EINVAL ambiguity
> > exists at socketcall level too, so testing whether the kernel's
> > socketcall supports __SC_accept4 or not would probably involve
> > calling it with known-good arguments on a separately created socket,
> > and I certainly don't propose to do that.
> > 
> > On the other hand, it could be argued that a function that can
> > emulate a certain baseline feature set of another function shouldn't
> > fail with ENOSYS at all because the real function would never do
> > that. The two cleanest options for possibly-not-supported functions
> > seem to be either always failing with ENOSYS if the kernel doesn't
> > support the syscall or failing with a reasonable error if the caller
> > requests something unsupported by the emulation. And I think accept4
> > satisfies the latter with this patch.
> > 
> > As an aside, note that dup3 and pipe2 currently also ignore unknown
> > flags on old kernels, and for pipe2 there is a valid flag (O_DIRECT)
> > that could be silently ignored because of that. But there is no
> > issue on newer kernels supporting the syscalls, unlike for accept4.
> 
> The dup3 situation is even worse than you thought. The dup3 syscall is
> only attempted if O_CLOEXEC is set in flags. If not, the rest of flags
> are ignored and the dup2 syscall is made. I'll make a fix.

These should fix both..


[-- Attachment #2: 0001-fix-pipe2-silently-ignoring-unknown-flags-on-old-ker.patch --]
[-- Type: text/plain, Size: 965 bytes --]

From fb7fb5e4bd7ccb8efa691364404efc7804fad90c Mon Sep 17 00:00:00 2001
From: Rich Felker <dalias@aerifal.cx>
Date: Tue, 28 Feb 2023 12:18:43 -0500
Subject: [PATCH 1/2] fix pipe2 silently ignoring unknown flags on old kernels

kernels using the fallback have an inherent close-on-exec race
condition and as such support for them is only best-effort anyway.
however, ignoring potential new flags is still very bad behavior.
instead, fail with EINVAL.
---
 src/unistd/pipe2.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/unistd/pipe2.c b/src/unistd/pipe2.c
index f24f74fb..a096990b 100644
--- a/src/unistd/pipe2.c
+++ b/src/unistd/pipe2.c
@@ -8,6 +8,7 @@ int pipe2(int fd[2], int flag)
 	if (!flag) return pipe(fd);
 	int ret = __syscall(SYS_pipe2, fd, flag);
 	if (ret != -ENOSYS) return __syscall_ret(ret);
+	if (flag & ~(O_CLOEXEC|O_NONBLOCK)) return __syscall_ret(-EINVAL);
 	ret = pipe(fd);
 	if (ret) return ret;
 	if (flag & O_CLOEXEC) {
-- 
2.21.0


[-- Attachment #3: 0002-fix-dup3-ignoring-all-flags-but-O_CLOEXEC-on-archs-w.patch --]
[-- Type: text/plain, Size: 1380 bytes --]

From c99b7daafdbf1e2415bf408e67ca7813e7ddeedf Mon Sep 17 00:00:00 2001
From: Rich Felker <dalias@aerifal.cx>
Date: Tue, 28 Feb 2023 12:21:23 -0500
Subject: [PATCH 2/2] fix dup3 ignoring all flags but O_CLOEXEC on archs with
 SYS_dup2 syscall

our dup3 code wrongly skipped directly to making the SYS_dup2 syscall
whenever the O_CLOEXEC bit of flags was not set. this is incorrect if
any new flags are ever added, as it would silently ignore them rather
than failing with an error.

archs which lack SYS_dup2 were unaffected.

adjust the logic so that SYS_dup3 is attempted whenever flags is
nonzero, and explicitly fail with EINVAL if SYS_dup3 is unavailable
and there are any unknown flags.
---
 src/unistd/dup3.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/unistd/dup3.c b/src/unistd/dup3.c
index f919f791..1e7dc3e7 100644
--- a/src/unistd/dup3.c
+++ b/src/unistd/dup3.c
@@ -9,9 +9,10 @@ int __dup3(int old, int new, int flags)
 	int r;
 #ifdef SYS_dup2
 	if (old==new) return __syscall_ret(-EINVAL);
-	if (flags & O_CLOEXEC) {
+	if (flags) {
 		while ((r=__syscall(SYS_dup3, old, new, flags))==-EBUSY);
 		if (r!=-ENOSYS) return __syscall_ret(r);
+		if (flags & ~O_CLOEXEC) return __syscall_ret(-EINVAL);
 	}
 	while ((r=__syscall(SYS_dup2, old, new))==-EBUSY);
 	if (flags & O_CLOEXEC) __syscall(SYS_fcntl, new, F_SETFD, FD_CLOEXEC);
-- 
2.21.0


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

* Re: [musl] [PATCH] accept4: don't fall back to accept if we got unknown flags
  2023-02-28 17:25       ` Rich Felker
@ 2023-02-28 20:15         ` Alexey Izbyshev
  2023-02-28 20:51           ` Rich Felker
  0 siblings, 1 reply; 9+ messages in thread
From: Alexey Izbyshev @ 2023-02-28 20:15 UTC (permalink / raw)
  To: musl

On 2023-02-28 20:25, Rich Felker wrote:
> On Tue, Feb 28, 2023 at 12:21:01PM -0500, Rich Felker wrote:
>> On Tue, Feb 28, 2023 at 02:42:39AM +0300, Alexey Izbyshev wrote:
>> > On 2023-02-28 01:38, Rich Felker wrote:
>> > >On Mon, Feb 27, 2023 at 10:46:54PM +0300, Alexey Izbyshev wrote:
>> > >>accept4 emulation via accept ignores unknown flags, so it can
>> > >>spuriously
>> > >>succeed instead of failing (or succeed without doing the action
>> > >>implied
>> > >>by an unknown flag if it's added in a future kernel). Worse, unknown
>> > >>flags trigger the fallback code even on modern kernels if the real
>> > >>accept4 syscall returns EINVAL, because this is indistinguishable from
>> > >>socketcall returning EINVAL due to lack of accept4 support. Fix
>> > >>this by
>> > >>always propagating the syscall attempt failure if unknown flags are
>> > >>present.
>> > >>
>> > >>The behavior is still not ideal on old kernels lacking accept4
>> > >>on arches
>> > >>with socketcall, where failing with ENOSYS instead of EINVAL
>> > >>returned by
>> > >>socketcall would be preferable, but at least modern kernels are now
>> > >>fine.
>> > >
>> > >Can you clarify what you mean about ENOSYS vs EINVAL here? I'm not
>> > >following.
>> > >
>> > Sorry for confusion, I meant the following. On arches with
>> > socketcall, if a program running on an old kernel that doesn't
>> > support accept4 in any form calls accept4 with unknown flags, musl's
>> > accept4 will fail with EINVAL after this patch. But the reason of
>> > failure remains unclear to the programmer: is it because some flag
>> > is not supported or because accept4 is not supported at all? So I
>> > thought it'd be better to fail with ENOSYS in this case instead,
>> > although I don't know a good way to do that: the EINVAL ambiguity
>> > exists at socketcall level too, so testing whether the kernel's
>> > socketcall supports __SC_accept4 or not would probably involve
>> > calling it with known-good arguments on a separately created socket,
>> > and I certainly don't propose to do that.
>> >
>> > On the other hand, it could be argued that a function that can
>> > emulate a certain baseline feature set of another function shouldn't
>> > fail with ENOSYS at all because the real function would never do
>> > that. The two cleanest options for possibly-not-supported functions
>> > seem to be either always failing with ENOSYS if the kernel doesn't
>> > support the syscall or failing with a reasonable error if the caller
>> > requests something unsupported by the emulation. And I think accept4
>> > satisfies the latter with this patch.
>> >
>> > As an aside, note that dup3 and pipe2 currently also ignore unknown
>> > flags on old kernels, and for pipe2 there is a valid flag (O_DIRECT)
>> > that could be silently ignored because of that. But there is no
>> > issue on newer kernels supporting the syscalls, unlike for accept4.
>> 
>> The dup3 situation is even worse than you thought. The dup3 syscall is
>> only attempted if O_CLOEXEC is set in flags. If not, the rest of flags
>> are ignored and the dup2 syscall is made. I'll make a fix.
> 
Indeed, I missed that, thanks.

> These should fix both..

The patches look good to me.

But looking at dup3 more closely, I've noticed another bug: fcntl is 
called even if SYS_dup2 fails. So on kernels where SYS_dup3 is 
unavailable, dup3(-1, fd, O_CLOEXEC) will wrongly try to set FD_CLOEXEC 
on fd.

Alexey

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

* Re: [musl] [PATCH] accept4: don't fall back to accept if we got unknown flags
  2023-02-28 20:15         ` Alexey Izbyshev
@ 2023-02-28 20:51           ` Rich Felker
  0 siblings, 0 replies; 9+ messages in thread
From: Rich Felker @ 2023-02-28 20:51 UTC (permalink / raw)
  To: musl

On Tue, Feb 28, 2023 at 11:15:06PM +0300, Alexey Izbyshev wrote:
> On 2023-02-28 20:25, Rich Felker wrote:
> >On Tue, Feb 28, 2023 at 12:21:01PM -0500, Rich Felker wrote:
> >>On Tue, Feb 28, 2023 at 02:42:39AM +0300, Alexey Izbyshev wrote:
> >>> On 2023-02-28 01:38, Rich Felker wrote:
> >>> >On Mon, Feb 27, 2023 at 10:46:54PM +0300, Alexey Izbyshev wrote:
> >>> >>accept4 emulation via accept ignores unknown flags, so it can
> >>> >>spuriously
> >>> >>succeed instead of failing (or succeed without doing the action
> >>> >>implied
> >>> >>by an unknown flag if it's added in a future kernel). Worse, unknown
> >>> >>flags trigger the fallback code even on modern kernels if the real
> >>> >>accept4 syscall returns EINVAL, because this is indistinguishable from
> >>> >>socketcall returning EINVAL due to lack of accept4 support. Fix
> >>> >>this by
> >>> >>always propagating the syscall attempt failure if unknown flags are
> >>> >>present.
> >>> >>
> >>> >>The behavior is still not ideal on old kernels lacking accept4
> >>> >>on arches
> >>> >>with socketcall, where failing with ENOSYS instead of EINVAL
> >>> >>returned by
> >>> >>socketcall would be preferable, but at least modern kernels are now
> >>> >>fine.
> >>> >
> >>> >Can you clarify what you mean about ENOSYS vs EINVAL here? I'm not
> >>> >following.
> >>> >
> >>> Sorry for confusion, I meant the following. On arches with
> >>> socketcall, if a program running on an old kernel that doesn't
> >>> support accept4 in any form calls accept4 with unknown flags, musl's
> >>> accept4 will fail with EINVAL after this patch. But the reason of
> >>> failure remains unclear to the programmer: is it because some flag
> >>> is not supported or because accept4 is not supported at all? So I
> >>> thought it'd be better to fail with ENOSYS in this case instead,
> >>> although I don't know a good way to do that: the EINVAL ambiguity
> >>> exists at socketcall level too, so testing whether the kernel's
> >>> socketcall supports __SC_accept4 or not would probably involve
> >>> calling it with known-good arguments on a separately created socket,
> >>> and I certainly don't propose to do that.
> >>>
> >>> On the other hand, it could be argued that a function that can
> >>> emulate a certain baseline feature set of another function shouldn't
> >>> fail with ENOSYS at all because the real function would never do
> >>> that. The two cleanest options for possibly-not-supported functions
> >>> seem to be either always failing with ENOSYS if the kernel doesn't
> >>> support the syscall or failing with a reasonable error if the caller
> >>> requests something unsupported by the emulation. And I think accept4
> >>> satisfies the latter with this patch.
> >>>
> >>> As an aside, note that dup3 and pipe2 currently also ignore unknown
> >>> flags on old kernels, and for pipe2 there is a valid flag (O_DIRECT)
> >>> that could be silently ignored because of that. But there is no
> >>> issue on newer kernels supporting the syscalls, unlike for accept4.
> >>
> >>The dup3 situation is even worse than you thought. The dup3 syscall is
> >>only attempted if O_CLOEXEC is set in flags. If not, the rest of flags
> >>are ignored and the dup2 syscall is made. I'll make a fix.
> >
> Indeed, I missed that, thanks.
> 
> >These should fix both..
> 
> The patches look good to me.
> 
> But looking at dup3 more closely, I've noticed another bug: fcntl is
> called even if SYS_dup2 fails. So on kernels where SYS_dup3 is
> unavailable, dup3(-1, fd, O_CLOEXEC) will wrongly try to set
> FD_CLOEXEC on fd.

Thanks for catching that. I'll fix it too.

Rich

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

end of thread, other threads:[~2023-02-28 20:52 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-02-27 19:46 [musl] [PATCH] accept4: don't fall back to accept if we got unknown flags Alexey Izbyshev
2023-02-27 22:38 ` Rich Felker
2023-02-27 23:42   ` Alexey Izbyshev
2023-02-27 23:51     ` Alexey Izbyshev
2023-02-27 23:53       ` Rich Felker
2023-02-28 17:21     ` Rich Felker
2023-02-28 17:25       ` Rich Felker
2023-02-28 20:15         ` Alexey Izbyshev
2023-02-28 20:51           ` 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).