diff options
author | Bhasker Hariharan <bhaskerh@google.com> | 2021-04-22 16:31:11 -0700 |
---|---|---|
committer | gVisor bot <gvisor-bot@google.com> | 2021-04-22 16:34:00 -0700 |
commit | 2739cf46284f2786ad33b545d55b8178bc46f7de (patch) | |
tree | 45440f2d40f3eb885341e4a9709bdecf95d8a5c0 | |
parent | 0a6eaed50b83a35a687699aa5e871b80605c9f46 (diff) |
Fix AF_UNIX listen() w/ zero backlog.
In https://github.com/google/gvisor/commit/f075522849fa a check to increase zero
to a minimum backlog length was removed from sys_socket.go to bring it in parity
with linux and then in tcp/endpoint.go we bump backlog by 1. But this broke
calling listen on a AF_UNIX socket w/ a zero backlog as in linux it does allow 1
connection even with a zero backlog.
This was caught by a php runtime test socket_abstract_path.phpt.
PiperOrigin-RevId: 369974744
-rw-r--r-- | pkg/sentry/socket/unix/transport/connectioned.go | 4 | ||||
-rw-r--r-- | pkg/sentry/syscalls/linux/sys_socket.go | 13 | ||||
-rw-r--r-- | pkg/sentry/syscalls/linux/vfs2/socket.go | 13 | ||||
-rw-r--r-- | pkg/tcpip/transport/tcp/endpoint.go | 4 | ||||
-rw-r--r-- | pkg/tcpip/transport/tcp/tcp_test.go | 6 | ||||
-rw-r--r-- | test/syscalls/linux/socket_unix_unbound_abstract.cc | 46 |
6 files changed, 71 insertions, 15 deletions
diff --git a/pkg/sentry/socket/unix/transport/connectioned.go b/pkg/sentry/socket/unix/transport/connectioned.go index 408dfb08d..33f9aeb06 100644 --- a/pkg/sentry/socket/unix/transport/connectioned.go +++ b/pkg/sentry/socket/unix/transport/connectioned.go @@ -346,11 +346,11 @@ func (e *connectionedEndpoint) BidirectionalConnect(ctx context.Context, ce Conn return nil default: - // Busy; return ECONNREFUSED per spec. + // Busy; return EAGAIN per spec. ne.Close(ctx) e.Unlock() ce.Unlock() - return syserr.ErrConnectionRefused + return syserr.ErrTryAgain } } diff --git a/pkg/sentry/syscalls/linux/sys_socket.go b/pkg/sentry/syscalls/linux/sys_socket.go index eff251cec..5e9e940df 100644 --- a/pkg/sentry/syscalls/linux/sys_socket.go +++ b/pkg/sentry/syscalls/linux/sys_socket.go @@ -383,12 +383,19 @@ func Listen(t *kernel.Task, args arch.SyscallArguments) (uintptr, *kernel.Syscal // Linux treats incoming backlog as uint with a limit defined by // sysctl_somaxconn. // https://github.com/torvalds/linux/blob/7acac4b3196/net/socket.c#L1666 - // - // We use the backlog to allocate a channel of that size, hence enforce - // a hard limit for the backlog. backlog = maxListenBacklog } + // Accept one more than the configured listen backlog to keep in parity with + // Linux. Ref, because of missing equality check here: + // https://github.com/torvalds/linux/blob/7acac4b3196/include/net/sock.h#L937 + // + // In case of unix domain sockets, the following check + // https://github.com/torvalds/linux/blob/7d6beb71da3/net/unix/af_unix.c#L1293 + // will allow 1 connect through since it checks for a receive queue len > + // backlog and not >=. + backlog++ + return 0, nil, s.Listen(t, int(backlog)).ToError() } diff --git a/pkg/sentry/syscalls/linux/vfs2/socket.go b/pkg/sentry/syscalls/linux/vfs2/socket.go index 936614eab..6edde0ed1 100644 --- a/pkg/sentry/syscalls/linux/vfs2/socket.go +++ b/pkg/sentry/syscalls/linux/vfs2/socket.go @@ -387,12 +387,19 @@ func Listen(t *kernel.Task, args arch.SyscallArguments) (uintptr, *kernel.Syscal // Linux treats incoming backlog as uint with a limit defined by // sysctl_somaxconn. // https://github.com/torvalds/linux/blob/7acac4b3196/net/socket.c#L1666 - // - // We use the backlog to allocate a channel of that size, hence enforce - // a hard limit for the backlog. backlog = maxListenBacklog } + // Accept one more than the configured listen backlog to keep in parity with + // Linux. Ref, because of missing equality check here: + // https://github.com/torvalds/linux/blob/7acac4b3196/include/net/sock.h#L937 + // + // In case of unix domain sockets, the following check + // https://github.com/torvalds/linux/blob/7d6beb71da3/net/unix/af_unix.c#L1293 + // will allow 1 connect through since it checks for a receive queue len > + // backlog and not >=. + backlog++ + return 0, nil, s.Listen(t, int(backlog)).ToError() } diff --git a/pkg/tcpip/transport/tcp/endpoint.go b/pkg/tcpip/transport/tcp/endpoint.go index 50f72bf38..3a7b2d166 100644 --- a/pkg/tcpip/transport/tcp/endpoint.go +++ b/pkg/tcpip/transport/tcp/endpoint.go @@ -2398,10 +2398,6 @@ func (e *endpoint) shutdownLocked(flags tcpip.ShutdownFlags) tcpip.Error { // Listen puts the endpoint in "listen" mode, which allows it to accept // new connections. func (e *endpoint) Listen(backlog int) tcpip.Error { - // Accept one more than the configured listen backlog to keep in parity with - // Linux. Ref, because of missing equality check here: - // https://github.com/torvalds/linux/blob/7acac4b3196/include/net/sock.h#L937 - backlog++ err := e.listen(backlog) if err != nil { if !err.IgnoreStats() { diff --git a/pkg/tcpip/transport/tcp/tcp_test.go b/pkg/tcpip/transport/tcp/tcp_test.go index 9f29a48fb..3750b0691 100644 --- a/pkg/tcpip/transport/tcp/tcp_test.go +++ b/pkg/tcpip/transport/tcp/tcp_test.go @@ -5736,7 +5736,7 @@ func TestListenSynRcvdQueueFull(t *testing.T) { } // Test acceptance. - if err := c.EP.Listen(0); err != nil { + if err := c.EP.Listen(1); err != nil { t.Fatalf("Listen failed: %s", err) } @@ -5837,7 +5837,7 @@ func TestListenBacklogFullSynCookieInUse(t *testing.T) { } // Test for SynCookies usage after filling up the backlog. - if err := c.EP.Listen(0); err != nil { + if err := c.EP.Listen(1); err != nil { t.Fatalf("Listen failed: %s", err) } @@ -6120,7 +6120,7 @@ func TestPassiveFailedConnectionAttemptIncrement(t *testing.T) { if err := c.EP.Bind(tcpip.FullAddress{Addr: context.StackAddr, Port: context.StackPort}); err != nil { t.Fatalf("Bind failed: %s", err) } - if err := c.EP.Listen(0); err != nil { + if err := c.EP.Listen(1); err != nil { t.Fatalf("Listen failed: %s", err) } diff --git a/test/syscalls/linux/socket_unix_unbound_abstract.cc b/test/syscalls/linux/socket_unix_unbound_abstract.cc index 8b1762000..dd3d25450 100644 --- a/test/syscalls/linux/socket_unix_unbound_abstract.cc +++ b/test/syscalls/linux/socket_unix_unbound_abstract.cc @@ -72,6 +72,52 @@ TEST_P(UnboundAbstractUnixSocketPairTest, BindNothing) { SyscallSucceeds()); } +TEST_P(UnboundAbstractUnixSocketPairTest, ListenZeroBacklog) { + SKIP_IF((GetParam().type & SOCK_DGRAM) != 0); + auto sockets = ASSERT_NO_ERRNO_AND_VALUE(NewSocketPair()); + struct sockaddr_un addr = {}; + addr.sun_family = AF_UNIX; + constexpr char kPath[] = "\x00/foo_bar"; + memcpy(addr.sun_path, kPath, sizeof(kPath)); + ASSERT_THAT(bind(sockets->first_fd(), + reinterpret_cast<struct sockaddr*>(&addr), sizeof(addr)), + SyscallSucceeds()); + ASSERT_THAT(listen(sockets->first_fd(), 0 /* backlog */), SyscallSucceeds()); + ASSERT_THAT(connect(sockets->second_fd(), + reinterpret_cast<struct sockaddr*>(&addr), sizeof(addr)), + SyscallSucceeds()); + auto sockets2 = ASSERT_NO_ERRNO_AND_VALUE(NewSocketPair()); + { + // Set the FD to O_NONBLOCK. + int opts; + int orig_opts; + ASSERT_THAT(opts = fcntl(sockets2->first_fd(), F_GETFL), SyscallSucceeds()); + orig_opts = opts; + opts |= O_NONBLOCK; + ASSERT_THAT(fcntl(sockets2->first_fd(), F_SETFL, opts), SyscallSucceeds()); + + ASSERT_THAT( + connect(sockets2->first_fd(), reinterpret_cast<struct sockaddr*>(&addr), + sizeof(addr)), + SyscallFailsWithErrno(EAGAIN)); + } + { + // Set the FD to O_NONBLOCK. + int opts; + int orig_opts; + ASSERT_THAT(opts = fcntl(sockets2->second_fd(), F_GETFL), + SyscallSucceeds()); + orig_opts = opts; + opts |= O_NONBLOCK; + ASSERT_THAT(fcntl(sockets2->second_fd(), F_SETFL, opts), SyscallSucceeds()); + + ASSERT_THAT( + connect(sockets2->second_fd(), + reinterpret_cast<struct sockaddr*>(&addr), sizeof(addr)), + SyscallFailsWithErrno(EAGAIN)); + } +} + TEST_P(UnboundAbstractUnixSocketPairTest, GetSockNameFullLength) { auto sockets = ASSERT_NO_ERRNO_AND_VALUE(NewSocketPair()); |