Avoid a spin loop when inactive is fired before active (#470)

Motivation:

The NIOSSLHandler can enter a spin loop in `doUnbufferActions` if
writing data into BoringSSL fails with SSL_ERROR_WANT_READ or
SSL_ERROR_WANT_WRITE.

One way these errors can happen is if a write into BoringSSL happens
prior to the handshake completing. The NIOSSLHandler is explicit about
starting the handshake: it's done in channel active and in handler added
if the channel is already active.

However, the handshaking step is currently done without any state
checking, so if the state is 'closed' (as it would be after channel
inactive) then the handshake will still start. This can happen if
channel inactive happens before channel active.

To reach the write loop there must be a buffered write and flush prior
to the handshake step starting and the state isn't 'idle' or
'handshaking'. This can happen if a write and flush hapens while
'NIOSSLHandler' is in 'channelActive' (it forwards the 'channelActive'
event _before_ starting the handshake) and 'channelInactive' came first.

Modifications:

- Early exit from 'doHandshakeStep' if the state isn't applicable for
starting a handshake.
- Don't buffer writes when the state is 'closed' as they'll never
succeed and can be failed immediately.
- If the write isn't succesful in 'doUnbufferActions' then either write
to the network or try reading.
- Only allow a limited number of spins through 'doUnbufferActions'

Result:

- Resolves #467

Sources/NIOSSL/NIOSSLHandler.swift

@@ -351,7 +351,15 @@ public class NIOSSLHandler : ChannelInboundHandler, ChannelOutboundHandler, Remo
     ///
     /// This method must not be called once the connection is established.
     private func doHandshakeStep(context: ChannelHandlerContext) {
-        let result = connection.doHandshake()
+        switch self.state {
+        case .unwrapped, .inputClosed, .outputClosed, .closed:
+            // We shouldn't be handshaking in any of these state.
+            return
+        case .idle, .handshaking, .additionalVerification, .active, .closing, .unwrapping:
+            ()
+        }
+
+        let result = self.connection.doHandshake()
 
         switch result {
         case .incomplete:
@@ -872,9 +880,15 @@ extension NIOSSLHandler {
     }
 
     private func bufferWrite(data: ByteBuffer, promise: EventLoopPromise<Void>?) {
-        if case .outputClosed = self.state {
+        switch self.state {
+        case .idle, .handshaking, .additionalVerification, .active, .unwrapping, .closing, .unwrapped, .inputClosed:
+            ()
+        case .outputClosed:
             promise?.fail(ChannelError.outputClosed)
             return
+        case .closed:
+            promise?.fail(ChannelError.ioOnClosedChannel)
+            return
         }
 
         var data = data
@@ -914,41 +928,58 @@ extension NIOSSLHandler {
         // These are some annoying variables we use to persist state across invocations of
         // our closures. A better version of this code might be able to simplify this somewhat.
         var promises: [EventLoopPromise<Void>] = []
-        var didWrite = false
 
         do {
             var invokeCloseOutput = false
-            bufferedActionsLoop: while bufferedActions.hasMark {
-                let element = bufferedActions.first!
-                switch element {
-                case .write(let bufferedWrite):
-                    var data = bufferedWrite.data
-                    let writeSuccessful = try self._encodeSingleWrite(buf: &data)
-                    if writeSuccessful {
-                        didWrite = true
-                        if let promise = bufferedWrite.promise { promises.append(promise) }
-                        _ = bufferedActions.removeFirst()
+            var bufferedActionsLoopCount = 0
+            bufferedActionsLoop: while self.bufferedActions.hasMark, bufferedActionsLoopCount < 1000 {
+                bufferedActionsLoopCount += 1
+                var didWrite = false
+
+                writeLoop: while self.bufferedActions.hasMark {
+                    let element = self.bufferedActions.first!
+                    switch element {
+                    case .write(let bufferedWrite):
+                        var data = bufferedWrite.data
+                        let writeSuccessful = try self._encodeSingleWrite(buf: &data)
+                        if writeSuccessful {
+                            didWrite = true
+                            if let promise = bufferedWrite.promise { promises.append(promise) }
+                            _ = self.bufferedActions.removeFirst()
+                        } else {
+                            // The write into BoringSSL unsuccessful. Break the write loop so any
+                            // data is written to the network before resuming.
+                            break writeLoop
+                        }
+                    case .closeOutput:
+                        invokeCloseOutput = true
+                        _ = self.bufferedActions.removeFirst()
+                        break writeLoop
                     }
-                case .closeOutput:
-                    invokeCloseOutput = true
-                    _ = bufferedActions.removeFirst()
-                    break bufferedActionsLoop
                 }
-            }
 
-            // If we got this far and did a write, we should shove the data out to the
-            // network.
-            if didWrite {
-                let ourPromise: EventLoopPromise<Void>? = promises.flattenPromises(on: context.eventLoop)
-                self.writeDataToNetwork(context: context, promise: ourPromise)
+                // If we got this far and did a write, we should shove the data out to the
+                // network.
+                if didWrite {
+                    let ourPromise: EventLoopPromise<Void>? = promises.flattenPromises(on: context.eventLoop)
+                    self.writeDataToNetwork(context: context, promise: ourPromise)
+                }
+
+                // We detected a .closeOutput action in our action buffer. This means we
+                // close the output after we have written all pending writes.
+                if invokeCloseOutput {
+                    self.state = .outputClosed
+                    self.doShutdownStep(context: context)
+                    self.discardBufferedActions(reason: ChannelError.outputClosed)
+                    break bufferedActionsLoop
+                }
             }
 
-            // We detected a .closeOutput action in our action buffer. This means we
-            // close the output after we have written all pending writes.
-            if invokeCloseOutput {
-                self.state = .outputClosed
-                self.doShutdownStep(context: context)
-                self.discardBufferedActions(reason: ChannelError.outputClosed)
+            // We spun the outer loop too many times, something isn't right so let's bail out
+            // instead of looping any longer.
+            if bufferedActionsLoopCount >= 1000 {
+                assertionFailure("\(#function) looped too many times, please file a GitHub issue against swift-nio-ssl.")
+                throw NIOSSLExtraError.noForwardProgress
             }
         } catch {
             // We encountered an error, it's cleanup time. Close ourselves down.

Sources/NIOSSL/SSLErrors.swift

@@ -200,6 +200,7 @@ extension NIOSSLExtraError {
         case cannotUseIPAddressInSNI
         case invalidSNIHostname
         case unknownPrivateKeyFileType
+        case noForwardProgress
     }
 }
 
@@ -228,6 +229,12 @@ extension NIOSSLExtraError {
     /// The private key file for the TLS configuration has an unknown type.
     public static let unknownPrivateKeyFileType = NIOSSLExtraError(baseError: .unknownPrivateKeyFileType, description: nil)
 
+    /// No forward progress is being made.
+    ///
+    /// This can happen when the `NIOSSLHandler` is unbuffering actions and gets into a state where
+    /// it would potentially spin loop indefinitely.
+    static let noForwardProgress = NIOSSLExtraError(baseError: .noForwardProgress, description: nil)
+
     @inline(never)
     internal static func failedToValidateHostname(expectedName: String) -> NIOSSLExtraError {
         let description = "Couldn't find \(expectedName) in certificate from peer"

Tests/NIOSSLTests/NIOSSLIntegrationTest.swift

@@ -2826,4 +2826,59 @@ class NIOSSLIntegrationTest: XCTestCase {
         b2b.client.close(promise: nil)
         try b2b.interactInMemory()
     }
+
+    func testDoesNotSpinLoopWhenInactiveAndActiveAreReversed() throws {
+        // This is a regression test for https://github.com/apple/swift-nio-ssl/issues/467
+        //
+        // If channelInactive occurs before channelActive and a re-entrant write and flush occurred
+        // in channelActive then 'NIOSSLHandler.doUnbufferActions(context:)' would loop
+        // indefinitely.
+        let eventLoop = EmbeddedEventLoop()
+        let promise = eventLoop.makePromise(of: Void.self)
+
+        final class WriteAndFlushOnActive: ChannelInboundHandler {
+            typealias InboundIn = ByteBuffer
+            typealias OutboundOut = ByteBuffer
+
+            private let promise: EventLoopPromise<Void>
+
+            init(promise: EventLoopPromise<Void>) {
+                self.promise = promise
+            }
+
+            func channelActive(context: ChannelHandlerContext) {
+                let buffer = context.channel.allocator.buffer(string: "You spin me right 'round")
+                context.writeAndFlush(self.wrapOutboundOut(buffer), promise: self.promise)
+                context.fireChannelActive()
+            }
+        }
+
+        let context = try self.configuredSSLContext()
+        let handler = try NIOSSLClientHandler(context: context, serverHostname: nil)
+        let channel = EmbeddedChannel(
+            handlers: [handler, WriteAndFlushOnActive(promise: promise)],
+            loop: eventLoop
+        )
+
+        // Close _before_ channel active. This shouldn't (but can https://github.com/apple/swift-nio/issues/2773)
+        // happen for 'real' channels by synchronously closing the channel when the connect promise
+        // is succeeded.
+        channel.pipeline.fireChannelInactive()
+        channel.pipeline.fireChannelActive()
+
+        // The handshake starts in channelActive (and handlerAdded if the channel is already
+        // active). If the events are reordered then the handshake shouldn't start and there
+        // shouldn't be any outbound data.
+        XCTAssertNil(try channel.readOutbound(as: ByteBuffer.self))
+
+        // The write promise should fail.
+        XCTAssertThrowsError(try promise.futureResult.wait()) { error in
+            XCTAssertEqual(error as? ChannelError, .ioOnClosedChannel)
+        }
+
+        // Subsequent writes should also fail.
+        XCTAssertThrowsError(try channel.writeOutbound(ByteBuffer(string: "Like a record, baby, right 'round"))) { error in
+            XCTAssertEqual(error as? ChannelError, .ioOnClosedChannel)
+        }
+    }
 }