Project

General

Profile

Actions

Backport #1567

closed

socket.c silently discards internal listen(2) failures

Added by pilcrow (Mike Pomraning) over 15 years ago. Updated over 5 years ago.

Status:
Closed
[ruby-core:23698]

Description

=begin
Symptom:
TCPServer.new(...) may appear to succeed, even though its implied listen(2) call has silently failed. Similarly for UNIXServer.new().

Expectation:
Failures in the underlying listen(2) call should translate to the appropriate ruby SystemCallError.

Patch:
The attached patch looks to me to address the issue under 1.9.1, and test/socket/* test cases all pass with it.

Background:
ext/socket/socket.c init_inetsock_internal() and init_unixsock() each conditionally call listen(2) but discard its return value. This behavior is incorrect, possibly leaving the caller with an unlistening socket when a listening one was expected, and inconsistent with the way those init_ functions handle failures of other implied socket calls, such as bind(2) or connect(2).

It is tempting to omit error-checking listen(2) calls when the preceding, successful socket(2) and bind(2) calls are close by and visually verifiable (as in the init_* functions), since the most familiar listen(2) failures result from the programmer mistakenly operating on an inappropriate file descriptor. However, misbehaving threads can modify the file descriptor table behind our backs (EBADF), system/process failures do occur (ENOBUFS, EACCES) on some platforms, and LD_PRELOADed libraries may play wild tricks with socketry (EGOODLUCK). For these reasons, the init_ functions should rb_sys_fail when listen(2) fails.
=end


Files

ruby-1.9.1-p129-socket_listen.patch (936 Bytes) ruby-1.9.1-p129-socket_listen.patch Catch listen(2) failures in init_inetsock_internal and init_unixsock pilcrow (Mike Pomraning), 06/04/2009 06:03 AM
ruby-1.9.1-p129-socket_listen_2.patch (949 Bytes) ruby-1.9.1-p129-socket_listen_2.patch Catch listen(2) failures and clean up after them pilcrow (Mike Pomraning), 06/05/2009 11:26 AM
Actions #1

Updated by pilcrow (Mike Pomraning) over 15 years ago

=begin
Mea culpa: the attached patch close(2)s socket fd if listen(2) fails inside TCPServer initialization.

With respect to SystemCallError behavior, one can (on Linux, at least) verify behavior like so:

$ cat /tmp/listen_fail.c
#include <errno.h>

/* gcc -fPIC -rdynamic -g -c -Wall listen_fail.c
* gcc -shared -Wl,-soname,liblisten_fail.so.1 -o liblisten_fail.so.1 listen_fail.o -lc
*/

int
listen(int s, int backlog)
{
errno = ENOBUFS;
return -1;
}

$ basename pwd
ruby-1.9.1-p129

without patch

$ LD_PRELOAD=/tmp/liblisten_fail.so.1 ./miniruby -I./lib -I.ext/common -I./- -r./ext/purelib.rb ./runruby.rb --extout=.ext -r socket -e 'TCPServer.new(1024)'
$

with patch

$ LD_PRELOAD=/tmp/liblisten_fail.so.1 ./miniruby -I./lib -I.ext/common -I./- -r./ext/purelib.rb ./runruby.rb --extout=.ext -r socket -e 'TCPServer.new(1024)'
-e:1:in initialize': No buffer space available - listen(2) (Errno::ENOBUFS) from -e:1:in new'
from -e:1:in `'

... and similarly for UNIXServer.new(...)
=end

Actions #2

Updated by rogerdpack (Roger Pack) almost 15 years ago

=begin
was this updated in trunk? if so which revision?
=end

Actions #3

Updated by mame (Yusuke Endoh) over 14 years ago

  • Assignee set to mame (Yusuke Endoh)

=begin
Hi,

Thank you for reporting. I agree that this is certainly a bug.

Unfortunately, your patch causes errors when running rubyspec
because it closes invalid fd when bind(2) fails.

The following patch would work. I'll commit soon.

diff --git a/ext/socket/ipsocket.c b/ext/socket/ipsocket.c
index 447ae40..b6b2426 100644
--- a/ext/socket/ipsocket.c
+++ b/ext/socket/ipsocket.c
@@ -104,8 +104,13 @@ init_inetsock_internal(struct inetsock_arg *arg)

  arg->fd = -1;
  • if (type == INET_SERVER)
  • listen(fd, 5);
  • if (type == INET_SERVER) {

  • status = listen(fd, 5);

  • if (status < 0) {

  •  close(fd);
    
  •  syscall = "listen(2)";
    
  • }

  • }

    /* create new instance */
    return rsock_init_sock(arg->sock, fd);
    diff --git a/ext/socket/unixsocket.c b/ext/socket/unixsocket.c
    index 907f89c..4c3c5a7 100644
    --- a/ext/socket/unixsocket.c
    +++ b/ext/socket/unixsocket.c
    @@ -65,7 +65,12 @@ rsock_init_unixsock(VALUE sock, VALUE path, int server)
    rb_sys_fail(sockaddr.sun_path);
    }

  • if (server) listen(fd, 5);
  • if (server) {

  • if (listen(fd, 5) < 0) {

  •  close(fd);
    
  •  rb_sys_fail("listen(2)");
    
  • }

  • }

    rsock_init_sock(sock, fd);
    if (server) {

--
Yusuke Endoh
=end

Actions #4

Updated by mame (Yusuke Endoh) over 14 years ago

  • Assignee changed from mame (Yusuke Endoh) to yugui (Yuki Sonoda)

=begin
Applied at r27272. Moved to Backport tracker.

--
Yusuke Endoh
=end

Actions #5

Updated by shyouhei (Shyouhei Urabe) over 14 years ago

  • Status changed from Open to Assigned

=begin

=end

Actions #6

Updated by jeremyevans0 (Jeremy Evans) over 5 years ago

  • Description updated (diff)
  • Status changed from Assigned to Closed
Actions

Also available in: Atom PDF

Like0
Like0Like0Like0Like0Like0Like0