Backport #6352: Windows: FD_SET and FD_SETSIZE segv due different compilation flags

• Status: Closed
• Assignee: naruse (Yui NARUSE)

[luislavena (Luis Lavena)]

Hello,

As mentioned in #6228 [ruby-core:43951]:

• Ruby compiled with -DFD_SETSIZE=32767 will allocate 32K fd_array elements for fd_set structure [1]
• FD_SET() macro has been redefined in win32/win32.h to use rb_w32_fdset instead [2]
• Other programs (like EventMachine) compiled with a different FD_SETSIZE will cause SEGV.

The technical details for this SEGV were provided by Hiroshi Shirosaki in Note 16, which I'm quoting:
https://bugs.ruby-lang.org/issues/6228#note-16

I think above issue is cause of fd_array buffer overflow.

typedef struct fd_set
{
u_int fd_count;
SOCKET fd_array[FD_SETSIZE];
} fd_set;

On EM, FD_SETSIZE = 1024 and fd_array[1024].
EM uses FD_SET() and FD_SET() seems rb_w32_fdset() on Windows.

In rb_w32_fdset(), FD_SETSIZE = 32767 since rb_w32_fdset is compiled with -DFD_SETSIZE=32767. [3]

if (i == set->fd_count) {
    if (set->fd_count < FD_SETSIZE) { // FD_SETSIZE = 32767
        set->fd_array[i] = s;                 // `i` could be over 1023
        set->fd_count++;
    }
}

If above scenario is correct, FD_SETSIZE of Ruby should be equal or less then FD_SETSIZE of EM.

include/winsock2.h has FD_SET macro on mingw, but MRI undef FD_SET and uses rb_w32_fdset() function. It might be better that FD_SET() is macro instead of function.

SEGV is caused by that discrepancy between rb_w32_fdset thinking have 32K of sockets and EventMachine only having 1K to iterate over.

[1] http://msdn.microsoft.com/en-us/library/windows/desktop/ms737873(v=vs.85).aspx
[2] https://github.com/ruby/ruby/blob/trunk/include/ruby/win32.h#L583-590
[3] https://github.com/ruby/ruby/blob/trunk/win32/win32.c#L2457-2474

[mame (Yusuke Endoh)]

• Status changed from Open to Assigned

[usa (Usaku NAKAMURA)]

• File fd_macros.diff added

like attached patch?

[usa (Usaku NAKAMURA)]

• File deleted (fd_macros.diff)

[usa (Usaku NAKAMURA)]

• File fd_macros.diff fd_macros.diff added

Updated the patch to keep compatibility.

[h.shirosaki (Hiroshi Shirosaki)]

I'm Sorry. My suggestion was just hypothesis by code review. I've investigated further. And real cause of SEGV seems invalid memory access by mismatched type fd_set. So far FD_SET() is not related with this.

gdb session:

C:\Users\hiroshi\work\eventmachine>gdb --args ruby -reventmachine -e "EM.run"
GNU gdb (GDB) 7.3
Copyright (C) 2011 Free Software Foundation, Inc.
License GPLv3+: GNU GPL version 3 or later http://gnu.org/licenses/gpl.html
This is free software: you are free to change and redistribute it.
There is NO WARRANTY, to the extent permitted by law. Type "show copying"
and "show warranty" for details.
This GDB was configured as "mingw32".
For bug reporting instructions, please see:
http://www.gnu.org/software/gdb/bugs/...
Reading symbols from v:\ruby19_mingw\bin\ruby.exe...done.
(gdb) b em.cpp:808
No source file named em.cpp.
Make breakpoint pending on future shared library load? (y or [n]) y

Breakpoint 1 (em.cpp:808) pending.
(gdb) r
Starting program: v:\ruby19_mingw\bin\ruby.exe -reventmachine -e EM.run
[New Thread 3444.0xef8]
[New Thread 3444.0xf18]

Breakpoint 1, _SelectDataSelect (v=0x28c66c) at em.cpp:808
808 sd->nSockets = select (sd->maxsocket+1, &(sd->fdreads), &(sd->fd
writes), &(sd->fderrors), &(sd->tv));
(gdb) p sizeof(fd_set)
$1 = 4100
(gdb) p sd->fdreads
$2 = {fd_count = 1, fd_array = {200, 65793, 0, 0, 0, 0, 0, 0, 16843008,
65793, 0 <repeats 38 times>, 8398853, 2672536, 1719461921, 0, 0, 2672520,
1720123927, 520, 47110968, 2672584, 47111352, 1720672320, 2, 3, 0,
48222088, 1719464304, 2672552, 268889518, 2, 1720672320, 2673600,
2673428, 0, 2676776, 0, 1719464214, 0, 0, 2672616, 1720123927, 496,
47110920, 2672680, 2004877312, 1720672320, 12, 496, 496, 47111352,
1719464304, 1720672320, 47110968, 1720672320, 2, 2672712, 1719463002,
48224544, 1719464192, 2672680, 1719464145, 47955512, 2, 2672712,
1720123927, 26989, 47960416, 2, 2004877312, 1720672320, 12, 1720672320,
2, 46697496, 45076784, 2672792, 47116992, 1720672320, 1, 1998202552,
2004969299, 10682368, 121, 48718456, 3840, 5, 60, 2673748, 2673748, 134,
0, 2673640, 1719980120, 1720672320, 12, 1720672320, 268889519, 520,
46696104, 2672904, 48194048, 1720672320, 12, 2672840, 1720420124, 0, 0,
61000, 2673816, 2673576, 2672868, 2673920, 2673748, 4, 2675336, 0,
2672868, 10682704, 1, 4, 0, 0, 2, 5, 0, 0, 0, 0, 0, 1720672256, 12, 1,
68369624, 47111328, 48223404, 0, 0, 0, 0, 0, 0, 0, 46553088, 2672136,
1719474474, 1720672320, 12, 2672136, 2, 5, 0, 0, 0, 0, 0, 2672128,
3011510385, 47111352, 48222748, 2672264, 47111232, 2, 5, 0, 0, 134,
0 <repeats 12 times>, 16843009, 16843009, 257, 0, 16843008, 65793, 0, 0,
0, 0, 0, 0, 16843008, 65793...}}
(gdb) p &(sd->fdreads)
$3 = (fd_set *) 0x28c670
(gdb) s
rb_w32_select@20 (nfds=4, rd=0x28c670, wr=0x28d674, ex=0x28e678,
timeout=0x28f67c) at ../../../ruby/win32/win32.c:2892
2892 return rb_w32_select_with_thread(nfds, rd, wr, ex, timeout, 0);
(gdb) p rd
$4 = (fd_set *) 0x28c670
(gdb) p *rd
Cannot access memory at address 0x28c670
(gdb) p *wr
Cannot access memory at address 0x28d674
(gdb) p *ex
Cannot access memory at address 0x28e678
(gdb) p sizeof(fd_set)
$5 = 131072

