Protect against HTT2ChannelHandler reentrancy (#319)

In rare cases the `HTTP2ChannelHandler`s method `unbufferAndFlushAutomaticFrames` and the `HTTP2StreaMultiplexer`s `newConnectionWindowSize` method were recursively calling themselves. This was causing a stack overflow. To protect our selves from this stack overflow I added a variable to protect ourselves against reentrancy inside the `unbufferAndFlushAutomaticFrames` method

Author: FranzBusch

Sources/NIOHTTP2/HTTP2ChannelHandler.swift

@@ -81,6 +81,10 @@ public final class NIOHTTP2Handler: ChannelDuplexHandler {
     /// to determine buffering strategies.
     private var channelWritable: Bool = true
 
+    /// This variable is set to `true` when we are inside `unbufferAndFlushAutomaticFrames` and protects us from reentering the method
+    /// which can cause an infinite recursion.
+    private var isUnbufferingAndFlushingAutomaticFrames = false
+
     /// The mode for this parser to operate in: client or server.
     public enum ParserMode {
         /// Client mode
@@ -568,6 +572,13 @@ extension NIOHTTP2Handler {
     }
 
     private func unbufferAndFlushAutomaticFrames(context: ChannelHandlerContext) {
+        if self.isUnbufferingAndFlushingAutomaticFrames {
+            // Don't allow infinite recursion through this method.
+            return
+        }
+        
+        self.isUnbufferingAndFlushingAutomaticFrames = true
+    
         loop: while true {
             switch self.outboundBuffer.nextFlushedWritableFrame(channelWritable: self.channelWritable) {
             case .noFrame:
@@ -578,6 +589,8 @@ extension NIOHTTP2Handler {
                 self.processOutboundFrame(context: context, frame: frame, promise: promise)
             }
         }
+        
+        self.isUnbufferingAndFlushingAutomaticFrames = false
 
         if self.wroteFrame {
             context.flush()

Tests/NIOHTTP2Tests/ReentrancyTests+XCTest.swift

@@ -30,6 +30,7 @@ extension ReentrancyTests {
                 ("testReEnterReadOnRead", testReEnterReadOnRead),
                 ("testReenterInactiveOnRead", testReenterInactiveOnRead),
                 ("testReenterReadEOFOnRead", testReenterReadEOFOnRead),
+                ("testReenterAutomaticFrames", testReenterAutomaticFrames),
            ]
    }
 }

Tests/NIOHTTP2Tests/ReentrancyTests.swift

@@ -17,6 +17,7 @@ import NIOCore
 import NIOEmbedded
 import NIOHTTP1
 import NIOHTTP2
+import NIOHPACK
 
 /// A `ChannelInboundHandler` that re-entrantly calls into a handler that just passed
 /// it `channelRead`.
@@ -43,6 +44,22 @@ final class ReenterOnReadHandler: ChannelInboundHandler {
     }
 }
 
+final class WindowUpdatedEventHandler: ChannelInboundHandler {
+    public typealias InboundIn = HTTP2Frame
+    public typealias InboundOut = HTTP2Frame
+    public typealias OutboundOut = HTTP2Frame
+
+    init() {
+    }
+    
+    func userInboundEventTriggered(context: ChannelHandlerContext, event: Any) {
+        guard event is NIOHTTP2WindowUpdatedEvent else { return }
+
+        let frame = HTTP2Frame(streamID: .rootStream, payload: .windowUpdate(windowSizeIncrement: 1))
+        context.writeAndFlush(self.wrapOutboundOut(frame), promise: nil)
+    }
+}
+
 // Tests that the HTTP2Parser is safely re-entrant.
 //
 // These tests ensure that we don't ever call into the HTTP/2 session more than once at a time.
@@ -164,4 +181,38 @@ final class ReentrancyTests: XCTestCase {
         XCTAssertNoThrow(try self.clientChannel.finish())
         XCTAssertNoThrow(try self.serverChannel.finish())
     }
+    
+    func testReenterAutomaticFrames() throws {
+        // Start by setting up the connection.
+        try self.basicHTTP2Connection()
+        let windowUpdateFrameHandler = WindowUpdatedEventHandler()
+        XCTAssertNoThrow(try self.serverChannel.pipeline.addHandler(windowUpdateFrameHandler).wait())
+        
+        // Write and flush the header from the client to open a stream
+        let headers = HPACKHeaders([(":path", "/"), (":method", "POST"), (":scheme", "https"), (":authority", "localhost")])
+        let reqFramePayload = HTTP2Frame.FramePayload.headers(.init(headers: headers))
+        self.clientChannel.writeAndFlush(HTTP2Frame(streamID: 1, payload: reqFramePayload), promise: nil)
+        self.interactInMemory(clientChannel, serverChannel)
+        
+        // Write and flush the header from the server
+        let resHeaders = HPACKHeaders([(":status", "200")])
+        let resFramePayload = HTTP2Frame.FramePayload.headers(.init(headers: resHeaders))
+        self.serverChannel.writeAndFlush(HTTP2Frame(streamID: 1, payload: resFramePayload), promise: nil)
+        
+        // Write lots of small data frames and flush them all at once
+        var requestBody = self.serverChannel.allocator.buffer(capacity: 1)
+        requestBody.writeBytes(Array(repeating: UInt8(0x04), count: 1))
+        var reqBodyFramePayload = HTTP2Frame.FramePayload.data(.init(data: .byteBuffer(requestBody)))
+        for _ in 0..<10000 {
+            serverChannel.write(HTTP2Frame(streamID: 1, payload: reqBodyFramePayload), promise: nil)
+        }
+        reqBodyFramePayload = .data(.init(data: .byteBuffer(requestBody), endStream: true))
+        serverChannel.writeAndFlush(HTTP2Frame(streamID: 1, payload: reqBodyFramePayload), promise: nil)
+
+        // Now we can deliver these bytes.
+        self.deliverAllBytes(from: self.clientChannel, to: self.serverChannel)
+
+        XCTAssertNoThrow(try self.clientChannel.finish())
+        XCTAssertNoThrow(try self.serverChannel.finish())
+    }
 }