Re: [musl] [PATCH] accept4: don't fall back to accept if we got unknown flags

[Rich Felker <dalias@libc.org>]

[-- 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