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
Comments
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(). |
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. |
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. |
Comment author: @damiendoligez
|
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. |
Comment author: hnrgrgr For the exception, which is the better choice:
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 ? |
Comment author: @damiendoligez The Mac OS man page for select contains this:
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. |
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. |
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.
The text was updated successfully, but these errors were encountered: