http2: wrong Last-Stream-Id for connection error during header parsing

[ejona86]

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, 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, 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.

[normanmaurer]

@ejona86 this is the best I can think of as well... I guess this is "good enough" ?

[ejona86]

I think it will be good enough. I'll work on a fix after I'm back in the office on the 26th.

[normanmaurer]

ok cool... ping me once I should review some code

[ejona86]

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.

[normanmaurer]

I think I would go for 1) to keep it simple... 2) doesn't sound too bad as well.