From 0cac1a6c8c221070b03ee5099d2c87f53ded81c8 Mon Sep 17 00:00:00 2001 From: Moses Nakamura Date: Thu, 7 Dec 2017 16:46:16 -0800 Subject: [PATCH] H2C upgrades should be ineligible for flow control (#7400) H2C upgrades should be ineligible for flow control Motivation: When the h2c upgrade request is too big, the Http2FrameCodec complains it's too big for flow control reasons, even though it's ineligible for flow control. Modifications: Specially mark upgrade streams and make Http2FrameCodec know not to try to flow control on those streams. Result: Servers won't barf when they receive an upgrade request with a fat payload. [Fixes #7280] --- .../handler/codec/http2/Http2FrameCodec.java | 11 +++++ .../http2/InboundHttpToHttp2Adapter.java | 3 ++ .../codec/http2/Http2FrameCodecTest.java | 43 +++++++++++++++++++ 3 files changed, 57 insertions(+) diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2FrameCodec.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2FrameCodec.java index 1b5ecc2eff..ed51306750 100644 --- a/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2FrameCodec.java +++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/Http2FrameCodec.java @@ -145,6 +145,7 @@ public class Http2FrameCodec extends Http2ConnectionHandler { private static final InternalLogger LOG = InternalLoggerFactory.getInstance(Http2FrameCodec.class); private final PropertyKey streamKey; + private final PropertyKey upgradeKey; private final Integer initialFlowControlWindowSize; @@ -161,6 +162,7 @@ public class Http2FrameCodec extends Http2ConnectionHandler { connection().addListener(new ConnectionListener()); connection().remote().flowController().listener(new Http2RemoteFlowControllerListener()); streamKey = connection().newKey(); + upgradeKey = connection().newKey(); initialFlowControlWindowSize = initialSettings.initialWindowSize(); } @@ -247,6 +249,7 @@ public class Http2FrameCodec extends Http2ConnectionHandler { } upgrade.upgradeRequest().headers().setInt( HttpConversionUtil.ExtensionHeaderNames.STREAM_ID.text(), HTTP_UPGRADE_STREAM_ID); + stream.setProperty(upgradeKey, true); InboundHttpToHttp2Adapter.handle( ctx, connection(), decoder().frameListener(), upgrade.upgradeRequest().retain()); } finally { @@ -312,6 +315,14 @@ public class Http2FrameCodec extends Http2ConnectionHandler { final boolean consumeBytes(int streamId, int bytes) throws Http2Exception { Http2Stream stream = connection().stream(streamId); + // upgraded requests are ineligible for stream control + if (streamId == Http2CodecUtil.HTTP_UPGRADE_STREAM_ID) { + Boolean upgraded = stream.getProperty(upgradeKey); + if (Boolean.TRUE.equals(upgraded)) { + return false; + } + } + return connection().local().flowController().consumeBytes(stream, bytes); } diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/InboundHttpToHttp2Adapter.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/InboundHttpToHttp2Adapter.java index 5961ec7db6..b81c665399 100644 --- a/codec-http2/src/main/java/io/netty/handler/codec/http2/InboundHttpToHttp2Adapter.java +++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/InboundHttpToHttp2Adapter.java @@ -49,6 +49,9 @@ public class InboundHttpToHttp2Adapter extends ChannelInboundHandlerAdapter { } } + // note that this may behave strangely when used for the initial upgrade + // message when using h2c, since that message is ineligible for flow + // control, but there is not yet an API for signaling that. static void handle(ChannelHandlerContext ctx, Http2Connection connection, Http2FrameListener listener, FullHttpMessage message) throws Http2Exception { try { diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2FrameCodecTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2FrameCodecTest.java index 78dbc2202c..4d29f5465c 100644 --- a/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2FrameCodecTest.java +++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/Http2FrameCodecTest.java @@ -21,6 +21,7 @@ import io.netty.buffer.UnpooledByteBufAllocator; import io.netty.channel.ChannelFuture; import io.netty.channel.ChannelFutureListener; import io.netty.channel.ChannelHandlerContext; +import io.netty.channel.ChannelInboundHandlerAdapter; import io.netty.channel.ChannelPromise; import io.netty.channel.embedded.EmbeddedChannel; import io.netty.handler.codec.UnsupportedMessageTypeException; @@ -37,6 +38,7 @@ import io.netty.handler.codec.http2.Http2Stream.State; import io.netty.handler.logging.LogLevel; import io.netty.util.AbstractReferenceCounted; import io.netty.util.AsciiString; +import io.netty.util.ReferenceCountUtil; import io.netty.util.ReferenceCounted; import io.netty.util.concurrent.DefaultPromise; import io.netty.util.concurrent.GlobalEventExecutor; @@ -666,6 +668,47 @@ public class Http2FrameCodecTest { assertEquals(1, upgradeEvent.refCnt()); } + @Test + public void upgradeWithoutFlowControlling() throws Exception { + channel.pipeline().addAfter(http2HandlerCtx.name(), null, new ChannelInboundHandlerAdapter() { + @Override + public void channelRead(final ChannelHandlerContext ctx, Object msg) throws Exception { + if (msg instanceof Http2DataFrame) { + // Simulate consuming the frame and update the flow-controller. + Http2DataFrame data = (Http2DataFrame) msg; + ctx.writeAndFlush(new DefaultHttp2WindowUpdateFrame(data.initialFlowControlledBytes()) + .stream(data.stream())).addListener(new ChannelFutureListener() { + @Override + public void operationComplete(ChannelFuture future) throws Exception { + Throwable cause = future.cause(); + if (cause != null) { + ctx.fireExceptionCaught(cause); + } + } + }); + } + ReferenceCountUtil.release(msg); + } + }); + + frameListener.onHeadersRead(http2HandlerCtx, Http2CodecUtil.HTTP_UPGRADE_STREAM_ID, request, 31, false); + + // Using reflect as the constructor is package-private and the class is final. + Constructor constructor = + UpgradeEvent.class.getDeclaredConstructor(CharSequence.class, FullHttpRequest.class); + + // Check if we could make it accessible which may fail on java9. + Assume.assumeTrue(ReflectionUtil.trySetAccessible(constructor) == null); + + String longString = new String(new char[70000]).replace("\0", "*"); + DefaultFullHttpRequest request = + new DefaultFullHttpRequest(HttpVersion.HTTP_1_1, HttpMethod.GET, "/", bb(longString)); + + HttpServerUpgradeHandler.UpgradeEvent upgradeEvent = constructor.newInstance( + "HTTP/2", request); + channel.pipeline().fireUserEventTriggered(upgradeEvent); + } + private static ChannelPromise anyChannelPromise() { return any(ChannelPromise.class); }