Skip to content

Commit

Permalink
Add unit tests for data frames with padding including support for sim…
Browse files Browse the repository at this point in the history 
…ple POST requests.

Fix errors in parsing of padded data frames.
Make parser responsible for swallowing unwanted data rather than the code using the parser and rename output methods to make this clearer
More debug logging

git-svn-id: https://svn.apache.org/repos/asf/tomcat/trunk@1686156 13f79535-47bb-0310-9956-ffa450edef68
  • Loading branch information
@markt-asf
markt-asf committed Jun 18, 2015
1 parent b9fd6fa commit a8ded9b
Show file tree
Hide file tree
Showing 5 changed files with 113 additions and 21 deletions.
45 changes: 35 additions & 10 deletions java/org/apache/coyote/http2/Http2Parser.java
View file
Expand Up @@ -86,7 +86,7 @@ private boolean readFrame(boolean block, FrameType expected)
try {
validateFrame(expected, frameType, streamId, flags, payloadSize);
} catch (StreamException se) {
swallow(payloadSize);
swallow(streamId, payloadSize);
throw se;
}

Expand Down Expand Up @@ -136,28 +136,47 @@ private void readDataFrame(int streamId, int flags, int payloadSize)

boolean endOfStream = Flags.isEndOfStream(flags);

int dataLength;
if (Flags.hasPadding(flags)) {
byte[] b = new byte[1];
input.fill(true, b);
padLength = b[0] & 0xFF;
// +1 is for the padding length byte we just read above
dataLength = payloadSize - (padLength + 1);
} else {
dataLength = payloadSize;
}

ByteBuffer dest = output.getInputByteBuffer(streamId, payloadSize);
if (log.isDebugEnabled()) {
String padding;
if (Flags.hasPadding(flags)) {
padding = Integer.toString(padLength);
} else {
padding = "none";
}
log.debug(sm.getString("http2Parser.processFrameData.lengths", connectionId,
Integer.toString(streamId), Integer.toString(dataLength), padding));
}

ByteBuffer dest = output.getInputByteBuffer(streamId, dataLength);
if (dest == null) {
swallow(payloadSize);
swallow(streamId, dataLength);
if (endOfStream) {
output.receiveEndOfStream(streamId);
}
} else {
synchronized (dest) {
input.fill(true, dest, payloadSize);
input.fill(true, dest, dataLength);
if (endOfStream) {
output.receiveEndOfStream(streamId);
}
dest.notifyAll();
}
}
swallow(padLength);
if (padLength > 0) {
swallow(streamId, padLength);
output.swallowedPadding(streamId, padLength);
}
}


Expand All @@ -170,7 +189,7 @@ private void readHeadersFrame(int streamId, int flags, int payloadSize)
try {
hpackDecoder.setHeaderEmitter(output.headersStart(streamId));
} catch (StreamException se) {
swallow(payloadSize);
swallow(streamId, payloadSize);
throw se;
}

Expand Down Expand Up @@ -205,7 +224,7 @@ private void readHeadersFrame(int streamId, int flags, int payloadSize)

readHeaderBlock(payloadSize, endOfHeaders);

swallow(padLength);
swallow(streamId, padLength);

