From 7e9824cde253186f7e5f6d5c999e78eb3a448460 Mon Sep 17 00:00:00 2001 From: Norman Maurer Date: Tue, 20 Mar 2018 14:05:20 +0100 Subject: [PATCH] Return failed EventLoopFuture when getOption(...) / setOption(...) is called on a closed Channel Motivation: We should return a failed EventLoopFuture when getOption(...) / setOption(...) is called on a closed Channel as otherwise it may not be safe to modify the Channel ioptions after its closed. Modifications: - Add guards that check if the Channel is still open and if not fail the operation. - Add testcase Result: Calling getOption(...) / setOption(...) on a closed Channel fails. --- Sources/NIO/SocketChannel.swift | 38 +++++++++++++++++++ .../DatagramChannelTests+XCTest.swift | 1 + Tests/NIOTests/DatagramChannelTests.swift | 4 ++ Tests/NIOTests/SocketChannelTest+XCTest.swift | 1 + Tests/NIOTests/SocketChannelTest.swift | 12 ++++++ Tests/NIOTests/TestUtils.swift | 19 ++++++++++ 6 files changed, 75 insertions(+) diff --git a/Sources/NIO/SocketChannel.swift b/Sources/NIO/SocketChannel.swift index 3033a13ac7..52feb0170c 100644 --- a/Sources/NIO/SocketChannel.swift +++ b/Sources/NIO/SocketChannel.swift @@ -284,6 +284,10 @@ class BaseSocketChannel: SelectableChannel, ChannelCore { fileprivate func setOption0(option: T, value: T.OptionType) throws { assert(eventLoop.inEventLoop) + guard isOpen else { + throw ChannelError.ioOnClosedChannel + } + switch option { case _ as SocketOption: let (level, name) = option.value as! (SocketOptionLevel, SocketOptionName) @@ -328,6 +332,10 @@ class BaseSocketChannel: SelectableChannel, ChannelCore { fileprivate func getOption0(option: T) throws -> T.OptionType { assert(eventLoop.inEventLoop) + guard isOpen else { + throw ChannelError.ioOnClosedChannel + } + switch option { case _ as SocketOption: let (level, name) = option.value as! (SocketOptionLevel, SocketOptionName) @@ -802,6 +810,11 @@ final class SocketChannel: BaseSocketChannel { override fileprivate func setOption0(option: T, value: T.OptionType) throws { assert(eventLoop.inEventLoop) + + guard isOpen else { + throw ChannelError.ioOnClosedChannel + } + switch option { case _ as ConnectTimeoutOption: connectTimeout = value as? TimeAmount @@ -818,6 +831,11 @@ final class SocketChannel: BaseSocketChannel { override fileprivate func getOption0(option: T) throws -> T.OptionType { assert(eventLoop.inEventLoop) + + guard isOpen else { + throw ChannelError.ioOnClosedChannel + } + switch option { case _ as ConnectTimeoutOption: return connectTimeout as! T.OptionType @@ -1046,6 +1064,11 @@ final class ServerSocketChannel: BaseSocketChannel { override fileprivate func setOption0(option: T, value: T.OptionType) throws { assert(eventLoop.inEventLoop) + + guard isOpen else { + throw ChannelError.ioOnClosedChannel + } + switch option { case _ as BacklogOption: backlog = value as! Int32 @@ -1056,6 +1079,11 @@ final class ServerSocketChannel: BaseSocketChannel { override fileprivate func getOption0(option: T) throws -> T.OptionType { assert(eventLoop.inEventLoop) + + guard isOpen else { + throw ChannelError.ioOnClosedChannel + } + switch option { case _ as BacklogOption: return backlog as! T.OptionType @@ -1212,6 +1240,11 @@ final class DatagramChannel: BaseSocketChannel { override fileprivate func setOption0(option: T, value: T.OptionType) throws { assert(eventLoop.inEventLoop) + + guard isOpen else { + throw ChannelError.ioOnClosedChannel + } + switch option { case _ as WriteSpinOption: pendingWrites.writeSpinCount = value as! UInt @@ -1224,6 +1257,11 @@ final class DatagramChannel: BaseSocketChannel { override fileprivate func getOption0(option: T) throws -> T.OptionType { assert(eventLoop.inEventLoop) + + guard isOpen else { + throw ChannelError.ioOnClosedChannel + } + switch option { case _ as WriteSpinOption: return pendingWrites.writeSpinCount as! T.OptionType diff --git a/Tests/NIOTests/DatagramChannelTests+XCTest.swift b/Tests/NIOTests/DatagramChannelTests+XCTest.swift index 5cc31361f4..bb547fa852 100644 --- a/Tests/NIOTests/DatagramChannelTests+XCTest.swift +++ b/Tests/NIOTests/DatagramChannelTests+XCTest.swift @@ -39,6 +39,7 @@ extension DatagramChannelTests { ("testRecvFromFailsWithECONNREFUSED", testRecvFromFailsWithECONNREFUSED), ("testRecvFromFailsWithENOMEM", testRecvFromFailsWithENOMEM), ("testRecvFromFailsWithEFAULT", testRecvFromFailsWithEFAULT), + ("testSetGetOptionClosedDatagramChannel", testSetGetOptionClosedDatagramChannel), ] } } diff --git a/Tests/NIOTests/DatagramChannelTests.swift b/Tests/NIOTests/DatagramChannelTests.swift index a91cd639e8..4beb085456 100644 --- a/Tests/NIOTests/DatagramChannelTests.swift +++ b/Tests/NIOTests/DatagramChannelTests.swift @@ -403,4 +403,8 @@ final class DatagramChannelTests: XCTestCase { let ioError = try promise.futureResult.wait() XCTAssertEqual(error, ioError.errnoCode) } + + public func testSetGetOptionClosedDatagramChannel() throws { + try assertSetGetOptionOnOpenAndClosed(channel: firstChannel, option: ChannelOptions.maxMessagesPerRead, value: 1) + } } diff --git a/Tests/NIOTests/SocketChannelTest+XCTest.swift b/Tests/NIOTests/SocketChannelTest+XCTest.swift index 5ab1b42438..a512c02864 100644 --- a/Tests/NIOTests/SocketChannelTest+XCTest.swift +++ b/Tests/NIOTests/SocketChannelTest+XCTest.swift @@ -34,6 +34,7 @@ extension SocketChannelTest { ("testAcceptFailsWithENOBUFS", testAcceptFailsWithENOBUFS), ("testAcceptFailsWithENOMEM", testAcceptFailsWithENOMEM), ("testAcceptFailsWithEFAULT", testAcceptFailsWithEFAULT), + ("testSetGetOptionClosedServerSocketChannel", testSetGetOptionClosedServerSocketChannel), ] } } diff --git a/Tests/NIOTests/SocketChannelTest.swift b/Tests/NIOTests/SocketChannelTest.swift index 811a338deb..79b34c42bb 100644 --- a/Tests/NIOTests/SocketChannelTest.swift +++ b/Tests/NIOTests/SocketChannelTest.swift @@ -164,4 +164,16 @@ public class SocketChannelTest : XCTestCase { let ioError = try promise.futureResult.wait() XCTAssertEqual(error, ioError.errnoCode) } + + public func testSetGetOptionClosedServerSocketChannel() throws { + let group = MultiThreadedEventLoopGroup(numThreads: 1) + defer { XCTAssertNoThrow(try group.syncShutdownGracefully()) } + + // Create two channels with different event loops. + let serverChannel = try ServerBootstrap(group: group).bind(host: "127.0.0.1", port: 0).wait() + let clientChannel = try ClientBootstrap(group: group).connect(to: serverChannel.localAddress!).wait() + + try assertSetGetOptionOnOpenAndClosed(channel: clientChannel, option: ChannelOptions.allowRemoteHalfClosure, value: true) + try assertSetGetOptionOnOpenAndClosed(channel: serverChannel, option: ChannelOptions.backlog, value: 100) + } } diff --git a/Tests/NIOTests/TestUtils.swift b/Tests/NIOTests/TestUtils.swift index 4517328628..02bec36c8f 100644 --- a/Tests/NIOTests/TestUtils.swift +++ b/Tests/NIOTests/TestUtils.swift @@ -132,3 +132,22 @@ final class NonAcceptingServerSocket: ServerSocket { return nil } } + +func assertSetGetOptionOnOpenAndClosed(channel: Channel, option: T, value: T.OptionType) throws { + _ = try channel.setOption(option: option, value: value).wait() + _ = try channel.getOption(option: option).wait() + try channel.close().wait() + try channel.closeFuture.wait() + + do { + _ = try channel.setOption(option: option, value: value).wait() + } catch let err as ChannelError where err == .ioOnClosedChannel { + // expected + } + + do { + _ = try channel.getOption(option: option).wait() + } catch let err as ChannelError where err == .ioOnClosedChannel { + // expected + } +}