HTTP/2 OutboundFlowControl negative window excpetion

Motivation:
The DefaultOutboundFlowController was attempting to write frames with a negative length.  This resulted in attempting to allocate a buffer of negative size and thus an exception.

Modifications:
- Don't allow DefaultOutboundFlowController to write negative length buffers.

Result:
No more negative length writes which resulted in IllegalArgumentExceptions.
This commit is contained in:
Scott Mitchell 2014-10-31 14:13:43 -04:00
parent 8235337d4e
commit d5042baf58
2 changed files with 53 additions and 3 deletions

View File

@ -147,8 +147,9 @@ public class DefaultHttp2OutboundFlowController implements Http2OutboundFlowCont
// Update the stream window and write any pending frames for the stream. // Update the stream window and write any pending frames for the stream.
OutboundFlowState state = stateOrFail(streamId); OutboundFlowState state = stateOrFail(streamId);
state.incrementStreamWindow(delta); state.incrementStreamWindow(delta);
state.writeBytes(state.writableWindow()); if (state.writeBytes(state.writableWindow()) > 0) {
flush(); flush();
}
} }
} }
@ -508,7 +509,7 @@ public class DefaultHttp2OutboundFlowController implements Http2OutboundFlowCont
// Window size is large enough to send entire data frame // Window size is large enough to send entire data frame
bytesWritten += pendingWrite.size(); bytesWritten += pendingWrite.size();
pendingWrite.write(); pendingWrite.write();
} else if (maxBytes == 0) { } else if (maxBytes <= 0) {
// No data from the current frame can be written - we're done. // No data from the current frame can be written - we're done.
// We purposely check this after first testing the size of the // We purposely check this after first testing the size of the
// pending frame to properly handle zero-length frame. // pending frame to properly handle zero-length frame.

View File

@ -298,6 +298,55 @@ public class DefaultHttp2OutboundFlowControllerTest {
} }
} }
@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 @Test
public void initialWindowUpdateShouldSendEmptyFrame() throws Http2Exception { public void initialWindowUpdateShouldSendEmptyFrame() throws Http2Exception {
controller.initialOutboundWindowSize(0); controller.initialOutboundWindowSize(0);