if (endOfHeaders) {
output.headersEnd(streamId);
Expand Down Expand Up @@ -386,11 +405,16 @@ private void readHeaderBlock(int payloadSize, boolean endOfHeaders)

private void readUnknownFrame(int streamId, FrameType frameType, int flags, int payloadSize)
throws IOException {
output.swallow(streamId, frameType, flags, payloadSize);
swallow(streamId, payloadSize);
output.swallowed(streamId, frameType, flags, payloadSize);
}


private void swallow(int len) throws IOException {
private void swallow(int streamId, int len) throws IOException {
if (log.isDebugEnabled()) {
log.debug(sm.getString("http2Parser.swallow.debug", connectionId,
Integer.toString(streamId), Integer.toString(len)));
}
if (len == 0) {
return;
}
Expand Down Expand Up @@ -527,6 +551,7 @@ static interface Output {
// Data frames
ByteBuffer getInputByteBuffer(int streamId, int payloadSize) throws Http2Exception;
void receiveEndOfStream(int streamId) throws ConnectionException;
void swallowedPadding(int streamId, int paddingLength) throws ConnectionException, IOException;

// Header frames
HeaderEmitter headersStart(int streamId) throws Http2Exception;
Expand Down Expand Up @@ -554,6 +579,6 @@ void reprioritise(int streamId, int parentStreamId, boolean exclusive, int weigh
void incrementWindowSize(int streamId, int increment) throws Http2Exception;

// Testing
void swallow(int streamId, FrameType frameType, int flags, int size) throws IOException;
void swallowed(int streamId, FrameType frameType, int flags, int size) throws IOException;
}
}
14 changes: 12 additions & 2 deletions java/org/apache/coyote/http2/Http2UpgradeHandler.java
View file
Expand Up @@ -811,6 +811,15 @@ public void receiveEndOfStream(int streamId) throws ConnectionException {
}


@Override
public void swallowedPadding(int streamId, int paddingLength) throws
ConnectionException, IOException {
Stream stream = getStream(streamId, true);
// +1 is for the payload byte used to define the padding length
writeWindowUpdate(stream, paddingLength + 1);
}


@Override
public HeaderEmitter headersStart(int streamId) throws Http2Exception {
Stream stream = getStream(streamId, false);
Expand Down Expand Up @@ -942,7 +951,8 @@ public void incrementWindowSize(int streamId, int increment) throws Http2Excepti


@Override
public void swallow(int streamId, FrameType frameType, int flags, int size) throws IOException {
swallow(size);
public void swallowed(int streamId, FrameType frameType, int flags, int size)
throws IOException {
// NO-OP.
}
}
2 changes: 2 additions & 0 deletions java/org/apache/coyote/http2/LocalStrings.properties
View file
Expand Up @@ -44,6 +44,7 @@ http2Parser.preface.io=Unable to read connection preface
http2Parser.processFrame=Connection [{0}], Stream [{1}], Frame type [{2}], Flags [{3}], Payload size [{4}]
http2Parser.processFrame.unexpectedType=Expected frame type [{0}] but received frame type [{1}]
http2Parser.processFrameContinuation.notExpected=Connection [{0}], Continuation frame received for stream [{1}] when no headers were in progress
http2Parser.processFrameData.lengths=Connection [{0}], Stream [{1}], Data length, [{2}], Padding length [{3}]
http2Parser.processFrameGoaway.payloadTooSmall=Connection [{0}]: Goaway payload size was [{1}] which is less than the minimum 8
http2Parser.processFrameHeaders.decodingFailed=There was an error during the HPACK decoding of HTTP headers
http2Parser.processFrameHeaders.decodingDataLeft=Data left over after HPACK decoding - it should have been consumed
Expand All @@ -56,6 +57,7 @@ http2Parser.processFrameSettings.invalidPayloadSize=Settings frame received with
http2Parser.processFrameWindowUpdate.debug=Connection [{0}], Stream [{1}], Window size increment [{2}]
http2Parser.processFrameWindowUpdate.invalidIncrement=Window update frame received with an invalid increment size of [0]
http2Parser.processFrameWindowUpdate.invalidPayloadSize=Window update frame received with an invalid payload size of [{0}]
http2Parser.swallow.debug=Connection [{0}], Stream [{1}], Swallowed [{2}] bytes

stream.header.debug=Connection [{0}], Stream [{1}], HTTP header [{2}], Value [{3}]
stream.reprioritisation.debug=Connection [{0}], Stream [{1}], Exclusive [{2}], Parent [{3}], Weight [{4}]
Expand Down
42 changes: 35 additions & 7 deletions test/org/apache/coyote/http2/Http2TestBase.java
View file
Expand Up @@ -193,21 +193,21 @@ protected void buildSimpleGetRequestPart2(byte[] frameHeader, ByteBuffer headers
}


protected void sendSimplePostRequest(int streamId) throws IOException {
protected void sendSimplePostRequest(int streamId, byte[] padding) throws IOException {
byte[] headersFrameHeader = new byte[9];
ByteBuffer headersPayload = ByteBuffer.allocate(128);
byte[] dataFrameHeader = new byte[9];
ByteBuffer dataPayload = ByteBuffer.allocate(128);

buildPostRequest(headersFrameHeader, headersPayload,
dataFrameHeader, dataPayload, streamId);
dataFrameHeader, dataPayload, padding, streamId);
writeFrame(headersFrameHeader, headersPayload);
writeFrame(dataFrameHeader, dataPayload);
}


protected void buildPostRequest(byte[] headersFrameHeader, ByteBuffer headersPayload,
byte[] dataFrameHeader, ByteBuffer dataPayload, int streamId) {
byte[] dataFrameHeader, ByteBuffer dataPayload, byte[] padding, int streamId) {
MimeHeaders headers = new MimeHeaders();
headers.addValue(":method").setString("POST");
headers.addValue(":path").setString("/simple");
Expand All @@ -225,16 +225,30 @@ protected void buildPostRequest(byte[] headersFrameHeader, ByteBuffer headersPay
ByteUtil.set31Bits(headersFrameHeader, 5, streamId);

// Data
if (padding != null) {
dataPayload.put((byte) (padding.length & 0xFF));
dataPayload.limit(dataPayload.capacity() - padding.length);
}

while (dataPayload.hasRemaining()) {
dataPayload.put((byte) 'x');
}
if (padding != null && padding.length > 0) {
dataPayload.limit(dataPayload.capacity());
dataPayload.put(padding);
}

dataPayload.flip();

// Size
ByteUtil.setThreeBytes(dataFrameHeader, 0, dataPayload.limit());
// Data is type 0
// End of stream
dataFrameHeader[4] = 0x01;
// Flags: End of stream 1, Padding 8
if (padding == null) {
dataFrameHeader[4] = 0x01;
} else {
dataFrameHeader[4] = 0x09;
}
ByteUtil.set31Bits(dataFrameHeader, 5, streamId);
}

Expand All @@ -254,7 +268,12 @@ protected void readSimpleGetResponse() throws Http2Exception, IOException {
}


protected void readSimplePostResponse() throws Http2Exception, IOException {
protected void readSimplePostResponse(boolean padding) throws Http2Exception, IOException {
if (padding) {
// Window updates for padding
parser.readFrame(true);
parser.readFrame(true);
}
// Connection window update after reading request body
parser.readFrame(true);
// Stream window update after reading request body
Expand Down Expand Up @@ -675,7 +694,7 @@ public void incrementWindowSize(int streamId, int increment) {


@Override
public void swallow(int streamId, FrameType frameType, int flags, int size) {
public void swallowed(int streamId, FrameType frameType, int flags, int size) {
trace.append(streamId);
trace.append(",");
trace.append(frameType);
Expand All @@ -687,6 +706,15 @@ public void swallow(int streamId, FrameType frameType, int flags, int size) {
}


@Override
public void swallowedPadding(int streamId, int paddingLength) {
trace.append(streamId);
trace.append("-SwallowedPadding-[");
trace.append(paddingLength);
trace.append("]\n");
}


public void clearTrace() {
trace = new StringBuffer();
}
Expand Down
31 changes: 29 additions & 2 deletions test/org/apache/coyote/http2/TestHttp2Section_6_1.java
View file
Expand Up @@ -32,8 +32,8 @@ public class TestHttp2Section_6_1 extends Http2TestBase {
public void testDataFrame() throws Exception {
http2Connect();

sendSimplePostRequest(3);
readSimplePostResponse();
sendSimplePostRequest(3, null);
readSimplePostResponse(false);

Assert.assertEquals("0-WindowSize-[128]\n"
+ "3-WindowSize-[128]\n"
Expand All @@ -45,5 +45,32 @@ public void testDataFrame() throws Exception {
}


@Test
public void testDataFrameWithPadding() throws Exception {
http2Connect();

byte[] padding = new byte[8];

sendSimplePostRequest(3, padding);
readSimplePostResponse(true);


// The window update for the padding could occur anywhere since it
// happens on a different thead to the response.
String trace = output.getTrace();
String paddingWindowUpdate = "0-WindowSize-[9]\n3-WindowSize-[9]\n";

Assert.assertTrue(trace, trace.contains(paddingWindowUpdate));
trace = trace.replace(paddingWindowUpdate, "");

Assert.assertEquals("0-WindowSize-[119]\n"
+ "3-WindowSize-[119]\n"
+ "3-HeadersStart\n"
+ "3-Header-[:status]-[200]\n"
+ "3-HeadersEnd\n"
+ "3-Body-119\n"
+ "3-EndOfStream\n", trace);
}

// TODO: Remainder if section 6.1 tests
}

0 comments on commit a8ded9b

Please sign in to comment.