NetBSD Problem Report #57504

From www@netbsd.org  Thu Jul  6 02:03:42 2023
Return-Path: <www@netbsd.org>
Received: from mail.netbsd.org (mail.netbsd.org [199.233.217.200])
	(using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits))
	(Client CN "mail.NetBSD.org", Issuer "mail.NetBSD.org CA" (not verified))
	by mollari.NetBSD.org (Postfix) with ESMTPS id E1A001A923D
	for <gnats-bugs@gnats.NetBSD.org>; Thu,  6 Jul 2023 02:03:41 +0000 (UTC)
Message-Id: <20230706020340.BF73D1A923E@mollari.NetBSD.org>
Date: Thu,  6 Jul 2023 02:03:40 +0000 (UTC)
From: campbell+netbsd@mumble.net
Reply-To: campbell+netbsd@mumble.net
To: gnats-bugs@NetBSD.org
Subject: select with large enough bogus fd number set hangs instead of failing with EBADF
X-Send-Pr-Version: www-1.0

>Number:         57504
>Category:       kern
>Synopsis:       select with large enough bogus fd number set hangs instead of failing with EBADF
>Confidential:   no
>Severity:       serious
>Priority:       medium
>Responsible:    kre
>State:          closed
>Class:          sw-bug
>Submitter-Id:   net
>Arrival-Date:   Thu Jul 06 02:05:00 +0000 2023
>Closed-Date:    Wed Nov 20 14:24:57 +0000 2024
>Last-Modified:  Wed Nov 20 14:24:57 +0000 2024
>Originator:     Taylor R Campbell
>Release:        current
>Organization:
The SelectBSD Foundation
>Environment:
>Description:
$ cat selwhat.c
#include <sys/poll.h>
#include <sys/select.h>

#include <err.h>
#include <errno.h>
#include <fcntl.h>
#include <stdio.h>
#include <stdlib.h>
#include <string.h>

int
main(int argc, char **argv)
{
	fd_set readfds;
	int fd, ret;

	setprogname(argv[0]);
	if (argc != 2)
		errx(1, "Usage: %s <fd>", getprogname());
	fd = atoi(argv[1]);

	if (fcntl(fd, F_GETFL) != -1 || errno != EBADF)
		errx(1, "fd %d is already open", fd);

	FD_ZERO(&readfds);
	FD_SET(fd, &readfds);
	errno = 0;
	ret = select(fd + 1, &readfds, NULL, NULL, NULL);
	printf("ret=%d\nerrno=%d %s\n", ret, errno, strerror(errno));
	fflush(stdout);
	return ferror(stdout);
}
$ make selwhat
cc -O2   -o selwhat selwhat.c
$ ./selwhat 19
ret=-1
errno=9 Bad file descriptor
$ ./selwhat 20

and it just hangs.

The issue is this logic in revision 1.60 of sys_select.c:

   363          nf = atomic_load_consume(&curlwp->l_fd->fd_dt)->dt_nfiles;
   364          if (nd > nf) {
   365                  /* forgiving; slightly wrong */
   366                  nd = nf;
   367          }

The initial value of dt_nfiles (NDFILE from sys/filedesc.h) is 20, so this code treats select(124, ...) _as if_ it had been select(20, ...).  Finding no fds in {0, 1, 2, ..., 19} set in the fd sets, select chooses to sleep until timeout -- which is to say, forever, in this case.
>How-To-Repeat:

>Fix:
Yes, please!  At the very least, this should be documented if it's intentional.  But it's not clear why this 'forgiving' logic is there at all; is the purpose to avoid large scans when someone calls select(INT_MAX, ...)?

>Release-Note:

>Audit-Trail:

Responsible-Changed-From-To: kern-bug-people->kre
Responsible-Changed-By: kre@NetBSD.org
Responsible-Changed-When: Mon, 16 Sep 2024 18:29:35 +0000
Responsible-Changed-Why:
I have code to fix this issue (validating all fds passed)
currently being tested.


State-Changed-From-To: open->analyzed
State-Changed-By: kre@NetBSD.org
State-Changed-When: Mon, 23 Sep 2024 21:34:56 +0000
State-Changed-Why:
I have code running now which seems to fix this issue.

For now I am just using the test program (selwhat) provided in the PR, but
before committing I need to make some better tests (ones which actually do
a select() intended to work!).   I made one mod to the source, I added
	#define FD_SETSIZE 40000
before any of the #includes (so I can test operation with much larger
arg values).

On my system I have:

jacaranda$ ulimit -H -n
6668