At em.cpp:808, p sd->fdreads is valid, but in rb_w32_select, p *rd is invalid though the pointer is same 0x28c670.
Memory access to undefined region seems not permitted. sizeof(fd_set) is larger than real size of *rd.

If FD_SETSIZE of ruby is smaller than FD_SETSIZE of EM, memory access to undefined region doesn't occur. So the case would work without SEGV.

So far as I know, FD_SETSIZE of ruby <= FD_SETSIZE of EM would be required.

[h.shirosaki (Hiroshi Shirosaki)]

• File fix_em_segv.patch fix_em_segv.patch added

I found a solution. rb_w32_select_with_thread() uses *rd.
Modifying not to use *rd seems to work fine. Using copy_fd() is right way?

I attached the patch. I confirmed EM works without SEGV.

[h.shirosaki (Hiroshi Shirosaki)]

• File fix_em_segv2.patch fix_em_segv2.patch added

I've updated the patch and added tests.
make test and test-all are fine.

sizeof() is determined by type at compile time. So the usage of sizeof() in copy_fd() was same as FD_SETSIZE in the previous patch. I removed that.
Is inline function better than macro do .. while? Either way is okay by me.
I'm not sure that FD_ISSET() and FD_CLR() are worth to change. They seem not related to this SEGV issue.

[usa (Usaku NAKAMURA)]

Perhaps there is no big difference in this case between macros
and inline functions.
You say that you have already testet your patch with EM, so
it's good to commit yours.

• Status changed from Assigned to Closed
• % Done changed from 0 to 100

This issue was solved with changeset r35595.
Luis, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.

---

• include/ruby/win32.h (FD_SET): change function to macro.
  To avoid buffer overflow when smaller FD_SETSISE is used in ext
  libraries.

• win32/win32.c (rb_w32_fdset): this function is not used anymore.
  But we leave this for compatibility.

• win32/win32.c (rb_w32_select_with_thread): fix SEGV when smaller
  FD_SETSISE is used in ext libraries. Dereference of fd_set pointer
  causes SEGV.

• test/-ext-/win32/test_fd_setsize.rb(TestFdSetSize): add tests for
  above.

• ext/-test-/win32/fd_setsize/depend: ditto.

• ext/-test-/win32/fd_setsize/extconf.rb: ditto.

• ext/-test-/win32/fd_setsize/fd_setsize.c: ditto.

  [ruby-core:44588] [Bug #6352]

[shyouhei (Shyouhei Urabe)]

• Tracker changed from Bug to Backport
• Project changed from Ruby master to Backport193
• Category deleted (core)
• Status changed from Closed to Assigned
• Assignee changed from usa (Usaku NAKAMURA) to naruse (Yui NARUSE)
• Target version deleted (1.9.3)

Please backport r35595.

[naruse (Yui NARUSE)]

• Status changed from Assigned to Closed

This issue was solved with changeset r35783.
Luis, thank you for reporting this issue.
Your contribution to Ruby is greatly appreciated.
May Ruby be with you.

---

merge revision(s) 35595: [Backport #6446]

* include/ruby/win32.h (FD_SET): change function to macro.
  To avoid buffer overflow when smaller FD_SETSISE is used in ext
  libraries.

* win32/win32.c (rb_w32_fdset): this function is not used anymore.
  But we leave this for compatibility.

* win32/win32.c (rb_w32_select_with_thread): fix SEGV when smaller
  FD_SETSISE is used in ext libraries. Dereference of fd_set pointer
  causes SEGV.

* test/-ext-/win32/test_fd_setsize.rb(TestFdSetSize): add tests for
  above.

* ext/-test-/win32/fd_setsize/depend: ditto.

* ext/-test-/win32/fd_setsize/extconf.rb: ditto.

* ext/-test-/win32/fd_setsize/fd_setsize.c: ditto.
  [ruby-core:44588] [Bug #6352]