Mantis Bug Tracker
Main | My View | View Issues | Change Log | Roadmap
  

View Issue Details [ Jump to Notes ] [ Issue History ] [ Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0005563OCamlOCaml otherlibspublic2012-03-27 20:512012-09-24 13:28
Reportersweeks 
Assigned To 
PriorityhighSeveritymajorReproducibilityalways
StatusresolvedResolutionfixed 
PlatformOSOS Version
Product Version3.12.1 
Target Version4.00.2+devFixed in Version4.00.1+dev 
Summary0005563: Caml.Unix.select doesn't bounds-check file-descriptor integer
DescriptionCaml.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.
TagsNo tags attached.
Attached Files

- Relationships

-  Notes
(0007212)
gerd (reporter)
2012-03-28 14:54

 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().
(0007217)
milanst (reporter)
2012-03-28 17:52

 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.
(0007219)
gerd (reporter)
2012-03-28 19:21

 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.
(0007257)
doligez (manager)
2012-04-01 00:41

 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.
(0008082)
frisch (developer)
2012-09-15 17:12

 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.
(0008102)
hnrgrgr (reporter)
2012-09-18 09:50

 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 ?
(0008136)
doligez (manager)
2012-09-21 13:56

 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.
(0008151)
xleroy (administrator)
2012-09-24 13:28

 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.

- Issue History
Date Modified Username Field Change
2012-03-27 20:51 sweeks New Issue
2012-03-28 14:54 gerd Note Added: 0007212
2012-03-28 17:52 milanst Note Added: 0007217
2012-03-28 19:21 gerd Note Added: 0007219
2012-04-01 00:41 doligez Note Added: 0007257
2012-04-01 00:41 doligez Status new => acknowledged
2012-07-09 17:48 doligez Target Version => 4.01.0+dev
2012-07-31 13:36 doligez Target Version 4.01.0+dev => 4.00.1+dev
2012-09-15 17:12 frisch Note Added: 0008082
2012-09-15 17:13 frisch Severity minor => major
2012-09-18 09:50 hnrgrgr Note Added: 0008102
2012-09-21 13:56 doligez Note Added: 0008136
2012-09-21 13:56 doligez Priority normal => high
2012-09-21 13:56 doligez Target Version 4.00.1+dev => 4.00.2+dev
2012-09-24 13:28 xleroy Note Added: 0008151
2012-09-24 13:28 xleroy Status acknowledged => resolved
2012-09-24 13:28 xleroy Resolution open => fixed
2012-09-24 13:28 xleroy Fixed in Version => 4.00.1+dev

Copyright © 2000 - 2011 MantisBT Group
Powered by Mantis Bugtracker