ASPLes / nopoll

OpenSource WebSocket toolkit
http://www.aspl.es/nopoll
GNU Lesser General Public License v2.1
124 stars 74 forks source link

Range check fds before using for FD_SET or FD_ISSET #35

Closed softins closed 6 years ago

softins commented 7 years ago

I had a crash associated with the closing of a websocket session from the remote end. I tracked it down to an out-of-range fds in nopoll_io_wait_select_is_set(). I think it might have been a race condition with shutting down the socket. Strange thing was that debugging the core dump with gdb showed fds contained 0x07FFFFFF rather than 0xFFFFFFFF.

But in any case, I think we need to check fds is not negative and not equal or higher than FD_SETSIZE before passing it to FD_SET or FD_ISSET, to avoid an array out-of-bounds access. Then such events will be handled gracefully.

francisbrosnan commented 6 years ago

Hello. I'm closing this PR because it will break nopoll_io_wait_select_is_set since you can have a FD_SETSIZE of, let's say, 4096, and attempt to add a fd with value 10043. Regards.

softins commented 6 years ago

Firstly, my copy of nopoll is running with this modification, and has not crashed since it was applied in October.

Secondly, I think your statement is not correct. Please see https://www.gnu.org/software/libc/manual/html_node/Waiting-for-I_002fO.html, particularly the description of FD_SETSIZE, where it says: "If you get a file descriptor with a value as high as FD_SETSIZE, you cannot put that descriptor into an fd_set."

FD_SETSIZE defines the size of an array of bits, where the array index is the fd itself. So you could never have FD_SETSIZE of 4096 and then add an fd of 10043.

francisbrosnan commented 6 years ago

Hello Tony. You are right. I've been reading what you point it seems fdset is implemented using a bitset which, obviously, cannot accept a value higher than the bit it is going to hold. I'm reopening and merging your patch. Thanks for your time and patience :-)