Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Caml.Unix.select doesn't bounds-check file-descriptor integer #5563

Closed
vicuna opened this issue Mar 27, 2012 · 8 comments
Closed

Caml.Unix.select doesn't bounds-check file-descriptor integer #5563

vicuna opened this issue Mar 27, 2012 · 8 comments

Comments

@vicuna
Copy link

vicuna commented Mar 27, 2012

Original bug ID: 5563
Reporter: sweeks
Status: closed (set by @xavierleroy on 2015-12-11T18:08:07Z)
Resolution: fixed
Priority: high
Severity: major
Version: 3.12.1
Target version: 4.00.2+dev
Fixed in version: 4.00.1+dev
Category: otherlibs
Related to: #7276
Monitored by: @gasche @ygrek @hcarty @dbuenzli

Bug description

Caml.Unix.select, via the C implementation in otherlibs/unix/select.c, has a bug
because it assumes the file descriptors have a value that is smaller than the
maximum index supported by the C [fd_set] bit-array type. Here is the relevant
code from select.c:

CAMLprim value unix_select(value readfds, value writefds, value exceptfds,
value timeout)
{
fd_set read, write, except;
...
fdlist_to_fdset(readfds, &read, &maxfd);
fdlist_to_fdset(writefds, &write, &maxfd);
fdlist_to_fdset(exceptfds, &except, &maxfd);
...
}

static void fdlist_to_fdset(value fdlist, fd_set *fdset, int *maxfd)
{
value l;
FD_ZERO(fdset);
for (l = fdlist; l != Val_int(0); l = Field(l, 1)) {
int fd = Int_val(Field(l, 0));
FD_SET(fd, fdset);
if (fd > *maxfd) *maxfd = fd;
}
}

So, unix_select allocates fd_set bit-arrays on the stack, with the size of the
fd_set a compile-time constant determined by the sys/select.h include file.
Then, the calls to fdlist_to_fdset essentially set bits in the bit-arrays
without doing a bounds-check on the index:

The most obvious fix would be for fdlist_to_fdset to do a bounds check before
calling FD_SET.

@vicuna
Copy link
Author

vicuna commented Mar 28, 2012

Comment author: gerd

This problem can normally not occur, as the limit for descriptors in fd_set is intentionally as large as (or larger than) the standard limit for the number of open files (ulimit -n, this is the case for all operating systems I know). Only if you increase the limit for the number of open files this is an issue - however, select() is then broken anyway, because it has a compile-time limit, whereas the number of open files is a dynamic limit.

The real issue is that we need poll(). Don't try to fix select().

@vicuna
Copy link
Author

vicuna commented Mar 28, 2012

Comment author: milanst

The issue here is that there are programs that need more than 1024 file descriptors, and if they call select on some high numbered file descriptor (which would be a bug), the stack will get corrupted and the program can behave in all sort of bad ways (if you are lucky it will segfault, but it might not). With the suggested change, the program would get an informative exception and it would be much easier to debug and fix.

Btw, I'm not sure what you mean by select() is broken. The compile-time limit (the size of fd_set structure) is set by libc headers, and is not a limitation of the actual system call. On some systems you can just define FD_SETSIZE macro before including <select.h> (it is slightly more complicated on linux, but is doable) and thne you can call select with higher numbered file descriptors.

@vicuna
Copy link
Author

vicuna commented Mar 28, 2012

Comment author: gerd

milanst: In order to create more than 1024 descriptors, you need to call ulimit first. If you are then running a select-based program, you are completely on your own, IMHO. The issue is well-known.

I think we should stick to POSIX and ignore any possible extensions by which you can dynamize fd_set. FD_SETSIZE always assumes a static limit - on some systems you can modify it, but it is still static. The system call may accept variable length sets, but for what is this good if we don't have libc support (Drepper rejects it), and are completely non-portable. So, getting a version of select() with fully-dynamic set sizes might be possible, but is unrealistic to do and to maintain.

@vicuna
Copy link
Author

vicuna commented Mar 31, 2012

Comment author: @damiendoligez

  1. We will stick to POSIX.
  2. If FD_SET corrupts the stack, I say it's a bug in FD_SET.
  3. We should do the bound checking anyway.

@vicuna
Copy link
Author

vicuna commented Sep 15, 2012

Comment author: @alainfrisch

I've been told it's actually an important issue. If it's as simple as adding some bound checking, I'd suggest to do it.

@vicuna
Copy link
Author

vicuna commented Sep 18, 2012

Comment author: hnrgrgr

For the exception, which is the better choice:

  • Invalid_argument("Unix.select: invalid file descriptor")
  • Unix_error(EINVAL,"select", "invalid file descriptor")
  • a new exception

I would prefer the second one as 'Invalid_argument' is currently used in the Unix module only for non implemented functions and because it avoids the burden of a new exception (and corresponding exception handlers). However it uses EINVAL in a non-standard way.

Should it be documented in the ocamldoc ?

@vicuna
Copy link
Author

vicuna commented Sep 21, 2012

Comment author: @damiendoligez

The Mac OS man page for select contains this:

The behavior of these macros is undefined if a descriptor value is less than zero or greater than or equal to FD_SETSIZE
So basically, it's not a bug of FD_SET and we need the check.

Also, the man page mentions EINVAL as a possible error for select (specifically for the case where the highest fd is too high), so let's reuse it.

@vicuna
Copy link
Author

vicuna commented Sep 24, 2012

Comment author: @xavierleroy

Tentative fix in 4.00 branch (r12947) and in trunk (r12948). Unix_error(EINVAL, "select") is raised if the fd lists contain a fd < 0 or >= FD_SETSIZE.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

1 participant