Backport #6352

Windows: FD_SET and FD_SETSIZE segv due different compilation flags

Added by Luis Lavena almost 3 years ago. Updated almost 3 years ago.

[ruby-core:44588]
Status:Closed
Priority:Normal
Assignee:Yui NARUSE

Description

Hello,

As mentioned in #6228 :

  • 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

fd_macros.diff Magnifier (3.1 KB) Usaku NAKAMURA, 04/25/2012 12:00 PM

fix_em_segv.patch Magnifier (2.33 KB) Hiroshi Shirosaki, 04/25/2012 11:23 PM

fix_em_segv2.patch Magnifier (4.75 KB) Hiroshi Shirosaki, 05/04/2012 10:38 PM

Associated revisions

Revision 35595
Added by shirosaki almost 3 years ago

  • 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.

[Bug #6352]

Revision 35783
Added by Yui NARUSE almost 3 years ago

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.
   [Bug #6352]

History

#1 Updated by Yusuke Endoh almost 3 years ago

  • Status changed from Open to Assigned

#2 Updated by Usaku NAKAMURA almost 3 years ago

  • File fd_macros.diff added

like attached patch?

#3 Updated by Usaku NAKAMURA almost 3 years ago

  • File deleted (fd_macros.diff)

#4 Updated by Usaku NAKAMURA almost 3 years ago

Updated the patch to keep compatibility.

#5 Updated by Hiroshi Shirosaki almost 3 years ago

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 , 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 , 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.

#6 Updated by Hiroshi Shirosaki almost 3 years ago

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.

#7 Updated by Hiroshi Shirosaki almost 3 years ago

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.

#8 Updated by Usaku NAKAMURA almost 3 years ago

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.

#9 Updated by Anonymous almost 3 years ago

  • 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.

[Bug #6352]

#10 Updated by Shyouhei Urabe almost 3 years ago

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

Please backport r35595.

#11 Updated by Yui NARUSE almost 3 years ago

  • 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.
   [Bug #6352]

Also available in: Atom PDF