diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2HeadersDecoder.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2HeadersDecoder.java index 684fef0352..5d6320950c 100644 --- a/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2HeadersDecoder.java +++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2HeadersDecoder.java @@ -22,6 +22,7 @@ import io.netty.util.internal.UnstableApi; import static io.netty.handler.codec.http2.Http2CodecUtil.DEFAULT_HEADER_LIST_SIZE; import static io.netty.handler.codec.http2.Http2CodecUtil.DEFAULT_INITIAL_HUFFMAN_DECODE_CAPACITY; import static io.netty.handler.codec.http2.Http2Error.COMPRESSION_ERROR; +import static io.netty.handler.codec.http2.Http2Error.INTERNAL_ERROR; import static io.netty.handler.codec.http2.Http2Exception.connectionError; @UnstableApi @@ -31,6 +32,7 @@ public class DefaultHttp2HeadersDecoder implements Http2HeadersDecoder, Http2Hea private final HpackDecoder hpackDecoder; private final boolean validateHeaders; + private long maxHeaderListSizeGoAway; /** * Used to calculate an exponential moving average of header sizes to get an estimate of how large the data @@ -79,6 +81,8 @@ public class DefaultHttp2HeadersDecoder implements Http2HeadersDecoder, Http2Hea DefaultHttp2HeadersDecoder(boolean validateHeaders, HpackDecoder hpackDecoder) { this.hpackDecoder = ObjectUtil.checkNotNull(hpackDecoder, "hpackDecoder"); this.validateHeaders = validateHeaders; + this.maxHeaderListSizeGoAway = + Http2CodecUtil.calculateMaxHeaderListSizeGoAway(hpackDecoder.getMaxHeaderListSize()); } @Override @@ -93,7 +97,12 @@ public class DefaultHttp2HeadersDecoder implements Http2HeadersDecoder, Http2Hea @Override public void maxHeaderListSize(long max, long goAwayMax) throws Http2Exception { - hpackDecoder.setMaxHeaderListSize(max, goAwayMax); + if (goAwayMax < max || goAwayMax < 0) { + throw connectionError(INTERNAL_ERROR, "Header List Size GO_AWAY %d must be non-negative and >= %d", + goAwayMax, max); + } + hpackDecoder.setMaxHeaderListSize(max); + this.maxHeaderListSizeGoAway = goAwayMax; } @Override @@ -103,7 +112,7 @@ public class DefaultHttp2HeadersDecoder implements Http2HeadersDecoder, Http2Hea @Override public long maxHeaderListSizeGoAway() { - return hpackDecoder.getMaxHeaderListSizeGoAway(); + return maxHeaderListSizeGoAway; } @Override diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackDecoder.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackDecoder.java index 67d6aa9944..078c079891 100644 --- a/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackDecoder.java +++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/HpackDecoder.java @@ -42,7 +42,6 @@ import static io.netty.handler.codec.http2.Http2CodecUtil.MIN_HEADER_LIST_SIZE; import static io.netty.handler.codec.http2.Http2CodecUtil.MIN_HEADER_TABLE_SIZE; import static io.netty.handler.codec.http2.Http2CodecUtil.headerListSizeExceeded; import static io.netty.handler.codec.http2.Http2Error.COMPRESSION_ERROR; -import static io.netty.handler.codec.http2.Http2Error.INTERNAL_ERROR; import static io.netty.handler.codec.http2.Http2Error.PROTOCOL_ERROR; import static io.netty.handler.codec.http2.Http2Exception.connectionError; import static io.netty.handler.codec.http2.Http2Headers.PseudoHeaderName.getPseudoHeader; @@ -84,7 +83,6 @@ final class HpackDecoder { private final HpackDynamicTable hpackDynamicTable; private final HpackHuffmanDecoder hpackHuffmanDecoder; - private long maxHeaderListSizeGoAway; private long maxHeaderListSize; private long maxDynamicTableSize; private long encoderMaxDynamicTableSize; @@ -108,7 +106,6 @@ final class HpackDecoder { */ HpackDecoder(long maxHeaderListSize, int initialHuffmanDecodeCapacity, int maxHeaderTableSize) { this.maxHeaderListSize = checkPositive(maxHeaderListSize, "maxHeaderListSize"); - this.maxHeaderListSizeGoAway = Http2CodecUtil.calculateMaxHeaderListSizeGoAway(maxHeaderListSize); maxDynamicTableSize = encoderMaxDynamicTableSize = maxHeaderTableSize; maxDynamicTableSizeChangeRequired = false; @@ -122,8 +119,18 @@ final class HpackDecoder { * This method assumes the entire header block is contained in {@code in}. */ public void decode(int streamId, ByteBuf in, Http2Headers headers, boolean validateHeaders) throws Http2Exception { + Http2HeadersSink sink = new Http2HeadersSink(headers, maxHeaderListSize); + decode(in, sink, validateHeaders); + + // we have read all of our headers. See if we have exceeded our maxHeaderListSize. We must + // delay throwing until this point to prevent dynamic table corruption + if (sink.exceededMaxLength()) { + headerListSizeExceeded(streamId, maxHeaderListSize, true); + } + } + + private void decode(ByteBuf in, Sink sink, boolean validateHeaders) throws Http2Exception { int index = 0; - long headersLength = 0; int nameLength = 0; int valueLength = 0; byte state = READ_HEADER_REPRESENTATION; @@ -151,8 +158,7 @@ final class HpackDecoder { default: HpackHeaderField indexedHeader = getIndexedHeader(index); headerType = validate(indexedHeader.name, headerType, validateHeaders); - headersLength = addHeader(headers, indexedHeader.name, indexedHeader.value, - headersLength); + sink.appendToHeaderList(indexedHeader.name, indexedHeader.value); } } else if ((b & 0x40) == 0x40) { // Literal Header Field with Incremental Indexing @@ -193,11 +199,11 @@ final class HpackDecoder { state = READ_INDEXED_HEADER_NAME; break; default: - // Index was stored as the prefix - name = readName(index); + // Index was stored as the prefix + name = readName(index); headerType = validate(name, headerType, validateHeaders); - nameLength = name.length(); - state = READ_LITERAL_HEADER_VALUE_LENGTH_PREFIX; + nameLength = name.length(); + state = READ_LITERAL_HEADER_VALUE_LENGTH_PREFIX; } } break; @@ -210,7 +216,7 @@ final class HpackDecoder { case READ_INDEXED_HEADER: HpackHeaderField indexedHeader = getIndexedHeader(decodeULE128(in, index)); headerType = validate(indexedHeader.name, headerType, validateHeaders); - headersLength = addHeader(headers, indexedHeader.name, indexedHeader.value, headersLength); + sink.appendToHeaderList(indexedHeader.name, indexedHeader.value); state = READ_HEADER_REPRESENTATION; break; @@ -229,9 +235,6 @@ final class HpackDecoder { if (index == 0x7f) { state = READ_LITERAL_HEADER_NAME_LENGTH; } else { - if (index > maxHeaderListSizeGoAway - headersLength) { - headerListSizeExceeded(maxHeaderListSizeGoAway); - } nameLength = index; state = READ_LITERAL_HEADER_NAME; } @@ -241,9 +244,6 @@ final class HpackDecoder { // Header Name is a Literal String nameLength = decodeULE128(in, index); - if (nameLength > maxHeaderListSizeGoAway - headersLength) { - headerListSizeExceeded(maxHeaderListSizeGoAway); - } state = READ_LITERAL_HEADER_NAME; break; @@ -269,14 +269,10 @@ final class HpackDecoder { break; case 0: headerType = validate(name, headerType, validateHeaders); - headersLength = insertHeader(headers, name, EMPTY_STRING, indexType, headersLength); + insertHeader(sink, name, EMPTY_STRING, indexType); state = READ_HEADER_REPRESENTATION; break; default: - // Check new header size against max header size - if ((long) index + nameLength > maxHeaderListSizeGoAway - headersLength) { - headerListSizeExceeded(maxHeaderListSizeGoAway); - } valueLength = index; state = READ_LITERAL_HEADER_VALUE; } @@ -287,10 +283,6 @@ final class HpackDecoder { // Header Value is a Literal String valueLength = decodeULE128(in, index); - // Check new header size against max header size - if ((long) valueLength + nameLength > maxHeaderListSizeGoAway - headersLength) { - headerListSizeExceeded(maxHeaderListSizeGoAway); - } state = READ_LITERAL_HEADER_VALUE; break; @@ -302,7 +294,7 @@ final class HpackDecoder { CharSequence value = readStringLiteral(in, valueLength, huffmanEncoded); headerType = validate(name, headerType, validateHeaders); - headersLength = insertHeader(headers, name, value, indexType, headersLength); + insertHeader(sink, name, value, indexType); state = READ_HEADER_REPRESENTATION; break; @@ -311,13 +303,6 @@ final class HpackDecoder { } } - // we have read all of our headers, and not exceeded maxHeaderListSizeGoAway see if we have - // exceeded our actual maxHeaderListSize. This must be done here to prevent dynamic table - // corruption - if (headersLength > maxHeaderListSize) { - headerListSizeExceeded(streamId, maxHeaderListSize, true); - } - if (state != READ_HEADER_REPRESENTATION) { throw connectionError(COMPRESSION_ERROR, "Incomplete header block fragment."); } @@ -341,27 +326,27 @@ final class HpackDecoder { } } + /** + * @deprecated use {@link #setmaxHeaderListSize(long)}; {@code maxHeaderListSizeGoAway} is + * ignored + */ + @Deprecated public void setMaxHeaderListSize(long maxHeaderListSize, long maxHeaderListSizeGoAway) throws Http2Exception { - if (maxHeaderListSizeGoAway < maxHeaderListSize || maxHeaderListSizeGoAway < 0) { - throw connectionError(INTERNAL_ERROR, "Header List Size GO_AWAY %d must be positive and >= %d", - maxHeaderListSizeGoAway, maxHeaderListSize); - } + setMaxHeaderListSize(maxHeaderListSize); + } + + public void setMaxHeaderListSize(long maxHeaderListSize) throws Http2Exception { if (maxHeaderListSize < MIN_HEADER_LIST_SIZE || maxHeaderListSize > MAX_HEADER_LIST_SIZE) { throw connectionError(PROTOCOL_ERROR, "Header List Size must be >= %d and <= %d but was %d", MIN_HEADER_TABLE_SIZE, MAX_HEADER_TABLE_SIZE, maxHeaderListSize); } this.maxHeaderListSize = maxHeaderListSize; - this.maxHeaderListSizeGoAway = maxHeaderListSizeGoAway; } public long getMaxHeaderListSize() { return maxHeaderListSize; } - public long getMaxHeaderListSizeGoAway() { - return maxHeaderListSizeGoAway; - } - /** * Return the maximum table size. This is the maximum size allowed by both the encoder and the * decoder. @@ -450,9 +435,9 @@ final class HpackDecoder { throw INDEX_HEADER_ILLEGAL_INDEX_VALUE; } - private long insertHeader(Http2Headers headers, CharSequence name, CharSequence value, - IndexType indexType, long headerSize) throws Http2Exception { - headerSize = addHeader(headers, name, value, headerSize); + private void insertHeader(Sink sink, CharSequence name, CharSequence value, + IndexType indexType) throws Http2Exception { + sink.appendToHeaderList(name, value); switch (indexType) { case NONE: @@ -466,18 +451,6 @@ final class HpackDecoder { default: throw new Error("should not reach here"); } - - return headerSize; - } - - private long addHeader(Http2Headers headers, CharSequence name, CharSequence value, long headersLength) - throws Http2Exception { - headersLength += HpackHeaderField.sizeOf(name, value); - if (headersLength > maxHeaderListSizeGoAway) { - headerListSizeExceeded(maxHeaderListSizeGoAway); - } - headers.add(name, value); - return headersLength; } private CharSequence readStringLiteral(ByteBuf in, int length, boolean huffmanEncoded) throws Http2Exception { @@ -553,4 +526,35 @@ final class HpackDecoder { REQUEST_PSEUDO_HEADER, RESPONSE_PSEUDO_HEADER } + + private interface Sink { + void appendToHeaderList(CharSequence name, CharSequence value); + } + + private static final class Http2HeadersSink implements Sink { + private final Http2Headers headers; + private final long maxHeaderListSize; + private long headersLength; + private boolean exceededMaxLength; + + public Http2HeadersSink(Http2Headers headers, long maxHeaderListSize) { + this.headers = headers; + this.maxHeaderListSize = maxHeaderListSize; + } + + @Override + public void appendToHeaderList(CharSequence name, CharSequence value) { + headersLength += HpackHeaderField.sizeOf(name, value); + if (headersLength > maxHeaderListSize) { + exceededMaxLength = true; + } + if (!exceededMaxLength) { + headers.add(name, value); + } + } + + public boolean exceededMaxLength() { + return exceededMaxLength; + } + } } diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/HpackDecoderTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/HpackDecoderTest.java index 4652a1fceb..ba0117553e 100644 --- a/codec-http2/src/test/java/io/netty/handler/codec/http2/HpackDecoderTest.java +++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/HpackDecoderTest.java @@ -431,15 +431,13 @@ public class HpackDecoderTest { } @Test - public void testDecodeLargerThanMaxHeaderListSizeButSmallerThanMaxHeaderListSizeUpdatesDynamicTable() - throws Http2Exception { + public void testDecodeLargerThanMaxHeaderListSizeUpdatesDynamicTable() throws Http2Exception { ByteBuf in = Unpooled.buffer(300); try { - hpackDecoder.setMaxHeaderListSize(200, 300); + hpackDecoder.setMaxHeaderListSize(200); HpackEncoder hpackEncoder = new HpackEncoder(true); // encode headers that are slightly larger than maxHeaderListSize - // but smaller than maxHeaderListSizeGoAway Http2Headers toEncode = new DefaultHttp2Headers(); toEncode.add("test_1", "1"); toEncode.add("test_2", "2"); @@ -447,8 +445,7 @@ public class HpackDecoderTest { toEncode.add("test_3", "3"); hpackEncoder.encodeHeaders(1, in, toEncode, NEVER_SENSITIVE); - // decode the headers, we should get an exception, but - // the decoded headers object should contain all of the headers + // decode the headers, we should get an exception Http2Headers decoded = new DefaultHttp2Headers(); try { hpackDecoder.decode(1, in, decoded, true); @@ -457,8 +454,18 @@ public class HpackDecoderTest { assertTrue(e instanceof Http2Exception.HeaderListSizeException); } - assertEquals(4, decoded.size()); - assertTrue(decoded.contains("test_3")); + // but the dynamic table should have been updated, so that later blocks + // can refer to earlier headers + in.clear(); + // 0x80, "indexed header field representation" + // index 62, the first (most recent) dynamic table entry + in.writeByte(0x80 | 62); + Http2Headers decoded2 = new DefaultHttp2Headers(); + hpackDecoder.decode(1, in, decoded2, true); + + Http2Headers golden = new DefaultHttp2Headers(); + golden.add("test_3", "3"); + assertEquals(golden, decoded2); } finally { in.release(); } @@ -468,11 +475,10 @@ public class HpackDecoderTest { public void testDecodeCountsNamesOnlyOnce() throws Http2Exception { ByteBuf in = Unpooled.buffer(200); try { - hpackDecoder.setMaxHeaderListSize(3500, 4000); + hpackDecoder.setMaxHeaderListSize(3500); HpackEncoder hpackEncoder = new HpackEncoder(true); // encode headers that are slightly larger than maxHeaderListSize - // but smaller than maxHeaderListSizeGoAway Http2Headers toEncode = new DefaultHttp2Headers(); toEncode.add(String.format("%03000d", 0).replace('0', 'f'), "value"); toEncode.add("accept", "value"); @@ -493,7 +499,7 @@ public class HpackDecoderTest { String headerName = "12345"; String headerValue = "56789"; long headerSize = headerName.length() + headerValue.length(); - hpackDecoder.setMaxHeaderListSize(headerSize, 100); + hpackDecoder.setMaxHeaderListSize(headerSize); HpackEncoder hpackEncoder = new HpackEncoder(true); Http2Headers toEncode = new DefaultHttp2Headers(); diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/HpackEncoderTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/HpackEncoderTest.java index b1dce05c38..de049a6e50 100644 --- a/codec-http2/src/test/java/io/netty/handler/codec/http2/HpackEncoderTest.java +++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/HpackEncoderTest.java @@ -74,7 +74,7 @@ public class HpackEncoderTest { try { hpackEncoder.encodeHeaders(0, buf, headersIn, Http2HeadersEncoder.NEVER_SENSITIVE); - hpackDecoder.setMaxHeaderListSize(bigHeaderSize + 1024, bigHeaderSize + 1024); + hpackDecoder.setMaxHeaderListSize(bigHeaderSize + 1024); hpackDecoder.decode(0, buf, headersOut, false); } finally { buf.release();