And with that:

/tmp/selwhat 19
ret=-1
errno=9 Bad file descriptor
/tmp/selwhat 20
ret=-1
errno=9 Bad file descriptor
/tmp/selwhat 500
ret=-1
errno=9 Bad file descriptor
/tmp/selwhat 6668
ret=-1
errno=9 Bad file descriptor
/tmp/selwhat 6923
ret=-1
errno=9 Bad file descriptor
/tmp/selwhat 6924
ret=-1
errno=22 Invalid argument
/tmp/selwhat 20000
ret=-1
errno=22 Invalid argument


The code returns EINVAL if the fd arg for select() is >= $(ulimit -H -n) + FD_SETSIZE
(the kernel version of FD_SETSIZE, hence 256).  6668+256 == 2924.   Allowing the
extra is intended to allow applications to calculate the maximum FD_SETSIZE that
will work based upon the hard limit of file descriptors, and round up, and also to
allow some slop should that limit have been reduced after a very high numbered fd
was opened.

That the limit exists is just to prevent applications causing the kernel to do
excess work by supplying INT_MAX as the fd number.   If EINVAL is returned, no
checking of the fd_sets supplied is performed.   Otherwise, EBADF is returned
only if there is a set bit that doesn't represent an open fd (so passing fd == 6900
with only the bit representing fd==0 set, assuming stdin is open, won't generate
an error).

