Avoid keeping hold of partial bytes forever. (#984)

Motivation:

The HTTPDecoder is a complex object that has very careful state management goals. One source of this
complexity is that it is fed a stream of bytes with arbitrary chunk sizes, but needs to produce a
collection of objects that are contiguous in memory. For example, each header field name and value
must be turned into a String, which requires a contiguous sequence of bytes to do.

As a result, it is quite common to have a situation where the HTTPDecoder has only *part* of an
object that must be emitted atomically. In this situation, the HTTPDecoder would like to instruct
its ByteToMessageHandler to keep hold of the bytes that form the beginning of that object. To avoid
asking http_parser to parse those bytes twice, the HTTPDecoder uses a value called httpParserOffset
to keep track.

As an example, consider what would happen if the "Connection: keep-alive\r\n" header field was delivered
in two chunks: first "Connection: keep-al", and then "ive\r\n". The header field name can be emitted in
its entirety, but the partial field value must be preserved. To achieve this, the HTTPDecoder will store
an offset internally to keep track of which bytes have been parsed. In this case, the offset will be set
to 7: the number of bytes in "keep-al". It will then tell the rest of the code that only 12 bytes of the
original 19 byte message were consumed, causing the ByteToMessageHandler to preserve those 7 bytes.

However, when the next chunk is received, the ByteToMessageHandler will *replay* those bytes to
HTTPDecoder. To avoid parsing them a second time, HTTPDecoder keeps track of how many bytes it is
expecting to see replayed. This is the value in httpParserOffset.

Due to a logic error in the HTTPDecoder, the httpParserOffset field was never returned to zero.
This field would be modified whenever a partial field was received, but would never be returned
to zero when a complete message was parsed. This would cause the HTTPDecoder to unnecessarily keep
hold of extra bytes in the ByteToMessageHandler even when they were no longer needed. In some cases
the number could get smaller, such as when a new partial field was received, but it could never drop
to zero even when a complete HTTP message was receivedincremented.

Happily, due to the rest of the HTTPDecoder logic this never produced an invalid message: while
ByteToMessageHandler was repeatedly producing extra bytes, it never actually passed them to http_parser
again, or caused any other issue. The only situation in which a problem would occur is if the HTTPDecoder
had a RemoveAfterUpgradeStrategy other than .dropBytes. In that circumstance, decodeLast would not
consume any extra bytes, but those bytes would have remained in the buffer passed to decodeLast, which
would then incorrectly *forward them on*. This is the only circumstance in which this error manifested,
and in most applications it led to surprising and irregular crashes on connection teardown. In all
other applications the only effect was unnecessarily preserving a few tens of extra bytes on
some connections, until receiving EOF caused us to drop all that memory anyway.

Modifications:

- Return httpParserOffset to 0 when a full message has been delivered.

Result:

Fewer weird crashes.

(cherry picked from commit ae3d298)

Sources/NIOHTTP1/HTTPDecoder.swift

@@ -366,7 +366,15 @@ private class BetterHTTPParser {
             self.firstNonDiscardableOffset = 0
             return firstNonDiscardableOffset
         } else {
-            return parserConsumed
+            // By definition we've consumed all of the http parser offset at this stage. There may still be bytes
+            // left in the buffer though: we didn't consume them because they aren't ours to consume, as they may belong
+            // to an upgraded protocol.
+            //
+            // Set the HTTP parser offset back to zero, and tell the parent that we consumed
+            // the whole buffer.
+            let consumedBytes = self.httpParserOffset + parserConsumed
+            self.httpParserOffset = 0
+            return consumedBytes
         }
     }
 }

Tests/NIOHTTP1Tests/HTTPDecoderTest+XCTest.swift

@@ -43,6 +43,7 @@ extension HTTPDecoderTest {
                 ("testExtraCarriageReturnBetweenSubsequentRequests", testExtraCarriageReturnBetweenSubsequentRequests),
                 ("testIllegalHeaderNamesCauseError", testIllegalHeaderNamesCauseError),
                 ("testNonASCIIWorksAsHeaderValue", testNonASCIIWorksAsHeaderValue),
+                ("testDoesNotDeliverLeftoversUnnecessarily", testDoesNotDeliverLeftoversUnnecessarily),
            ]
    }
 }

Tests/NIOHTTP1Tests/HTTPDecoderTest.swift

@@ -516,4 +516,22 @@ class HTTPDecoderTest: XCTestCase {
         XCTAssertNoThrow(XCTAssertEqual(.head(expectedHead),
                                         try writeToFreshRequestDecoderChannel("GET / HTTP/1.1\r\nfoo: bär\r\n\r\n")))
     }
+
+    func testDoesNotDeliverLeftoversUnnecessarily() {
+        // This test isolates a nasty problem where the http parser offset would never be reset to zero. This would cause us to gradually leak
+        // very small amounts of memory on each connection, or sometimes crash.
+        let data: StaticString = "GET / HTTP/1.1\r\nHost: localhost\r\n\r\n"
+
+        let channel = EmbeddedChannel()
+        var dataBuffer = channel.allocator.buffer(capacity: 128)
+        dataBuffer.writeStaticString(data)
+
+        XCTAssertNoThrow(try channel.pipeline.addHandler(ByteToMessageHandler(HTTPRequestDecoder(leftOverBytesStrategy: .fireError))).wait())
+        XCTAssertNoThrow(try channel.writeInbound(dataBuffer.getSlice(at: 0, length: dataBuffer.readableBytes - 6)!))
+        XCTAssertNoThrow(try channel.writeInbound(dataBuffer.getSlice(at: dataBuffer.readableBytes - 6, length: 6)!))
+
+        XCTAssertNoThrow(try channel.throwIfErrorCaught())
+        channel.pipeline.fireChannelInactive()
+        XCTAssertNoThrow(try channel.throwIfErrorCaught())
+    }
 }