Skip to content

Commit

Permalink
Remove HpackDecoder.maxHeaderListSizeGoAway (netty#7911)
Browse files Browse the repository at this point in the history 
Motivation:

When a sender sends too large of headers it should not unnecessarily
kill the connection, as killing the connection is a heavy-handed
solution while SETTINGS_MAX_HEADER_LIST_SIZE is advisory and may be
ignored.

The maxHeaderListSizeGoAway limit in HpackDecoder is unnecessary because
any headers causing the list to exceeding the max size can simply be
thrown away. In addition, DefaultHttp2FrameReader.HeadersBlockBuilder
limits the entire block to maxHeaderListSizeGoAway. Thus individual
literals are limited to maxHeaderListSizeGoAway.

(Technically, literals are limited to 1.6x maxHeaderListSizeGoAway,
since the canonical Huffman code has a maximum compression ratio of
.625. However, the "unnecessary" limit in HpackDecoder was also being
applied to compressed sizes.)

Modifications:

Remove maxHeaderListSizeGoAway checking in HpackDecoder and instead
eagerly throw away any headers causing the list to exceed
maxHeaderListSize.

Result:

Fewer large header cases will trigger connection-killing.
DefaultHttp2FrameReader.HeadersBlockBuilder will still kill the
connection when maxHeaderListSizeGoAway is exceeded, however.

Fixes netty#7887
  • Loading branch information
@ejona86 @normanmaurer
ejona86 authored and normanmaurer committed May 19, 2018
1 parent 987c443 commit 88f0586
Show file tree
Hide file tree
Showing 4 changed files with 91 additions and 72 deletions.
13 changes: 11 additions & 2 deletions codec-http2/src/main/java/io/netty/handler/codec/http2/DefaultHttp2HeadersDecoder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@
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
Expand All @@ -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
Expand Down Expand Up @@ -79,6 +81,8 @@ public DefaultHttp2HeadersDecoder(boolean validateHeaders, long maxHeaderListSiz
DefaultHttp2HeadersDecoder(boolean validateHeaders, HpackDecoder hpackDecoder) {
this.hpackDecoder = ObjectUtil.checkNotNull(hpackDecoder, "hpackDecoder");
this.validateHeaders = validateHeaders;
this.maxHeaderListSizeGoAway =
Http2CodecUtil.calculateMaxHeaderListSizeGoAway(hpackDecoder.getMaxHeaderListSize());
}

@Override
Expand All @@ -93,7 +97,12 @@ public long maxHeaderTableSize() {

@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
Expand All @@ -103,7 +112,7 @@ public long maxHeaderListSize() {

@Override
public long maxHeaderListSizeGoAway() {
return hpackDecoder.getMaxHeaderListSizeGoAway();
return maxHeaderListSizeGoAway;
}

@Override
Expand Down
120 changes: 62 additions & 58 deletions codec-http2/src/main/java/io/netty/handler/codec/http2/HpackDecoder.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@
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;
Expand Down Expand Up @@ -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;
Expand All @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -151,8 +158,7 @@ public void decode(int streamId, ByteBuf in, Http2Headers headers, boolean valid
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
Expand Down Expand Up @@ -193,11 +199,11 @@ public void decode(int streamId, ByteBuf in, Http2Headers headers, boolean valid
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;
Expand All @@ -210,7 +216,7 @@ public void decode(int streamId, ByteBuf in, Http2Headers headers, boolean valid
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;

Expand All @@ -229,9 +235,6 @@ public void decode(int streamId, ByteBuf in, Http2Headers headers, boolean valid
if (index == 0x7f) {
state = READ_LITERAL_HEADER_NAME_LENGTH;
} else {
if (index > maxHeaderListSizeGoAway - headersLength) {
headerListSizeExceeded(maxHeaderListSizeGoAway);
}
nameLength = index;
state = READ_LITERAL_HEADER_NAME;
}
Expand All @@ -241,9 +244,6 @@ public void decode(int streamId, ByteBuf in, Http2Headers headers, boolean valid
// Header Name is a Literal String
nameLength = decodeULE128(in, index);

if (nameLength > maxHeaderListSizeGoAway - headersLength) {
headerListSizeExceeded(maxHeaderListSizeGoAway);
}
state = READ_LITERAL_HEADER_NAME;
break;

Expand All @@ -269,14 +269,10 @@ public void decode(int streamId, ByteBuf in, Http2Headers headers, boolean valid
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;
}
Expand All @@ -287,10 +283,6 @@ public void decode(int streamId, ByteBuf in, Http2Headers headers, boolean valid
// 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;

Expand All @@ -302,7 +294,7 @@ public void decode(int streamId, ByteBuf in, Http2Headers headers, boolean valid

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;

Expand All @@ -311,13 +303,6 @@ public void decode(int streamId, ByteBuf in, Http2Headers headers, boolean valid
}
}

// 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.");
}
Expand All @@ -341,27 +326,27 @@ public void setMaxHeaderTableSize(long maxHeaderTableSize) throws Http2Exception
}
}

/**
* @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.
Expand Down Expand Up @@ -450,9 +435,9 @@ private HpackHeaderField getIndexedHeader(int index) throws Http2Exception {
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:
Expand All @@ -466,18 +451,6 @@ private long insertHeader(Http2Headers headers, CharSequence name, CharSequence
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 {
Expand Down Expand Up @@ -553,4 +526,35 @@ private enum HeaderType {
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;
}
}
}
28 changes: 17 additions & 11 deletions codec-http2/src/test/java/io/netty/handler/codec/http2/HpackDecoderTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -431,24 +431,21 @@ public void testLiteralNeverIndexedWithLargeValue() throws Http2Exception {
}

@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");
toEncode.add("long", String.format("%0100d", 0).replace('0', 'A'));
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);
Expand All @@ -457,8 +454,18 @@ public void testDecodeLargerThanMaxHeaderListSizeButSmallerThanMaxHeaderListSize
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();
}
Expand All @@ -468,11 +475,10 @@ public void testDecodeLargerThanMaxHeaderListSizeButSmallerThanMaxHeaderListSize
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");
Expand All @@ -493,7 +499,7 @@ public void testAccountForHeaderOverhead() throws Exception {
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();
Expand Down
2 changes: 1 addition & 1 deletion codec-http2/src/test/java/io/netty/handler/codec/http2/HpackEncoderTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ public void testWillEncode16MBHeaderByDefault() throws Http2Exception {

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();
Expand Down

0 comments on commit 88f0586

Please sign in to comment.