andreacavalli/netty5
1
0
Fork 0
Code Issues Pull Requests Projects Releases Wiki Activity

HTTP/2: Fix some errors reported by h2spec.

Browse Source 
Motivation:

h2spec [1] reported 32 issues [2] with Netty's HTTP/2 implementation.

Modifications:

Fixed 11 of those issues. Mostly wrong error codes or added missing validation
code in the frame reader.

Result:

11 fewer errors. h2spec now reports: 70 tests, 48 passed, 1 skipped, 21 failed

[1] https://github.com/summerwind/h2spec
[2] https://github.com/netty/netty/issues/5761
This commit is contained in:
buchgr 2016-08-30 12:35:52 +02:00 committed by Norman Maurer
parent 8cf90f0512
commit 515f8964b4
4 changed files with 85 additions and 30 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2ConnectionDecoder.java
View File

@ -453,6 +453,11 @@ public class DefaultHttp2ConnectionDecoder implements Http2ConnectionDecoder {
@Override
public void onPushPromiseRead(ChannelHandlerContext ctx, int streamId, int promisedStreamId,
Http2Headers headers, int padding) throws Http2Exception {
// A client cannot push.
if (connection().isServer()) {
throw connectionError(PROTOCOL_ERROR, "A client cannot push.");
}
Http2Stream parentStream = connection.stream(streamId);
if (shouldIgnoreHeadersOrDataFrame(ctx, streamId, parentStream, "PUSH_PROMISE")) {

44
codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2FrameReader.java
View File

@ -179,7 +179,8 @@ public class DefaultHttp2FrameReader implements Http2FrameReader, Http2FrameSize
// Read the header and prepare the unmarshaller to read the frame.
payloadLength = in.readUnsignedMedium();
if (payloadLength > maxFrameSize) {
throw connectionError(PROTOCOL_ERROR, "Frame length: %d exceeds maximum: %d", payloadLength, maxFrameSize);
throw connectionError(FRAME_SIZE_ERROR, "Frame length: %d exceeds maximum: %d", payloadLength,
maxFrameSize);
}
frameType = in.readByte();
flags = new Http2Flags(in.readUnsignedByte());
@ -277,6 +278,7 @@ public class DefaultHttp2FrameReader implements Http2FrameReader, Http2FrameSize
}
private void verifyDataFrame() throws Http2Exception {
verifyAssociatedWithAStream();
verifyNotProcessingHeaders();
verifyPayloadLength(payloadLength);
@ -287,6 +289,7 @@ public class DefaultHttp2FrameReader implements Http2FrameReader, Http2FrameSize
}
private void verifyHeadersFrame() throws Http2Exception {
verifyAssociatedWithAStream();
verifyNotProcessingHeaders();
verifyPayloadLength(payloadLength);
@ -298,6 +301,7 @@ public class DefaultHttp2FrameReader implements Http2FrameReader, Http2FrameSize
}
private void verifyPriorityFrame() throws Http2Exception {
verifyAssociatedWithAStream();
verifyNotProcessingHeaders();
if (payloadLength != PRIORITY_ENTRY_LENGTH) {
@ -307,11 +311,11 @@ public class DefaultHttp2FrameReader implements Http2FrameReader, Http2FrameSize
}
private void verifyRstStreamFrame() throws Http2Exception {
verifyAssociatedWithAStream();
verifyNotProcessingHeaders();
if (payloadLength != INT_FIELD_LENGTH) {
throw streamError(streamId, FRAME_SIZE_ERROR,
"Invalid frame length %d.", payloadLength);
throw connectionError(FRAME_SIZE_ERROR, "Invalid frame length %d.", payloadLength);
}
}
@ -322,7 +326,7 @@ public class DefaultHttp2FrameReader implements Http2FrameReader, Http2FrameSize
throw connectionError(PROTOCOL_ERROR, "A stream ID must be zero.");
}
if (flags.ack() && payloadLength > 0) {
throw connectionError(PROTOCOL_ERROR, "Ack settings frame must have an empty payload.");
throw connectionError(FRAME_SIZE_ERROR, "Ack settings frame must have an empty payload.");
}
if (payloadLength % SETTING_ENTRY_LENGTH > 0) {
throw connectionError(FRAME_SIZE_ERROR, "Frame length %d invalid.", payloadLength);
@ -370,12 +374,12 @@ public class DefaultHttp2FrameReader implements Http2FrameReader, Http2FrameSize
verifyStreamOrConnectionId(streamId, "Stream ID");
if (payloadLength != INT_FIELD_LENGTH) {
throw streamError(streamId, FRAME_SIZE_ERROR,
"Invalid frame length %d.", payloadLength);
throw connectionError(FRAME_SIZE_ERROR, "Invalid frame length %d.", payloadLength);
}
}
private void verifyContinuationFrame() throws Http2Exception {
verifyAssociatedWithAStream();
verifyPayloadLength(payloadLength);
if (headersContinuation == null) {
@ -397,14 +401,11 @@ public class DefaultHttp2FrameReader implements Http2FrameReader, Http2FrameSize
private void readDataFrame(ChannelHandlerContext ctx, ByteBuf payload,
Http2FrameListener listener) throws Http2Exception {
int padding = readPadding(payload);
verifyPadding(padding);
// Determine how much data there is to read by removing the trailing
// padding.
int dataLength = lengthWithoutTrailingPadding(payload.readableBytes(), padding);
if (dataLength < 0) {
throw streamError(streamId, FRAME_SIZE_ERROR,
"Frame payload too small for padding.");
}
ByteBuf data = payload.readSlice(dataLength);
listener.onDataRead(ctx, streamId, data, padding, flags.endOfStream());
@ -416,6 +417,7 @@ public class DefaultHttp2FrameReader implements Http2FrameReader, Http2FrameSize
final int headersStreamId = streamId;
final Http2Flags headersFlags = flags;
final int padding = readPadding(payload);
verifyPadding(padding);
// The callback that is invoked is different depending on whether priority information
// is present in the headers frame.
@ -423,6 +425,9 @@ public class DefaultHttp2FrameReader implements Http2FrameReader, Http2FrameSize
long word1 = payload.readUnsignedInt();
final boolean exclusive = (word1 & 0x80000000L) != 0;
final int streamDependency = (int) (word1 & 0x7FFFFFFFL);
if (streamDependency == streamId) {
throw streamError(streamId, PROTOCOL_ERROR, "A stream cannot depend on itself.");
}
final short weight = (short) (payload.readUnsignedByte() + 1);
final ByteBuf fragment = payload.readSlice(lengthWithoutTrailingPadding(payload.readableBytes(), padding));
@ -480,6 +485,9 @@ public class DefaultHttp2FrameReader implements Http2FrameReader, Http2FrameSize
long word1 = payload.readUnsignedInt();
boolean exclusive = (word1 & 0x80000000L) != 0;
int streamDependency = (int) (word1 & 0x7FFFFFFFL);
if (streamDependency == streamId) {
throw streamError(streamId, PROTOCOL_ERROR, "A stream cannot depend on itself.");
}
short weight = (short) (payload.readUnsignedByte() + 1);
listener.onPriorityRead(ctx, streamId, streamDependency, weight, exclusive);
}
@ -505,7 +513,7 @@ public class DefaultHttp2FrameReader implements Http2FrameReader, Http2FrameSize
} catch (IllegalArgumentException e) {
switch(id) {
case SETTINGS_MAX_FRAME_SIZE:
throw connectionError(FRAME_SIZE_ERROR, e, e.getMessage());
throw connectionError(PROTOCOL_ERROR, e, e.getMessage());
case SETTINGS_INITIAL_WINDOW_SIZE:
throw connectionError(FLOW_CONTROL_ERROR, e, e.getMessage());
default:
@ -521,6 +529,7 @@ public class DefaultHttp2FrameReader implements Http2FrameReader, Http2FrameSize
Http2FrameListener listener) throws Http2Exception {
final int pushPromiseStreamId = streamId;
final int padding = readPadding(payload);
verifyPadding(padding);
final int promisedStreamId = readUnsignedInt(payload);
// Create a handler that invokes the listener when the header block is complete.
@ -599,6 +608,13 @@ public class DefaultHttp2FrameReader implements Http2FrameReader, Http2FrameSize
return payload.readUnsignedByte() + 1;
}
private void verifyPadding(int padding) throws Http2Exception {
int len = lengthWithoutTrailingPadding(payloadLength, padding);
if (len < 0) {
throw connectionError(PROTOCOL_ERROR, "Frame payload too small for padding.");
}
}
/**
* The padding parameter consists of the 1 byte pad length field and the trailing padding bytes. This method
* returns the number of readable bytes without the trailing padding.
@ -740,6 +756,12 @@ public class DefaultHttp2FrameReader implements Http2FrameReader, Http2FrameSize
}
}
private void verifyAssociatedWithAStream() throws Http2Exception {
if (streamId == 0) {
throw connectionError(PROTOCOL_ERROR, "Frame of type %s must be associated with a stream.", frameType);
}
}
private static void verifyStreamOrConnectionId(int streamId, String argumentName)
throws Http2Exception {
if (streamId < 0) {

57
codec-http2/src/main/java/io/netty/handler/codec/http2/Http2ConnectionHandler.java
View File

@ -99,7 +99,7 @@ public class Http2ConnectionHandler extends ByteToMessageDecoder implements Http
public void gracefulShutdownTimeoutMillis(long gracefulShutdownTimeoutMillis) {
if (gracefulShutdownTimeoutMillis < 0) {
throw new IllegalArgumentException("gracefulShutdownTimeoutMillis: " + gracefulShutdownTimeoutMillis +
" (expected: >= 0)");
" (expected: >= 0)");
}
this.gracefulShutdownTimeoutMillis = gracefulShutdownTimeoutMillis;
}
@ -264,11 +264,12 @@ public class Http2ConnectionHandler extends ByteToMessageDecoder implements Http
// If the input so far doesn't match the preface, break the connection.
if (bytesRead == 0 || !ByteBufUtil.equals(in, in.readerIndex(),
clientPrefaceString, clientPrefaceString.readerIndex(), bytesRead)) {
clientPrefaceString, clientPrefaceString.readerIndex(),
bytesRead)) {
String receivedBytes = hexDump(in, in.readerIndex(),
min(in.readableBytes(), clientPrefaceString.readableBytes()));
min(in.readableBytes(), clientPrefaceString.readableBytes()));
throw connectionError(PROTOCOL_ERROR, "HTTP/2 client preface string missing or corrupt. " +
"Hex dump for received bytes: %s", receivedBytes);
"Hex dump for received bytes: %s", receivedBytes);
}
in.skipBytes(bytesRead);
clientPrefaceString.skipBytes(bytesRead);
@ -300,7 +301,8 @@ public class Http2ConnectionHandler extends ByteToMessageDecoder implements Http
short flags = in.getUnsignedByte(in.readerIndex() + 4);
if (frameType != SETTINGS || (flags & Http2Flags.ACK) != 0) {
throw connectionError(PROTOCOL_ERROR, "First received frame was not SETTINGS. " +
"Hex dump for first 5 bytes: %s", hexDump(in, in.readerIndex(), 5));
"Hex dump for first 5 bytes: %s",
hexDump(in, in.readerIndex(), 5));
}
return true;
}
@ -434,7 +436,7 @@ public class Http2ConnectionHandler extends ByteToMessageDecoder implements Http
} else {
// If there are active streams we should wait until they are all closed before closing the connection.
closeListener = new ClosingChannelFutureListener(ctx, promise,
gracefulShutdownTimeoutMillis, MILLISECONDS);
gracefulShutdownTimeoutMillis, MILLISECONDS);
}
}
@ -604,15 +606,39 @@ public class Http2ConnectionHandler extends ByteToMessageDecoder implements Http
return encoder().frameWriter();
}
/**
* Sends a {@code RST_STREAM} frame even if we don't know about the stream. This error condition is most likely
* triggered by the first frame of a stream being invalid. That is, there was an error reading the frame before
* we could create a new stream.
*/
private ChannelFuture resetUnknownStream(final ChannelHandlerContext ctx, int streamId, long errorCode,
ChannelPromise promise) {
ChannelFuture future = frameWriter().writeRstStream(ctx, streamId, errorCode, promise);
if (future.isDone()) {
closeConnectionOnError(ctx, future);
} else {
future.addListener(new ChannelFutureListener() {
@Override
public void operationComplete(ChannelFuture future) throws Exception {
closeConnectionOnError(ctx, future);
}
});
}
return future;
}
@Override
public ChannelFuture resetStream(final ChannelHandlerContext ctx, int streamId, long errorCode,
final ChannelPromise promise) {
final ChannelPromise promise) {
final Http2Stream stream = connection().stream(streamId);
if (stream == null || stream.isResetSent()) {
// Don't write a RST_STREAM frame if we are not aware of the stream, or if we have already written one.
return promise.setSuccess();
if (stream == null) {
return resetUnknownStream(ctx, streamId, errorCode, promise);
}
if (stream.isResetSent()) {
// Don't write a RST_STREAM frame if we have already written one.
return promise.setSuccess();
}
final ChannelFuture future;
if (stream.state() == IDLE) {
// We cannot write RST_STREAM frames on IDLE streams https://tools.ietf.org/html/rfc7540#section-6.4.
@ -647,8 +673,7 @@ public class Http2ConnectionHandler extends ByteToMessageDecoder implements Http
if (connection.goAwaySent() && lastStreamId > connection.remote().lastStreamKnownByPeer()) {
throw connectionError(PROTOCOL_ERROR, "Last stream identifier must not increase between " +
"sending multiple GOAWAY frames (was '%d', is '%d').",
connection.remote().lastStreamKnownByPeer(),
lastStreamId);
connection.remote().lastStreamKnownByPeer(), lastStreamId);
}
connection.goAwaySent(lastStreamId, errorCode, debugData);
@ -714,6 +739,12 @@ public class Http2ConnectionHandler extends ByteToMessageDecoder implements Http
}
}
private void closeConnectionOnError(ChannelHandlerContext ctx, ChannelFuture future) {
if (!future.isSuccess()) {
onConnectionError(ctx, future.cause(), null);
}
}
/**
* Returns the client preface string if this is a client connection, otherwise returns {@code null}.
*/
@ -722,7 +753,7 @@ public class Http2ConnectionHandler extends ByteToMessageDecoder implements Http
}
private static void processGoAwayWriteResult(final ChannelHandlerContext ctx, final int lastStreamId,
final long errorCode, final ByteBuf debugData, ChannelFuture future) {
final long errorCode, final ByteBuf debugData, ChannelFuture future) {
try {
if (future.isSuccess()) {
if (errorCode != NO_ERROR.code()) {

9
codec-http2/src/test/java/io/netty/handler/codec/http2/Http2ConnectionHandlerTest.java
View File

@ -313,13 +313,10 @@ public class Http2ConnectionHandlerTest {
@Test
public void writeRstOnNonExistantStreamShouldSucceed() throws Exception {
handler = newHandler();
when(frameWriter.writeRstStream(eq(ctx), eq(NON_EXISTANT_STREAM_ID),
eq(STREAM_CLOSED.code()), eq(promise))).thenReturn(future);
handler.resetStream(ctx, NON_EXISTANT_STREAM_ID, STREAM_CLOSED.code(), promise);
verify(frameWriter, never())
.writeRstStream(any(ChannelHandlerContext.class), anyInt(), anyLong(),
any(ChannelPromise.class));
assertTrue(promise.isDone());
assertTrue(promise.isSuccess());
assertNull(promise.cause());
verify(frameWriter).writeRstStream(eq(ctx), eq(NON_EXISTANT_STREAM_ID), eq(STREAM_CLOSED.code()), eq(promise));
}
@Test