Repository: netty/netty
Issue: HTTP/2 outbound flow controller negative write size exception
Labels: defect

We are not handling the case where the `writableWindow()` can be negative in the `DefaultHttp2OutboundFlowController` which is resulting in an `IllegalArgumentException` exception when trying to get a negatively sized buffer.

The `writableWindow()` is negative [here](https://github.com/netty/netty/blob/master/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2OutboundFlowController.java#L504) and results in a negative slice [here](https://github.com/netty/netty/blob/master/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2OutboundFlowController.java#L656) which generates the following stack trace:

``` bash
java.lang.IllegalArgumentException: maxCapacity: -2356 (expected: >= 0)
    at io.netty.buffer.AbstractByteBuf.<init>(AbstractByteBuf.java:51)
    at io.netty.buffer.AbstractDerivedByteBuf.<init>(AbstractDerivedByteBuf.java:28)
    at io.netty.buffer.SlicedByteBuf.<init>(SlicedByteBuf.java:40)
    at io.netty.buffer.AbstractByteBuf.slice(AbstractByteBuf.java:910)
    at io.netty.buffer.AbstractByteBuf.readSlice(AbstractByteBuf.java:641)
    at io.netty.handler.codec.http2.DefaultHttp2OutboundFlowController$OutboundFlowState$Frame.split(DefaultHttp2OutboundFlowController.java:656)
    at io.netty.handler.codec.http2.DefaultHttp2OutboundFlowController$OutboundFlowState.writeBytes(DefaultHttp2OutboundFlowController.java:518)
    at io.netty.handler.codec.http2.DefaultHttp2OutboundFlowController$OutboundFlowState.access$500(DefaultHttp2OutboundFlowController.java:379)
    at io.netty.handler.codec.http2.DefaultHttp2OutboundFlowController.writeAllowedBytes(DefaultHttp2OutboundFlowController.java:271)
    at io.netty.handler.codec.http2.DefaultHttp2OutboundFlowController.writeAllowedBytes(DefaultHttp2OutboundFlowController.java:289)
    at io.netty.handler.codec.http2.DefaultHttp2OutboundFlowController.writePendingBytes(DefaultHttp2OutboundFlowController.java:253)
    at io.netty.handler.codec.http2.DefaultHttp2OutboundFlowController.updateOutboundWindowSize(DefaultHttp2OutboundFlowController.java:145)
    at io.netty.handler.codec.http2.DefaultHttp2ConnectionEncoder.updateOutboundWindowSize(DefaultHttp2ConnectionEncoder.java:437)
    at io.netty.handler.codec.http2.DefaultHttp2ConnectionDecoder$FrameReadListener.onWindowUpdateRead(DefaultHttp2ConnectionDecoder.java:500)
    at io.netty.handler.codec.http2.DefaultHttp2FrameReader.readWindowUpdateFrame(DefaultHttp2FrameReader.java:552)
    at io.netty.handler.codec.http2.DefaultHttp2FrameReader.processPayloadState(DefaultHttp2FrameReader.java:243)
    at io.netty.handler.codec.http2.DefaultHttp2FrameReader.readFrame(DefaultHttp2FrameReader.java:126)
    at io.netty.handler.codec.http2.DefaultHttp2ConnectionDecoder.decodeFrame(DefaultHttp2ConnectionDecoder.java:129)
    at io.netty.handler.codec.http2.Http2ConnectionHandler.decode(Http2ConnectionHandler.java:372)
    at io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:248)
    at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:148)
    at io.netty.channel.ChannelHandlerInvokerUtil.invokeChannelReadNow(ChannelHandlerInvokerUtil.java:83)
    at io.netty.channel.DefaultChannelHandlerInvoker.invokeChannelRead(DefaultChannelHandlerInvoker.java:153)
    at io.netty.channel.PausableChannelEventExecutor.invokeChannelRead(PausableChannelEventExecutor.java:86)
    at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:390)
    at io.netty.handler.ssl.SslHandler.unwrap(SslHandler.java:936)
    at io.netty.handler.ssl.SslHandler.decode(SslHandler.java:819)
    at io.netty.handler.codec.ByteToMessageDecoder.callDecode(ByteToMessageDecoder.java:248)
    at io.netty.handler.codec.ByteToMessageDecoder.channelRead(ByteToMessageDecoder.java:148)
    at io.netty.channel.ChannelHandlerInvokerUtil.invokeChannelReadNow(ChannelHandlerInvokerUtil.java:83)
    at io.netty.channel.DefaultChannelHandlerInvoker.invokeChannelRead(DefaultChannelHandlerInvoker.java:153)
    at io.netty.channel.PausableChannelEventExecutor.invokeChannelRead(PausableChannelEventExecutor.java:86)
    at io.netty.channel.AbstractChannelHandlerContext.fireChannelRead(AbstractChannelHandlerContext.java:390)
    at io.netty.channel.DefaultChannelPipeline.fireChannelRead(DefaultChannelPipeline.java:897)
    at io.netty.channel.epoll.EpollSocketChannel$EpollSocketUnsafe.epollInReady(EpollSocketChannel.java:718)
    at io.netty.channel.epoll.EpollEventLoop.processReady(EpollEventLoop.java:328)
    at io.netty.channel.epoll.EpollEventLoop.run(EpollEventLoop.java:264)
    at io.netty.util.concurrent.SingleThreadEventExecutor$2.run(SingleThreadEventExecutor.java:122)
    at io.netty.util.internal.chmv8.ForkJoinTask$RunnableExecuteAction.exec(ForkJoinTask.java:1412)
    at io.netty.util.internal.chmv8.ForkJoinTask.doExec(ForkJoinTask.java:280)
    at io.netty.util.internal.chmv8.ForkJoinPool$WorkQueue.runTask(ForkJoinPool.java:877)
    at io.netty.util.internal.chmv8.ForkJoinPool.scan(ForkJoinPool.java:1706)
    at io.netty.util.internal.chmv8.ForkJoinPool.runWorker(ForkJoinPool.java:1661)
    at io.netty.util.internal.chmv8.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:126)
```


Issue comments:
@nmittler - FYI.  This may be more general problem to whenever the windows sizes are used.  We should be handling the case where they can be negative.

@Scottmitch yup, agreed.  I'll take a look.

Opps sorry about stealing the assignment.  I have a fix I think.

I'll submit a PR and we can go from there.

Sgtm :)

