Repository: netty/netty (Java)

Issue:
http2: wrong Last-Stream-Id for connection error during header parsing
### Expected behavior

A channel error occurring during HEADERS decoding triggers a GOAWAY that includes the stream id including the stream for the header being processed.

### Actual behavior

Streams are allocated within `DefaultHttp2ConnectionDecoder.onHeadersRead()`. If a channel error occurs during header parsing within `DefaultHttp2FrameReader`, `DefaultHttp2ConnectionDecoder.onHeadersRead()` is not called. When the exception propagates to [`Http2ConnectionHandler.onConnectionError()` which triggers goAway](https://github.com/netty/netty/blob/d01471917b94d15fbe8b3b2e0f0ed8f24ee2954a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionHandler.java#L672), it uses `connection().remote().lastStreamCreated()` for the Last-Stream-Id which does not include the stream being processed.

This exposes the netty server to clients processing the stream closure similar to REFUSED_STREAM and transparently replaying the request on a new connection.

### Steps to reproduce

Send large enough HEADERS (+CONTINUATIONS) such that the encoded form exceeds the MAX_HEADER_LIST_SIZE. [In my test](https://github.com/grpc/grpc-java/blob/f9b428ab40cd6eec2f45dce15a16ff0c9390fbb2/netty/src/test/java/io/grpc/netty/NettyClientTransportTest.java#L538), I used a max size of 1 byte. The server-side exception triggered, for reference:

```
io.netty.handler.codec.http2.Http2Exception: Header size exceeded max allowed size (1)
	at io.netty.handler.codec.http2.Http2Exception.connectionError(Http2Exception.java:103)
	at io.netty.handler.codec.http2.Http2CodecUtil.headerListSizeExceeded(Http2CodecUtil.java:245)
	at io.netty.handler.codec.http2.DefaultHttp2FrameReader$HeadersBlockBuilder.headerSizeExceeded(DefaultHttp2FrameReader.java:694)
	at io.netty.handler.codec.http2.DefaultHttp2FrameReader$HeadersBlockBuilder.addFragment(DefaultHttp2FrameReader.java:710)
	at io.netty.handler.codec.http2.DefaultHttp2FrameReader$1.processFragment(DefaultHttp2FrameReader.java:455)
	at io.netty.handler.codec.http2.DefaultHttp2FrameReader.readHeadersFrame(DefaultHttp2FrameReader.java:464)
	at io.netty.handler.codec.http2.DefaultHttp2FrameReader.processPayloadState(DefaultHttp2FrameReader.java:254)
	at io.netty.handler.codec.http2.DefaultHttp2FrameReader.readFrame(DefaultHttp2FrameReader.java:160)
	at io.netty.handler.codec.http2.Http2InboundFrameLogger.readFrame(Http2InboundFrameLogger.java:41)
	at io.netty.handler.codec.http2.DefaultHttp2ConnectionDecoder.decodeFrame(DefaultHttp2ConnectionDecoder.java:174)
	at io.netty.handler.codec.http2.Http2ConnectionHandler$FrameDecoder.decode(Http2ConnectionHandler.java:378)
	at io.netty.handler.codec.http2.Http2ConnectionHandler.decode(Http2ConnectionHandler.java:438)
	at io.netty.handler.codec.ByteToMessageDecoder.decodeRemovalReentryProtection(ByteToMessageDecoder.java:501)
	at io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:440)
	at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:276)
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:379)
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:365)
	at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:357)
	at io.netty.handler.ssl.SslHandler.unwrap(SslHandler.java:1526)
	at io.netty.handler.ssl.SslHandler.decodeJdkCompatible(SslHandler.java:1275)
	at io.netty.handler.ssl.SslHandler.decode(SslHandler.java:1322)
	at io.netty.handler.codec.ByteToMessageDecoder.decodeRemovalReentryProtection(ByteToMessageDecoder.java:501)
	at io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:440)
	at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:276)
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:379)
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:365)
	at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:357)
	at io.netty.channel.DefaultChannelPipeline$HeadContext.channelRead(DefaultChannelPipeline.java:1410)
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:379)
	at io.netty.channel.AbstractChannelHandlerContext.invokeChannelRead(AbstractChannelHandlerContext.java:365)
	at io.netty.channel.DefaultChannelPipeline.fireChannelRead(DefaultChannelPipeline.java:919)
	at io.netty.channel.nio.AbstractNioByteChannel$NioByteUnsafe.read(AbstractNioByteChannel.java:163)
	at io.netty.channel.nio.NioEventLoop.processSelectedKey(NioEventLoop.java:714)
	at io.netty.channel.nio.NioEventLoop.processSelectedKeysOptimized(NioEventLoop.java:650)
	at io.netty.channel.nio.NioEventLoop.processSelectedKeys(NioEventLoop.java:576)
	at io.netty.channel.nio.NioEventLoop.run(NioEventLoop.java:493)
	at io.netty.util.concurrent.SingleThreadEventExecutor$4.run(SingleThreadEventExecutor.java:989)
	at io.netty.util.internal.ThreadExecutorMap$2.run(ThreadExecutorMap.java:74)
	at io.netty.util.concurrent.FastThreadLocalRunnable.run(FastThreadLocalRunnable.java:30)
	at java.lang.Thread.run(Thread.java:748)
```

### Netty version

4.1.51.Final

------

I'm open for ideas on how to resolve. We could have a catch in `DefaultHttp2FrameReader.readHeadersFrame()` that calls `DefaultHttp2Connection.remote().incrementExpectedSteramId()` before rethrowing. That seems sort of wrong, but we expect the connection to be destroyed so maybe it makes sense.


Hints:
@ejona86 this is the best I can think of as well... I guess this is "good enough" ? 
I think it will be good enough. I'll work on a fix after I'm back in the office on the 26th.
ok cool... ping me once I should review some code 
I don't see a good way to update `incrementExpectedStreamId()` from `DefaultHttp2FramReader`. It can't call the listener to do it and when an exception is thrown the caller doesn't know which frame it was processing nor the streamid for it.

Again, "I don't see a good way." But I see several variations via exceptions:
1. Http2ConnectionHandler is pessimistic and treats all connection errors with ShutdownHint.HARD_SHUTDOWN as a possible header parsing failure. Since it doesn't know the proper stream id it uses MAX_INT for the GOAWAY. This is safe, but obviously can cause unnecessary errors for streams that were racing on the wire
2. Introduce a new ShutdownHint.ALL_BETS_OFF_SHUTDOWN. Http2ConnectionHandler processes this hint as in (1), using MAX_INT for the GOAWAY. Http2FrameReader can catch any connection error and wrap it with a new exception with ALL_BETS_OFF_SHUTDOWN. Compared to (1), this reduces the number of failures that will use the MAX_INT GOAWAY behavior. A variation on this approach would be to create a new exception type or introduce a new field
3. Introduce a "processingStreamId" field to Http2Exception. It would be able to be set by anyone after the exception is created. DefaultHttp2FrameReader would set the value when the exception is propagating. Http2ConnectionHandler would take the max of the processingStreamId in the exception and the current `remote().lastStreamCreated()` when sending the GOAWAY. This won't work for "static" exceptions

We could change some interfaces:

4. We could add a method to both Http2ConnectionDecoder and Http2FrameReader to return the largest stream id seen. Http2ConnectionHandler would take the max of this method and the current `remote().lastStreamCreated()` when sending the GOAWAY. This breaks some interfaces. We could introduce new interfaces and have instanceof checks in Http2ConnectionHandler and Http2ConnectionDecoder; if those checks fail we could use MAX_INT
5. We could add a method to both Http2ConnectionDecoder and Http2FrameReader to set the Http2Connection. Http2FrameReader could then create a stream manually. This is a clear layering violation. Maybe we could just do this hack on DefaultHttp2FrameReader so no interfaces would change, but it'd still be a layering violation
6. Add a method to the listener, to allow creating the stream without any headers. DefaultHttp2FrameReader would guarantee to either call the normal onHeadersRead or this onUnprocessableHeadersRead. This breaks an interface; I don't see a way around that with instanceof checks

Or we can investigate "overly creative ideas:"

7. If a connection error is triggered during Http2FrameReader.readFrame(), throw a stream error. The next call to Http2FrameReader.readFrame() throw the connection error, and just let the GOAWAY work like it does today. It's unclear how much confusion this would cause on-the-wire, but no specific problems come to mind

I'm not wild about any of them, but my current preference is toward (1), as it is simple (like 3 lines of code), and these HARD_SHUTDOWN cases should only happen when there are other horrible errors going on. At the very least it is very quick to implement and would "stop the bleeding" and give us time to fix things up "properly"... assuming we had an idea of what "properly" is, which I've got no clue.

(2) is probably my second pick, but it does further complicate Http2Exception.
I think I would go for 1) to keep it simple... 2) doesn't sound too bad as well. 