Are any comments on how this should operate, or changes that could be
made (I could use the kernel's "maxfiles" as the limit below which fds are
checked for example, rather than basing it on the process's resource limit)?


From: "Robert Elz" <kre@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/57504 CVS commit: src/sys/kern
Date: Fri, 18 Oct 2024 13:12:34 +0000

 Module Name:	src
 Committed By:	kre
 Date:		Fri Oct 18 13:12:34 UTC 2024

 Modified Files:
 	src/sys/kern: sys_select.c

 Log Message:
 PR kern/57504 : Check all fds passed in to select

 If an application passes in a huge fd_set (select(BIG, ...))
 then check every bit in the fd_sets provided, to make sure
 they are valid.

 If BIG is too big (cannot possibly represent an open fd for
 this process, under any circumstances: ie: not just because
 that many are not currently open) return EINVAL.

 Otherwise, check every set bit to make sure it is valid.  Any
 fd bits set above the applications current highest open fd
 automatically generate EBADF and quick(ish) exit.

 fd's that are within the plausible range are then checked as
 they always were (it is possible for there to be a few there
 above the max open fd - as everything in select is done in
 multiples of __FDBITS (fd_mask) but the max open fd is not so
 constrained.  Those always were checked, continue using the
 same mechanism.

 This should have zero impact on any sane application which
 uses the highest fd for which it set a bit, +1, as the first
 arg to select.   However, if there are any broken applications
 that were relying upon the previous behaviour of simply ignoring
 any fd_masks that started beyond the max number of open files,
 then they might (if they happen to have any bits set) now fail.

 XXX pullup -10 -- but not for a long time.  Someone remind me
     sometime next year.  Leave a long settling time in HEAD just to
     be sure no issues arise, as in practice, almost nothing should
     cause any of the new code to be executed.

     pullup -9 -- probably not, what this fixes isn't significant
     enough to bother going that far back for (IMO).


 To generate a diff of this commit:
 cvs rdiff -u -r1.66 -r1.67 src/sys/kern/sys_select.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: analyzed->needs-pullups
State-Changed-By: kre@NetBSD.org
State-Changed-When: Fri, 18 Oct 2024 13:18:19 +0000
State-Changed-Why:
Needs pullup to -10 (sometime in 2025).


From: Taylor R Campbell <riastradh@NetBSD.org>
To: gnats-bugs@netbsd.org
Cc: kre@netbsd.org, gnats-admin@netbsd.org, netbsd-bugs@netbsd.org
Subject: Re: PR/57504 CVS commit: src/sys/kern
Date: Fri, 18 Oct 2024 13:42:07 +0000

 > Date: Fri, 18 Oct 2024 13:12:34 +0000
 > From: "Robert Elz" <kre@netbsd.org>
 > 
 > This should have zero impact on any sane application which
 > uses the highest fd for which it set a bit, +1, as the first
 > arg to select.   However, if there are any broken applications
 > that were relying upon the previous behaviour of simply ignoring
 > any fd_masks that started beyond the max number of open files,
 > then they might (if they happen to have any bits set) now fail.

 I think this came up in a real application, but it's been long enough
 that I forget which one, and unfortunately I neglected to record it in
 the original PR (if it was a real application and not just code
 inspection on my part).

 Some real applications might not keep track of the highest fd they set
 in the fd_set, and just keep track of the highest fd they have ever
 allocated seen, or just pass FD_SETSIZE.

 > XXX pullup -10 -- but not for a long time.  Someone remind me
 >     sometime next year.  Leave a long settling time in HEAD just to
 >     be sure no issues arise, as in practice, almost nothing should
 >     cause any of the new code to be executed.
 > 
 >     pullup -9 -- probably not, what this fixes isn't significant
 >     enough to bother going that far back for (IMO).

 Surely we should have an automatic test in atf for this so can verify
 it has been fixed in pullup too?

 I think a fix for this bug is also worth pulling up to 9 -- basic
 central functionality like select should be reliable, not spuriously
 hang in weird circumstances.

From: "Taylor R Campbell" <riastradh@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/57504 CVS commit: src/tests/lib/libc/sys
Date: Fri, 18 Oct 2024 16:39:41 +0000

 Module Name:	src
 Committed By:	riastradh
 Date:		Fri Oct 18 16:39:41 UTC 2024

 Modified Files:
 	src/tests/lib/libc/sys: t_select.c

 Log Message:
 tests/lib/libc/sys/t_select: Test select on bad file descriptors.

 This should immediately fail, not hang, even if the bad fd is
 high-numbered.

 PR kern/57504: select with large enough bogus fd number set hangs
 instead of failing with EBADF


 To generate a diff of this commit:
 cvs rdiff -u -r1.4 -r1.5 src/tests/lib/libc/sys/t_select.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: Robert Elz <kre@munnari.OZ.AU>
To: Taylor R Campbell <riastradh@NetBSD.org>
Cc: gnats-bugs@netbsd.org, netbsd-bugs@netbsd.org
Subject: Re: PR/57504 CVS commit: src/sys/kern
Date: Sat, 19 Oct 2024 00:06:59 +0700

     Date:        Fri, 18 Oct 2024 13:42:07 +0000
     From:        Taylor R Campbell <riastradh@NetBSD.org>
     Message-ID:  <20241018134212.9624084D09@mail.netbsd.org>

   | Some real applications might not keep track of the highest fd they set
   | in the fd_set, and just keep track of the highest fd they have ever
   | allocated seen, or just pass FD_SETSIZE.

 They might, but most I have seen (which still use select, and not
 poll) seem to use max_fd+1 -- probably just copying code from place
 to place as is normal.

   | Surely we should have an automatic test in atf for this so can verify
   | it has been fixed in pullup too?

 Yes, I was looking at doing one of those - waiting for that to happen
 was partly what held up the commit for so long, I've had the code for
 quite a while, & been running it for ages now.   I eventually decided
 that I simply wasn't getting that written any time soon, and decided to
 commit anyway.

   | I think a fix for this bug is also worth pulling up to 9 -- basic
   | central functionality like select should be reliable, not spuriously
   | hang in weird circumstances.

 Perhaps - it isn't as if we have lots of bug reports about things like
 that happening though do we?   It is also possible that -9 might be, or
 approaching, EOL before I get around do doing any pullups - I think this
 needs a long gestation in HEAD, just because this code is likely to be
 exercised very rarely - if there is something not quite right it might
 take a long time to reveal itself.

 kre


From: Robert Elz <kre@munnari.OZ.AU>
To: Taylor R Campbell <riastradh@NetBSD.org>
Cc: gnats-bugs@netbsd.org, netbsd-bugs@netbsd.org
Subject: Re: PR/57504 CVS commit: src/sys/kern
Date: Sat, 19 Oct 2024 00:08:20 +0700

 And now I see you have committed an ATF test already.   Great, thanks.

 kre

State-Changed-From-To: needs-pullups->pending-pullups
State-Changed-By: riastradh@NetBSD.org
State-Changed-When: Sun, 17 Nov 2024 17:51:46 +0000
State-Changed-Why:
pullup-9 #1926 https://releng.netbsd.org/cgi-bin/req-9.cgi?show=1926
pullup-10 #1011 https://releng.netbsd.org/cgi-bin/req-10.cgi?show=1011


From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/57504 CVS commit: [netbsd-10] src
Date: Mon, 18 Nov 2024 18:16:52 +0000

 Module Name:	src
 Committed By:	martin
 Date:		Mon Nov 18 18:16:52 UTC 2024

 Modified Files:
 	src/sys/kern [netbsd-10]: sys_select.c
 	src/tests/lib/libc/sys [netbsd-10]: t_select.c

 Log Message:
 Pull up following revision(s) (requested by riastradh in ticket #1011):

 	sys/kern/sys_select.c: revision 1.67
 	tests/lib/libc/sys/t_select.c: revision 1.5

 PR kern/57504 : Check all fds passed in to select

 If an application passes in a huge fd_set (select(BIG, ...))
 then check every bit in the fd_sets provided, to make sure
 they are valid.

 If BIG is too big (cannot possibly represent an open fd for
 this process, under any circumstances: ie: not just because
 that many are not currently open) return EINVAL.
 Otherwise, check every set bit to make sure it is valid.  Any
 fd bits set above the applications current highest open fd
 automatically generate EBADF and quick(ish) exit.
 fd's that are within the plausible range are then checked as
 they always were (it is possible for there to be a few there
 above the max open fd - as everything in select is done in
 multiples of __FDBITS (fd_mask) but the max open fd is not so
 constrained.  Those always were checked, continue using the
 same mechanism.

 This should have zero impact on any sane application which
 uses the highest fd for which it set a bit, +1, as the first
 arg to select.   However, if there are any broken applications
 that were relying upon the previous behaviour of simply ignoring
 any fd_masks that started beyond the max number of open files,
 then they might (if they happen to have any bits set) now fail.

 tests/lib/libc/sys/t_select: Test select on bad file descriptors.
 This should immediately fail, not hang, even if the bad fd is
 high-numbered.

 PR kern/57504: select with large enough bogus fd number set hangs
 instead of failing with EBADF


 To generate a diff of this commit:
 cvs rdiff -u -r1.60 -r1.60.4.1 src/sys/kern/sys_select.c
 cvs rdiff -u -r1.4 -r1.4.28.1 src/tests/lib/libc/sys/t_select.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

From: "Martin Husemann" <martin@netbsd.org>
To: gnats-bugs@gnats.NetBSD.org
Cc: 
Subject: PR/57504 CVS commit: [netbsd-9] src
Date: Wed, 20 Nov 2024 14:09:10 +0000

 Module Name:	src
 Committed By:	martin
 Date:		Wed Nov 20 14:09:10 UTC 2024

 Modified Files:
 	src/sys/kern [netbsd-9]: sys_select.c
 	src/tests/lib/libc/sys [netbsd-9]: t_select.c

 Log Message:
 Pull up following revision(s) (requested by riastradh in ticket #1926):

 	sys/kern/sys_select.c: revision 1.67 (patch)
 	tests/lib/libc/sys/t_select.c: revision 1.5 (patch)

 PR kern/57504 : Check all fds passed in to select

 If an application passes in a huge fd_set (select(BIG, ...))
 then check every bit in the fd_sets provided, to make sure
 they are valid.

 If BIG is too big (cannot possibly represent an open fd for
 this process, under any circumstances: ie: not just because
 that many are not currently open) return EINVAL.

 Otherwise, check every set bit to make sure it is valid.  Any
 fd bits set above the applications current highest open fd
 automatically generate EBADF and quick(ish) exit.
 fd's that are within the plausible range are then checked as
 they always were (it is possible for there to be a few there
 above the max open fd - as everything in select is done in
 multiples of __FDBITS (fd_mask) but the max open fd is not so
 constrained.  Those always were checked, continue using the
 same mechanism.

 This should have zero impact on any sane application which
 uses the highest fd for which it set a bit, +1, as the first
 arg to select.   However, if there are any broken applications
 that were relying upon the previous behaviour of simply ignoring
 any fd_masks that started beyond the max number of open files,
 then they might (if they happen to have any bits set) now fail.

 tests/lib/libc/sys/t_select: Test select on bad file descriptors.

 This should immediately fail, not hang, even if the bad fd is
 high-numbered.

 PR kern/57504: select with large enough bogus fd number set hangs
 instead of failing with EBADF


 To generate a diff of this commit:
 cvs rdiff -u -r1.46.2.1 -r1.46.2.2 src/sys/kern/sys_select.c
 cvs rdiff -u -r1.4 -r1.4.16.1 src/tests/lib/libc/sys/t_select.c

 Please note that diffs are not public domain; they are subject to the
 copyright notices on the relevant files.

State-Changed-From-To: pending-pullups->closed
State-Changed-By: riastradh@NetBSD.org
State-Changed-When: Wed, 20 Nov 2024 14:24:57 +0000
State-Changed-Why:
fixed in HEAD, pulled up to 9 and 10


>Unformatted:

Home	PR Database Search