Protect uses of select() from crash on too many file descriptors #119
Conversation
The fd_set macros don't check for overflow and will cause an out-of-bounds access when given a file description >= FD_SETSIZE (1024) This doesn't fix the fundamental issue, but prevents a crash and should allow partial functionality. The full fix requires switching to poll() instead.
penguin359
left a comment
penguin359
left a comment
There was a problem hiding this comment.
Hmm, seems I am still unable to assign this to @apconole for a review.
| return; | ||
|
|
||
| if (sock >= FD_SETSIZE) { | ||
| warn_too_many_fds(); |
There was a problem hiding this comment.
I'm okay with this being added, but a quick grep through the code doesn't seem to find any in-tree callers for eloop_wait_for_read_sock.
Maybe we should delete it instead?
There was a problem hiding this comment.
Yes, I only see eloop_register_read_sock() and eloop_unregister_read_sock() used, but not eloop_wait_for_read_sock(). Also, it looks like eloop_register_sock(), eloop_unregister_sock(), and os_get_time() can all be made static.
There was a problem hiding this comment.
Actually, I found several things to clean-up in eloop.c/eloop.h including function prototypes in the header files that refer to functions that have never existed in the Git history. I'll save that clean-up for another PR, but I think I can drop that function for now.
There was a problem hiding this comment.
If you are okay with it, I'll merge this patch and backport it to the 1.1 branch.
|
Merged and backported. Thanks! |
The fd_set macros don't check for overflow and will cause an
out-of-bounds access when given a file description >= FD_SETSIZE (1024)
This is to address issue #118 which originally reported this crash. Ultimately, we should switch to using either
poll()orepoll()for handling file descriptor events, but this PR will at least preventlldpadfrom a crash when used with a large (> ~500) interfaces. Some interfaces will fail to operate correctly, but this at least allows for partial functionality until the full fix can be included.