Patch:
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionHandler.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionHandler.java
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionHandler.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionHandler.java
@@ -876,7 +876,16 @@ private void checkCloseConnection(ChannelFuture future) {
      */
     private ChannelFuture goAway(ChannelHandlerContext ctx, Http2Exception cause, ChannelPromise promise) {
         long errorCode = cause != null ? cause.error().code() : NO_ERROR.code();
-        int lastKnownStream = connection().remote().lastStreamCreated();
+        int lastKnownStream;
+        if (cause != null && cause.shutdownHint() == Http2Exception.ShutdownHint.HARD_SHUTDOWN) {
+            // The hard shutdown could have been triggered during header processing, before updating
+            // lastStreamCreated(). Specifically, any connection errors encountered by Http2FrameReader or HPACK
+            // decoding will fail to update the last known stream. So we must be pessimistic.
+            // https://github.com/netty/netty/issues/10670
+            lastKnownStream = Integer.MAX_VALUE;
+        } else {
+            lastKnownStream = connection().remote().lastStreamCreated();
+        }
         return goAway(ctx, lastKnownStream, errorCode, Http2CodecUtil.toByteBuf(ctx, cause), promise);
     }
 


Test patch:
diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ConnectionHandlerTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ConnectionHandlerTest.java
--- a/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ConnectionHandlerTest.java
+++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ConnectionHandlerTest.java
@@ -309,7 +309,7 @@ public void serverReceivingInvalidClientPrefaceStringShouldHandleException() thr
         handler = newHandler();
         handler.channelRead(ctx, copiedBuffer("BAD_PREFACE", UTF_8));
         ArgumentCaptor<ByteBuf> captor = ArgumentCaptor.forClass(ByteBuf.class);
-        verify(frameWriter).writeGoAway(eq(ctx), eq(0), eq(PROTOCOL_ERROR.code()),
+        verify(frameWriter).writeGoAway(eq(ctx), eq(Integer.MAX_VALUE), eq(PROTOCOL_ERROR.code()),
                 captor.capture(), eq(promise));
         assertEquals(0, captor.getValue().refCnt());
     }
@@ -320,7 +320,7 @@ public void serverReceivingHttp1ClientPrefaceStringShouldIncludePreface() throws
         handler = newHandler();
         handler.channelRead(ctx, copiedBuffer("GET /path HTTP/1.1", US_ASCII));
         ArgumentCaptor<ByteBuf> captor = ArgumentCaptor.forClass(ByteBuf.class);
-        verify(frameWriter).writeGoAway(eq(ctx), eq(0), eq(PROTOCOL_ERROR.code()),
+        verify(frameWriter).writeGoAway(eq(ctx), eq(Integer.MAX_VALUE), eq(PROTOCOL_ERROR.code()),
             captor.capture(), eq(promise));
         assertEquals(0, captor.getValue().refCnt());
         assertTrue(goAwayDebugCap.contains("/path"));
@@ -336,7 +336,7 @@ public void serverReceivingClientPrefaceStringFollowedByNonSettingsShouldHandleE
         ByteBuf buf = Unpooled.buffer().writeBytes(connectionPrefaceBuf()).writeZero(10);
         handler.channelRead(ctx, buf);
         ArgumentCaptor<ByteBuf> captor = ArgumentCaptor.forClass(ByteBuf.class);
-        verify(frameWriter, atLeastOnce()).writeGoAway(eq(ctx), eq(0), eq(PROTOCOL_ERROR.code()),
+        verify(frameWriter, atLeastOnce()).writeGoAway(eq(ctx), eq(Integer.MAX_VALUE), eq(PROTOCOL_ERROR.code()),
                 captor.capture(), eq(promise));
         assertEquals(0, captor.getValue().refCnt());
     }
@@ -384,10 +384,13 @@ public void channelInactiveShouldCloseStreams() throws Exception {
     public void connectionErrorShouldStartShutdown() throws Exception {
         handler = newHandler();
         Http2Exception e = new Http2Exception(PROTOCOL_ERROR);
+        // There's no guarantee that lastStreamCreated in correct, as the error could have occurred during header
+        // processing before it was updated. Thus, it should _not_ be used for the GOAWAY.
+        // https://github.com/netty/netty/issues/10670
         when(remote.lastStreamCreated()).thenReturn(STREAM_ID);
         handler.exceptionCaught(ctx, e);
         ArgumentCaptor<ByteBuf> captor = ArgumentCaptor.forClass(ByteBuf.class);
-        verify(frameWriter).writeGoAway(eq(ctx), eq(STREAM_ID), eq(PROTOCOL_ERROR.code()),
+        verify(frameWriter).writeGoAway(eq(ctx), eq(Integer.MAX_VALUE), eq(PROTOCOL_ERROR.code()),
                 captor.capture(), eq(promise));
         captor.getValue().release();
     }
diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ControlFrameLimitEncoderTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ControlFrameLimitEncoderTest.java
--- a/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ControlFrameLimitEncoderTest.java
+++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ControlFrameLimitEncoderTest.java
@@ -266,7 +266,7 @@ private void verifyFlushAndClose(int invocations, boolean failed) {
         verify(ctx, atLeast(invocations)).flush();
         verify(ctx, times(invocations)).close();
         if (failed) {
-            verify(writer, times(1)).writeGoAway(eq(ctx), eq(0), eq(ENHANCE_YOUR_CALM.code()),
+            verify(writer, times(1)).writeGoAway(eq(ctx), eq(Integer.MAX_VALUE), eq(ENHANCE_YOUR_CALM.code()),
                     any(ByteBuf.class), any(ChannelPromise.class));
         }
     }