Handle bind failures when running NIO in a tight sandbox #2616
Conversation
There was a problem hiding this comment.
This file is not related to the PR right?
Sources/NIOCore/Channel.swift
Outdated
|
|
||
| /// An attempt to bind a `Channel` failed. | ||
| case bindFailed |
There was a problem hiding this comment.
Sadly this is a breaking change but I don't think we need this case at all.
Sources/NIOPosix/SocketChannel.swift
Outdated
| self.updateCachedAddressesFromSocket(updateRemote: false) | ||
| try self.socket.listen(backlog: backlog) | ||
| } catch { | ||
| promise?.fail(ChannelError.bindFailed) |
There was a problem hiding this comment.
We should definitely still fail the promise with the error produced by the socket.bind class. Otherwise it is hard to tell what went wrong. Could you explain why this approach works around the assert? I would like to understand how we actually ended up in the assert in the first place since something must have caused the call to readable and we just have to avoid that.
There was a problem hiding this comment.
after re thinking my approach the way I set up my sandbox to test this was not correct, thus nor is this solution. In the actual testChannelInactiveCancelScheduled I set up a class MockServerSocket: ServerSocket {} that essentially always simulate bind failure. However I did this by overriding bind but by doing that I am not hitting all of NIOs binding process.
Now I am overriding the standard bind sys call thats called by NIO by creating a bind function that always returns EPERM
int bind(int sockfd, const struct sockaddr *addr, socklen_t addrlen) {
int (*original_bind)(int, const struct sockaddr *, socklen_t);
original_bind = dlsym(RTLD_NEXT, "bind");
errno = EPERM;
return -1;
}
compile it down to a shared object and preload it and run the test. Going to use this approach to create a new solution here.
In the actual testChannelInactiveCancelScheduled we call readable since we want to test whether scheduled reads are closed when the channel is closed. The issue here is readable checks if the lifecycleManager is active, however at this stage it's only transitioned from fresh to preRegistered thus triggering the assert.
| @@ -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) | |||
There was a problem hiding this comment.
Upon bind failing, the lifecycleManager seems to remain in the preRegistered state. thus we are transiting to the closed state here.
| @@ -884,7 +884,18 @@ extension SALTest { | |||
| } | |||
| } | |||
| } | |||
|
|
|||
|
|
|||
| func assertBindFailure(expectedAddress: SocketAddress, file: StaticString = #filePath, line: UInt = #line) throws { | |||
There was a problem hiding this comment.
Can we fold this into the existing assertBind call, with an optional errorReturn parameter that defaults to nil? That's a little closer to how we typically structure things in the SAL.
| import NIOEmbedded | ||
| import NIOHTTP1 | ||
|
|
||
| private class MessageEndHandler<Head: Equatable, Body: Equatable>: ChannelInboundHandler { |
There was a problem hiding this comment.
Presumably you don't actually want to add this.
…t-nio into fix-sandbox-bind-error
…dbox-bind-error
There was a problem hiding this comment.
LGTM! I would like @Lukasa to have one final looks as well though
| @@ -322,6 +322,48 @@ final class SALChannelTest: XCTestCase, SALTest { | |||
| } | |||
| }.salWait()) | |||
| } | |||
|
|
|||
| func testBindFailureClosesChannel() { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I see, I will be taking a closer look at this over the weekend!
|
@dkz2 are you still interested in pushing this over the line? |
Addressed
AcceptBackoffHandlerTestassertion failure when running NIO in a tight sandbox with bind disallowed.Motivation:
When running SwiftNIO tests in a restricted environment, an assertion error was encountered when bind and other network related operations were disallowed. The tests were expected to fail in such environment, but hitting an assertion was not expected. This change aims to handle such failure gracefully. Notably, the test in
AcceptBackoffHandlerTestare all failing as expected, but it was also hitting an assertion, which was not the intended behavior. This PR closes #2465.Modifications:
XCTAssertNoThrowinsetupChannelto allow bind failures to propagate upwards.close0to release resources and ensure proper channel state management when bind fails.testBindFailureClosesChannelinSALChannelTeststo simulate and test the behavior when a bind operation fails.assertBindinSyscallAbstractionLayerto also account for when a bind operation could fails as expected.Result:
After these changes, all tests in
AcceptBackoffHandlerTestwill fail gracefully instead of hitting an assertion if bind fails.