From 4145fae91909ee42be1013290d2a6873f7f14eef Mon Sep 17 00:00:00 2001 From: Scott Mitchell Date: Fri, 12 Aug 2016 19:27:32 -0700 Subject: [PATCH] HTTP/2 HPACK Decoder VarInt Improvement Motivation: HTTP/2 Decoder#decodeULE128 current will tolerate more bytes than necessary when attempted to detect overflow. The usage of this method also currently requires an additional overflow conditional. Modifications: - Integrate the first byte into Decoder#decodeULE128 which allows us to detect overflow reliably and avoid overflow checks outside of this method. Result: Less conditionals and earlier overflow detection in Decoder#decodeULE128 --- .../codec/http2/internal/hpack/Decoder.java | 51 ++++++------------- .../http2/internal/hpack/DecoderTest.java | 13 ++++- 2 files changed, 27 insertions(+), 37 deletions(-) diff --git a/codec-http2/src/main/java/io/netty/handler/codec/http2/internal/hpack/Decoder.java b/codec-http2/src/main/java/io/netty/handler/codec/http2/internal/hpack/Decoder.java index a4dc6c0be2..7824f37337 100644 --- a/codec-http2/src/main/java/io/netty/handler/codec/http2/internal/hpack/Decoder.java +++ b/codec-http2/src/main/java/io/netty/handler/codec/http2/internal/hpack/Decoder.java @@ -39,6 +39,7 @@ import io.netty.util.AsciiString; import static io.netty.handler.codec.http2.Http2Error.COMPRESSION_ERROR; import static io.netty.handler.codec.http2.Http2Error.ENHANCE_YOUR_CALM; +import static io.netty.handler.codec.http2.Http2Error.valueOf; import static io.netty.handler.codec.http2.Http2Exception.connectionError; import static io.netty.util.AsciiString.EMPTY_STRING; import static io.netty.util.internal.ThrowableUtil.unknownStackTrace; @@ -173,36 +174,18 @@ public final class Decoder { break; case READ_MAX_DYNAMIC_TABLE_SIZE: - int maxSize = decodeULE128(in) + index; - - if (maxSize < 0) { // Check for numerical overflow - throw DECODE_DECOMPRESSION_EXCEPTION; - } - - setDynamicTableSize(maxSize); + setDynamicTableSize(decodeULE128(in, index)); state = READ_HEADER_REPRESENTATION; break; case READ_INDEXED_HEADER: - int headerIndex = decodeULE128(in) + index; - - if (headerIndex < 0) { // Check for numerical overflow - throw DECODE_DECOMPRESSION_EXCEPTION; - } - - headersLength = indexHeader(headerIndex, headers, headersLength); + headersLength = indexHeader(decodeULE128(in, index), headers, headersLength); state = READ_HEADER_REPRESENTATION; break; case READ_INDEXED_HEADER_NAME: // Header Name matches an entry in the Header Table - int nameIndex = decodeULE128(in) + index; - - if (nameIndex < 0) { // Check for numerical overflow - throw DECODE_DECOMPRESSION_EXCEPTION; - } - - name = readName(nameIndex); + name = readName(decodeULE128(in, index)); state = READ_LITERAL_HEADER_VALUE_LENGTH_PREFIX; break; @@ -223,11 +206,7 @@ public final class Decoder { case READ_LITERAL_HEADER_NAME_LENGTH: // Header Name is a Literal String - nameLength = decodeULE128(in) + index; - - if (nameLength < 0) { // Check for numerical overflow - throw DECODE_DECOMPRESSION_EXCEPTION; - } + nameLength = decodeULE128(in, index); if (nameLength > maxHeadersLength - headersLength) { maxHeaderSizeExceeded(); @@ -271,11 +250,7 @@ public final class Decoder { case READ_LITERAL_HEADER_VALUE_LENGTH: // Header Value is a Literal String - valueLength = decodeULE128(in) + index; - - if (valueLength < 0) { // Check for numerical overflow - throw DECODE_DECOMPRESSION_EXCEPTION; - } + valueLength = decodeULE128(in, index); // Check new header size against max header size if ((long) valueLength + nameLength > maxHeadersLength - headersLength) { @@ -429,21 +404,25 @@ public final class Decoder { } // Unsigned Little Endian Base 128 Variable-Length Integer Encoding - private static int decodeULE128(ByteBuf in) throws Http2Exception { + private static int decodeULE128(ByteBuf in, int result) throws Http2Exception { + assert result <= 0x7f && result >= 0; final int writerIndex = in.writerIndex(); - for (int readerIndex = in.readerIndex(), shift = 0, result = 0; + for (int readerIndex = in.readerIndex(), shift = 0; readerIndex < writerIndex; ++readerIndex, shift += 7) { byte b = in.getByte(readerIndex); - if (shift == 28 && (b & 0xF8) != 0) { + if (shift == 28 && ((b & 0x80) != 0 || b > 6)) { + // the maximum value that can be represented by a signed 32 bit number is: + // 0x7f + 0x7f + (0x7f << 7) + (0x7f << 14) + (0x7f << 21) + (0x6 << 28) + // this means any more shifts will result in overflow so we should break out and throw an error. in.readerIndex(readerIndex + 1); break; } if ((b & 0x80) == 0) { in.readerIndex(readerIndex + 1); - return result | ((b & 0x7F) << shift); + return result + ((b & 0x7F) << shift); } - result |= (b & 0x7F) << shift; + result += (b & 0x7F) << shift; } throw DECODE_ULE_128_DECOMPRESSION_EXCEPTION; diff --git a/codec-http2/src/test/java/io/netty/handler/codec/http2/internal/hpack/DecoderTest.java b/codec-http2/src/test/java/io/netty/handler/codec/http2/internal/hpack/DecoderTest.java index f3d449fad8..ea88899dd1 100644 --- a/codec-http2/src/test/java/io/netty/handler/codec/http2/internal/hpack/DecoderTest.java +++ b/codec-http2/src/test/java/io/netty/handler/codec/http2/internal/hpack/DecoderTest.java @@ -40,6 +40,7 @@ import org.junit.Test; import static io.netty.util.AsciiString.EMPTY_STRING; import static io.netty.util.AsciiString.of; +import static java.lang.Integer.MAX_VALUE; import static org.junit.Assert.assertEquals; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.reset; @@ -148,7 +149,7 @@ public class DecoderTest { @Test(expected = Http2Exception.class) public void testInsidiousIndex() throws Http2Exception { // Insidious index so the last shift causes sign overflow - decode("FF8080808008"); + decode("FF8080808007"); } @Test @@ -174,10 +175,20 @@ public class DecoderTest { @Test(expected = Http2Exception.class) public void testInsidiousMaxDynamicTableSize() throws Http2Exception { + decoder.setMaxHeaderTableSize(MAX_VALUE); // max header table size sign overflow decode("3FE1FFFFFF07"); } + @Test + public void testMaxValidDynamicTableSize() throws Http2Exception { + decoder.setMaxHeaderTableSize(MAX_VALUE); + String baseValue = "3FE1FFFFFF0"; + for (int i = 0; i < 7; ++i) { + decode(baseValue + i); + } + } + @Test public void testReduceMaxDynamicTableSize() throws Http2Exception { decoder.setMaxHeaderTableSize(0);