Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle bind failures when running NIO in a tight sandbox #2616

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
1 change: 1 addition & 0 deletions Sources/NIOPosix/SocketChannel.swift
Original file line number Diff line number Diff line change
Expand Up @@ -263,6 +263,7 @@ final class ServerSocketChannel: BaseSocketChannel<ServerSocket> {
// It's important to call the methods before we actually notify the original promise for ordering reasons.
self.becomeActive0(promise: promise)
}.whenFailure{ error in
self.close0(error: error, mode: .all, promise: nil)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Upon bind failing, the lifecycleManager seems to remain in the preRegistered state. thus we are transiting to the closed state here.

promise?.fail(error)
}
executeAndComplete(p) {
Expand Down
8 changes: 6 additions & 2 deletions Tests/NIOPosixTests/AcceptBackoffHandlerTest.swift
Original file line number Diff line number Diff line change
Expand Up @@ -265,15 +265,19 @@ public final class AcceptBackoffHandlerTest: XCTestCase {
name: self.acceptHandlerName)
}.wait())

XCTAssertNoThrow(try eventLoop.flatSubmit {
let bindFuture = eventLoop.flatSubmit {
// this is pretty delicate at the moment:
// `bind` must be _synchronously_ follow `register`, otherwise in our current implementation, `epoll` will
// send us `EPOLLHUP`. To have it run synchronously, we need to invoke the `flatMap` on the eventloop that the
// `register` will succeed.
serverChannel.register().flatMap { () -> EventLoopFuture<()> in
return serverChannel.bind(to: try! SocketAddress(ipAddress: "127.0.0.1", port: 0))
}
}.wait() as Void)
}

// If bind fails, the error will propagate up
try bindFuture.wait()

return serverChannel
}
}
42 changes: 42 additions & 0 deletions Tests/NIOPosixTests/SALChannelTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -322,6 +322,48 @@ final class SALChannelTest: XCTestCase, SALTest {
}
}.salWait())
}

func testBindFailureClosesChannel() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test isn't specific enough: it continues to pass even with the new close0 line removed. It looks like this is because the failed bind promise is caught by the ClientBootstrap in the connect flow and triggers a close0.

The same code appears to be present in the server bootstrap. Can we try to nail down where the bind issue came from in your original reproducer? It doesn't currently look like bind0 needs to close where it does.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, I will be taking a closer look at this over the weekend!

guard let channel = try? self.makeSocketChannel() else {
XCTFail("couldn't make a channel")
return
}
let localAddress = try! SocketAddress(ipAddress: "1.2.3.4", port: 5)
let serverAddress = try! SocketAddress(ipAddress: "9.8.7.6", port: 5)

XCTAssertThrowsError(try channel.eventLoop.runSAL(syscallAssertions: {
try self.assertSetOption(expectedLevel: .tcp, expectedOption: .tcp_nodelay) { value in
return (value as? SocketOptionValue) == 1
}
try self.assertSetOption(expectedLevel: .socket, expectedOption: .so_reuseaddr) { value in
return (value as? SocketOptionValue) == 1
}
try self.assertBind(expectedAddress: localAddress, errorReturn: IOError(errnoCode: EPERM, reason: "bind"))
try self.assertDeregister { selectable in
return true
}
try self.assertClose(expectedFD: .max)

}) {
ClientBootstrap(group: channel.eventLoop)
.channelOption(ChannelOptions.socketOption(.so_reuseaddr), value: 1)
.channelOption(ChannelOptions.autoRead, value: false)
.bind(to: localAddress)
.testOnly_connect(injectedChannel: channel, to: serverAddress)
.flatMapError { error in
guard let ioError = error as? IOError else {
XCTFail("Expected IOError, got \(error)")
return channel.eventLoop.makeFailedFuture(error)
}
XCTAssertEqual(ioError.errnoCode, EPERM, "Expected EPERM error code")
XCTAssertTrue(!channel.isActive, "Channel should be closed")
return channel.eventLoop.makeFailedFuture(error)
}
.flatMap { channel in
channel.close()
}
}.salWait())
}

func testAcceptingInboundConnections() throws {
final class ConnectionRecorder: ChannelInboundHandler {
Expand Down
5 changes: 3 additions & 2 deletions Tests/NIOPosixTests/SyscallAbstractionLayer.swift
Original file line number Diff line number Diff line change
Expand Up @@ -874,9 +874,10 @@ extension SALTest {
}
}

func assertBind(expectedAddress: SocketAddress, file: StaticString = #filePath, line: UInt = #line) throws {
func assertBind(expectedAddress: SocketAddress, errorReturn: IOError? = nil, file: StaticString = #filePath, line: UInt = #line) throws {
SAL.printIfDebug("\(#function)")
try self.selector.assertSyscallAndReturn(.returnVoid, file: (file), line: line) { syscall in

try self.selector.assertSyscallAndReturn(errorReturn != nil ? .error(errorReturn!) : .returnVoid, file: file, line: line) { syscall in
if case .bind(let address) = syscall {
return address == expectedAddress
} else {
Expand Down