don't crash when client sends bad magic

Motivation:

We shouldn't just crash if the client sends bad magic.

Changes:

Send an error through the pipeline instead of crashing on bad client
magic.

Result:

fewer crashes

Author: weissi

Sources/NIOHTTP2/HTTP2Error.swift

@@ -12,6 +12,8 @@
 //
 //===----------------------------------------------------------------------===//
 
+import CNIONghttp2
+
 public protocol NIOHTTP2Error: Equatable, Error { }
 
 /// Errors that NIO raises when handling HTTP/2 connections.
@@ -45,6 +47,18 @@ public enum NIOHTTP2Errors {
             self.errorCode = errorCode
         }
     }
+
+    public struct BadClientMagic: NIOHTTP2Error {
+        public init() {}
+    }
+
+    public struct InternalError: NIOHTTP2Error {
+        internal var nghttp2ErrorCode: nghttp2_error
+
+        internal init(nghttp2ErrorCode: nghttp2_error) {
+            self.nghttp2ErrorCode = nghttp2ErrorCode
+        }
+    }
 }
 
 

Sources/NIOHTTP2/HTTP2Parser.swift

@@ -49,6 +49,7 @@ fileprivate class ReentrancyManager {
     enum ProcessingResult {
         case `continue`
         case complete
+        case error(Error)
     }
 
     enum EOFType {
@@ -92,7 +93,16 @@ fileprivate class ReentrancyManager {
         self.processing = true
         defer { self.processing = false }
 
-        while case .continue = processOneOperation(ctx: ctx, body) { }
+        while true {
+            switch processOneOperation(ctx: ctx, body) {
+            case .continue:
+                continue
+            case .complete:
+                return
+            case .error(let error):
+                ctx.fireErrorCaught(error)
+            }
+        }
     }
 
     private func processOneOperation(ctx: ChannelHandlerContext, _ body: (Operation, ChannelHandlerContext) -> ProcessingResult) -> ProcessingResult {
@@ -127,11 +137,12 @@ fileprivate class ReentrancyManager {
 
         // Now we want to send some data. If this returns .continue, we want to spin
         // around keep going. If this returns .complete, we can go ahead and flush.
-        if case .complete = body(.doOneWrite, ctx) {
+        let result = body(.doOneWrite, ctx)
+        if case .complete = result {
             self.mustFlush = false
             return body(.flush, ctx)
         } else {
-            return .continue
+            return result
         }
     }
 }
@@ -239,7 +250,11 @@ public final class HTTP2Parser: ChannelInboundHandler, ChannelOutboundHandler {
     private func process(_ operation: ReentrancyManager.Operation, _ context: ChannelHandlerContext) -> ReentrancyManager.ProcessingResult {
         switch operation {
         case .feedInput(var input):
-            self.session.feedInput(buffer: &input)
+            do {
+                try self.session.feedInput(buffer: &input)
+            } catch {
+                return .error(error)
+            }
             return .continue
         case .feedOutput(let frame, let promise):
             self.session.feedOutput(frame: frame, promise: promise)

Sources/NIOHTTP2/NGHTTP2Session.swift

@@ -573,15 +573,17 @@ class NGHTTP2Session {
         precondition(rc == 0)
     }
 
-    public func feedInput(buffer: inout ByteBuffer) {
-        buffer.withUnsafeReadableBytes { data in
+    public func feedInput(buffer: inout ByteBuffer) throws {
+        try buffer.withUnsafeReadableBytes { data in
             switch nghttp2_session_mem_recv(self.session, data.baseAddress?.assumingMemoryBound(to: UInt8.self), data.count) {
             case let x where x >= 0:
                 precondition(x == data.count, "did not consume all bytes")
             case Int(NGHTTP2_ERR_NOMEM.rawValue):
                 fatalError("out of memory")
-            case let x:
-                fatalError("error \(x)")
+            case Int(NGHTTP2_ERR_BAD_CLIENT_MAGIC.rawValue):
+                throw NIOHTTP2Errors.BadClientMagic()
+            case let nghttp2ErrorCode:
+                throw NIOHTTP2Errors.InternalError(nghttp2ErrorCode: nghttp2_error(rawValue: Int32(nghttp2ErrorCode)))
             }
         }
     }

Tests/NIOHTTP2Tests/BasicTests+XCTest.swift

@@ -27,6 +27,7 @@ extension BasicTests {
    static var allTests : [(String, (BasicTests) -> () throws -> Void)] {
       return [
                 ("testCanInitializeInnerSession", testCanInitializeInnerSession),
+                ("testThrowsErrorOnBasicProtocolViolation", testThrowsErrorOnBasicProtocolViolation),
            ]
    }
 }

Tests/NIOHTTP2Tests/BasicTests.swift

@@ -13,7 +13,10 @@
 //===----------------------------------------------------------------------===//
 
 import XCTest
+
 import NIO
+import CNIONghttp2
+
 @testable import NIOHTTP2
 
 class BasicTests: XCTestCase {
@@ -26,4 +29,24 @@ class BasicTests: XCTestCase {
                                userEventFunction: { _ in })
         XCTAssertNotNil(x)
     }
+
+    func testThrowsErrorOnBasicProtocolViolation() {
+        let session = NGHTTP2Session(mode: .server,
+                                     allocator: ByteBufferAllocator(),
+                                     maxCachedStreamIDs: 1024,
+                                     frameReceivedHandler: { XCTFail("shouldn't have received frame \($0)") },
+                                     sendFunction: { XCTFail("send(\($0), \($1.debugDescription)) shouldn't have been called") },
+                                     userEventFunction: { XCTFail("userEventFunction(\($0)) shouldn't have been called") })
+        var buffer = ByteBufferAllocator().buffer(capacity: 16)
+        buffer.write(staticString: "GET / HTTP/1.1\r\nHost: apple.com\r\n\r\n")
+        XCTAssertThrowsError(try session.feedInput(buffer: &buffer)) { error in
+            switch error {
+            case _ as NIOHTTP2Errors.BadClientMagic:
+                // ok
+                ()
+            default:
+                XCTFail("wrong error \(error) thrown")
+            }
+        }
+    }
 }

Tests/NIOHTTP2Tests/SimpleClientServerTests+XCTest.swift

@@ -57,6 +57,7 @@ extension SimpleClientServerTests {
                 ("testStreamCloseEventForGoawayFiresAfterFrame", testStreamCloseEventForGoawayFiresAfterFrame),
                 ("testManyConcurrentInactiveStreams", testManyConcurrentInactiveStreams),
                 ("testDontRemoveActiveStreams", testDontRemoveActiveStreams),
+                ("testBadClientMagic", testBadClientMagic),
            ]
    }
 }

Tests/NIOHTTP2Tests/SimpleClientServerTests.swift

@@ -1317,4 +1317,50 @@ class SimpleClientServerTests: XCTestCase {
         XCTAssertNoThrow(try self.clientChannel.finish())
         XCTAssertNoThrow(try self.serverChannel.finish())
     }
+
+    func testBadClientMagic() throws {
+        class WaitForErrorHandler: ChannelInboundHandler {
+            typealias InboundIn = Never
+
+            private var errorSeenPromise: EventLoopPromise<Error>?
+
+            init(errorSeenPromise: EventLoopPromise<Error>) {
+                self.errorSeenPromise = errorSeenPromise
+            }
+
+            func channelRead(ctx: ChannelHandlerContext, data: NIOAny) {
+                XCTFail("shouldnt' have received \(data)")
+            }
+
+            func errorCaught(ctx: ChannelHandlerContext, error: Error) {
+                if let errorSeenPromise = self.errorSeenPromise {
+                    errorSeenPromise.succeed(result: error)
+                } else {
+                    XCTFail("extra error \(error) received")
+                }
+            }
+        }
+
+        let errorSeenPromise: EventLoopPromise<Error> = self.clientChannel.eventLoop.newPromise()
+        XCTAssertNoThrow(try self.serverChannel.pipeline.add(handler: HTTP2Parser(mode: .server)).wait())
+        XCTAssertNoThrow(try self.serverChannel.pipeline.add(handler: WaitForErrorHandler(errorSeenPromise: errorSeenPromise)).wait())
+
+        self.clientChannel?.pipeline.fireChannelActive()
+        self.serverChannel?.pipeline.fireChannelActive()
+
+        var buffer = self.clientChannel.allocator.buffer(capacity: 16)
+        buffer.write(staticString: "GET / HTTP/1.1\r\nHost: apple.com\r\n\r\n")
+        XCTAssertNoThrow(try self.clientChannel.writeAndFlush(buffer).wait())
+
+        self.interactInMemory(self.clientChannel, self.serverChannel)
+
+        XCTAssertNoThrow(try XCTAssertEqual(NIOHTTP2Errors.BadClientMagic(),
+                                            errorSeenPromise.futureResult.wait() as? NIOHTTP2Errors.BadClientMagic))
+        let clientReceived: ByteBuffer? = self.clientChannel.readInbound()
+        XCTAssertNotNil(clientReceived)
+
+        XCTAssertNoThrow(try XCTAssertFalse(self.clientChannel.finish()))
+        XCTAssertNoThrow(try XCTAssertFalse(self.serverChannel.finish()))
+    }
+
 }