Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove HpackDecoder.maxHeaderListSizeGoAway #7911

Merged
merged 1 commit into from May 19, 2018

Conversation

@ejona86
Copy link
Member

commented May 6, 2018

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 #7887


The Sink interface is a step toward splitting headerList processing out of the core Hpack decoding (verification is the only thing left). I can swap to referring to Http2HeadersSink directly, but it seemed more readable to use the interface as a "we're not assuming anything about the implementation."

CC @Scottmitch

@ejona86 ejona86 requested a review from normanmaurer May 6, 2018

goAwayMax, max);
}
hpackDecoder.setMaxHeaderListSize(max);
this.maxHeaderListSizeGoAway = max;

This comment has been minimized.

Copy link
@ejona86

ejona86 May 6, 2018

Author Member

s/max/goAwayMax/

This comment has been minimized.

Copy link
@normanmaurer

This comment has been minimized.

Copy link
@ejona86

ejona86 May 14, 2018

Author Member

It was a bug. goAwayMax wasn't used when it should have been. Fixed.

@@ -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 positive and >= %d",

This comment has been minimized.

Copy link
@fenik17

fenik17 May 6, 2018

Contributor

"positive or zero"?

This comment has been minimized.

Copy link
@ejona86

ejona86 May 6, 2018

Author Member

Yeah, looks like it should be "non-negative". This was pre-existing code and just moved, but I can update it.

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer May 7, 2018

Member

Whatever you prefer

This comment has been minimized.

Copy link
@ejona86

ejona86 May 14, 2018

Author Member

Done.

headerListSizeExceeded(streamId, maxHeaderListSize, true);
}
}

/**
* Decode the header block into header fields.
* <p>
* This method assumes the entire header block is contained in {@code in}.
*/

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer May 7, 2018

Member

@ejona86 move this to the public method (as it was before) ?

This comment has been minimized.

Copy link
@ejona86

ejona86 May 14, 2018

Author Member

Done. This had been public in my changes until the last minute. Oops.

@@ -341,27 +326,18 @@ public void setMaxHeaderTableSize(long maxHeaderTableSize) throws Http2Exception
}
}

public void setMaxHeaderListSize(long maxHeaderListSize, long maxHeaderListSizeGoAway) throws Http2Exception {

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer May 7, 2018

Member

@ejona86 should we just keep the method as well and deprecate it and ignore the second argument ?

This comment has been minimized.

Copy link
@ejona86

ejona86 May 14, 2018

Author Member

Unclear how many people are using that, but sure. It's easy. Done.

void appendToHeaderList(CharSequence name, CharSequence value);
}

private static class Http2HeadersSink implements Sink {

This comment has been minimized.

Copy link
@normanmaurer

This comment has been minimized.

Copy link
@ejona86

ejona86 May 14, 2018

Author Member

I don't tend to put final on private classes, but sure. Done.

goAwayMax, max);
}
hpackDecoder.setMaxHeaderListSize(max);
this.maxHeaderListSizeGoAway = max;

This comment has been minimized.

Copy link
@normanmaurer
@@ -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 positive and >= %d",

This comment has been minimized.

Copy link
@normanmaurer

normanmaurer May 7, 2018

Member

Whatever you prefer

@normanmaurer

This comment has been minimized.

Copy link
Member

commented May 7, 2018

@mosesn PTAL as well

@mosesn

This comment has been minimized.

Copy link
Contributor

commented May 7, 2018

I haven't looked at the PR like, but my vague memory of maxHeaderListSizeGoAway was that it was intended to help us defend against bad actors who might try to make the remote peer allocate too much memory in headers? Is that something we don't need to worry about?

@normanmaurer

This comment has been minimized.

Copy link
Member

commented May 11, 2018

@mosesn I think @ejona86 explained in his commit message why this is not "needed". Did you check?

@ejona86

This comment has been minimized.

Copy link
Member Author

commented May 11, 2018

@mosesn, check out the description, where I mention DefaultHttp2FrameReader. Also, this is specifically for maxHeaderListSizeGoAway, the "GoAway" portion being important. Pieces for maxHeaderListSize are still in place.

@mosesn

This comment has been minimized.

Copy link
Contributor

commented May 12, 2018

Got it, thanks for explaining.

@normanmaurer

This comment has been minimized.

Copy link
Member

commented May 12, 2018

So @mosesn wdyt about the changes ?

@mosesn
mosesn approved these changes May 12, 2018
Copy link
Contributor

left a comment

seems reasonable, shipit

@normanmaurer
Copy link
Member

left a comment

@ejona86 Please let me know once you addressed the comments

@ejona86

This comment has been minimized.

Copy link
Member Author

commented May 14, 2018

@normanmaurer, I've addressed the comments. I can squash when ready.

@normanmaurer normanmaurer self-assigned this May 15, 2018

@normanmaurer

This comment has been minimized.

Copy link
Member

commented May 15, 2018

@ejona86 please squash... LGTM

Remove HpackDecoder.maxHeaderListSizeGoAway
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 #7887

@ejona86 ejona86 force-pushed the ejona86:remove-hpack-goaway branch from ff96673 to 9cae436 May 15, 2018

@ejona86

This comment has been minimized.

Copy link
Member Author

commented May 15, 2018

Rebased + squashed

@normanmaurer normanmaurer added this to the 4.1.26.Final milestone May 19, 2018

@normanmaurer normanmaurer merged commit 88f0586 into netty:4.1 May 19, 2018

1 check passed

continuous-integration/teamcity Finished TeamCity Build pull requests :: netty : Tests passed: 13670, ignored: 120
Details
@normanmaurer

This comment has been minimized.

Copy link
Member

commented May 19, 2018

@ejona86 thanks!

@ejona86 ejona86 deleted the ejona86:remove-hpack-goaway branch Jun 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.