PR https://github.com/netty/netty/pull/3086 is to address this issue.

Fix was cherry-picked


Pull request review comments:

Patch that resolved the issue:
diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2OutboundFlowController.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2OutboundFlowController.java
index e3d55cc7ce4..bfaa70327db 100644
--- a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2OutboundFlowController.java
+++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2OutboundFlowController.java
@@ -147,8 +147,9 @@ public void updateOutboundWindowSize(int streamId, int delta) throws Http2Except
             // Update the stream window and write any pending frames for the stream.
             OutboundFlowState state = stateOrFail(streamId);
             state.incrementStreamWindow(delta);
-            state.writeBytes(state.writableWindow());
-            flush();
+            if (state.writeBytes(state.writableWindow()) > 0) {
+                flush();
+            }
         }
     }
 
@@ -508,7 +509,7 @@ private int writeBytes(int bytes) throws Http2Exception {
                     // Window size is large enough to send entire data frame
                     bytesWritten += pendingWrite.size();
                     pendingWrite.write();
-                } else if (maxBytes == 0) {
+                } else if (maxBytes <= 0) {
                     // No data from the current frame can be written - we're done.
                     // We purposely check this after first testing the size of the
                     // pending frame to properly handle zero-length frame.


Test patch:
diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2OutboundFlowControllerTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2OutboundFlowControllerTest.java
index 3fe2e0b2092..b026243bbb3 100644
--- a/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2OutboundFlowControllerTest.java
+++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/DefaultHttp2OutboundFlowControllerTest.java
@@ -298,6 +298,55 @@ public void initialWindowUpdateShouldSendFrame() throws Http2Exception {
         }
     }
 
+    @Test
+    public void negativeWindowShouldNotThrowException() throws Http2Exception {
+        final int initWindow = 20;
+        final int secondWindowSize = 10;
+        controller.initialOutboundWindowSize(initWindow);
+        Http2Stream streamA = connection.stream(STREAM_A);
+
+        final ByteBuf data = dummyData(initWindow, 0);
+        final ByteBuf data2 = dummyData(5, 0);
+        try {
+            // Deplete the stream A window to 0
+            send(STREAM_A, data.slice(0, initWindow), 0);
+            verifyWrite(STREAM_A, data.slice(0, initWindow), 0);
+
+            // Make the window size for stream A negative
+            controller.initialOutboundWindowSize(initWindow - secondWindowSize);
+            assertEquals(-secondWindowSize, streamA.outboundFlow().window());
+
+            // Queue up a write. It should not be written now because the window is negative
+            resetFrameWriter();
+            send(STREAM_A, data2.slice(), 0);
+            verifyNoWrite(STREAM_A);
+
+            // Open the window size back up a bit (no send should happen)
+            controller.updateOutboundWindowSize(STREAM_A, 5);
+            assertEquals(-5, streamA.outboundFlow().window());
+            verifyNoWrite(STREAM_A);
+
+            // Open the window size back up a bit (no send should happen)
+            controller.updateOutboundWindowSize(STREAM_A, 5);
+            assertEquals(0, streamA.outboundFlow().window());
+            verifyNoWrite(STREAM_A);
+
+            // Open the window size back up and allow the write to happen
+            controller.updateOutboundWindowSize(STREAM_A, 5);
+            assertEquals(0, streamA.outboundFlow().window());
+
+            // Verify that the entire frame was sent.
+            ArgumentCaptor<ByteBuf> argument = ArgumentCaptor.forClass(ByteBuf.class);
+            captureWrite(STREAM_A, argument, 0, false);
+            final ByteBuf writtenBuf = argument.getValue();
+            assertEquals(data2, writtenBuf);
+            assertEquals(1, data2.refCnt());
+        } finally {
+            manualSafeRelease(data);
+            manualSafeRelease(data2);
+        }
+    }
+
     @Test
     public void initialWindowUpdateShouldSendEmptyFrame() throws Http2Exception {
         controller.initialOutboundWindowSize(0);