Skip to content

Correct handling of unsigned HTTP/2 SETTINGS values #713

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.

Sign up for GitHub

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

Open
arturobernalg wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Correct handling of unsigned HTTP/2 SETTINGS values #713

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 31 additions & 27 deletions ...ore5-h2/src/main/java/org/apache/hc/core5/http2/impl/nio/AbstractH2StreamMultiplexer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -138,6 +138,9 @@ enum SettingsHandshake { READY, TRANSMITTED, ACKED }
private int lowMark;

private volatile H2Config remoteConfig;
private volatile int remoteHeaderTableSize;

@ok2c ok2c Oct 3, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arturobernalg Why do you need these variable at all? I do not get it. You can still use their signed int representation from H2Config and convert it to unsigned long only when doing some calculations.

private volatile int remoteMaxConcurrentStreams;
private volatile int remoteMaxHeaderListSize;

private Continuation continuation;

Expand Down Expand Up @@ -205,6 +208,9 @@ enum SettingsHandshake { READY, TRANSMITTED, ACKED }
this.hPackEncoder = new HPackEncoder(H2Config.INIT.getHeaderTableSize(), CharCodingSupport.createEncoder(charCodingConfig));
this.hPackDecoder = new HPackDecoder(H2Config.INIT.getHeaderTableSize(), CharCodingSupport.createDecoder(charCodingConfig));
this.remoteConfig = H2Config.INIT;
this.remoteHeaderTableSize = H2Config.INIT.getHeaderTableSize();
this.remoteMaxConcurrentStreams = H2Config.INIT.getMaxConcurrentStreams();
this.remoteMaxHeaderListSize = H2Config.INIT.getMaxHeaderListSize();
this.connInputWindow = new AtomicInteger(H2Config.INIT.getInitialWindowSize());
this.connOutputWindow = new AtomicInteger(H2Config.INIT.getInitialWindowSize());

Expand Down Expand Up @@ -526,11 +532,6 @@ public final void onOutput() throws HttpException, IOException {

if (connOutputWindow.get() > 0 && remoteSettingState == SettingsHandshake.ACKED) {
produceOutput();
} else {

@ok2c ok2c Oct 3, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arturobernalg Please be careful. You are reverting changes from a previous fix.

// RST_STREAM is not subject to flow control
for (final Iterator<H2Stream> it = streams.iterator(); it.hasNext(); ) {
it.next().resetIfCancelled();
}
}
final int pendingOutputRequests = outputRequests.get();
boolean outputPending = false;
Expand Down Expand Up @@ -576,7 +577,7 @@ public final void onOutput() throws HttpException, IOException {
})));
return;
}
while (streams.getLocalCount() < remoteConfig.getMaxConcurrentStreams()) {
while (streams.getLocalCount() < Integer.toUnsignedLong(remoteMaxConcurrentStreams)) {
final Command command = ioSession.poll();
if (command == null) {
break;
Expand Down Expand Up @@ -1089,7 +1090,7 @@ private void consumeFrame(final RawFrame frame) throws HttpException, IOExceptio
for (final Iterator<H2Stream> it = streams.iterator(); it.hasNext(); ) {
final H2Stream stream = it.next();
final int activeStreamId = stream.getId();
if (streams.isSameSide(activeStreamId) && activeStreamId > processedLocalStreamId) {
if (!streams.isSameSide(activeStreamId) && activeStreamId > processedLocalStreamId) {

@ok2c ok2c Oct 3, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@arturobernalg Please be careful. You are reverting changes from a previous fix

stream.fail(new RequestNotExecutedException());
it.remove();
}
Expand Down Expand Up @@ -1262,6 +1263,9 @@ private void consumeContinuationFrame(final RawFrame frame, final H2Stream strea

private void consumeSettingsFrame(final ByteBuffer payload) throws IOException {
final H2Config.Builder configBuilder = H2Config.initial();
int headerTableSize = remoteHeaderTableSize;
int maxConcurrentStreams = remoteMaxConcurrentStreams;
int maxHeaderListSize = remoteMaxHeaderListSize;
while (payload.hasRemaining()) {
final int code = payload.getShort();
final int value = payload.getInt();
Expand All @@ -1270,26 +1274,18 @@ private void consumeSettingsFrame(final ByteBuffer payload) throws IOException {
validateSetting(param, value);
switch (param) {
case HEADER_TABLE_SIZE:
try {
configBuilder.setHeaderTableSize(value);
} catch (final IllegalArgumentException ex) {
throw new H2ConnectionException(H2Error.PROTOCOL_ERROR, ex.getMessage());
}
headerTableSize = value;
break;
case MAX_CONCURRENT_STREAMS:
try {
configBuilder.setMaxConcurrentStreams(value);
} catch (final IllegalArgumentException ex) {
throw new H2ConnectionException(H2Error.PROTOCOL_ERROR, ex.getMessage());
}
maxConcurrentStreams = value;
break;
case ENABLE_PUSH:
configBuilder.setPushEnabled(value == 1);
break;
case INITIAL_WINDOW_SIZE:
if (value < 0) {
throw new H2ConnectionException(H2Error.FLOW_CONTROL_ERROR,
"Invalid initial window size: " + (value & 0xffffffffL));
"Invalid initial window size: " + Integer.toUnsignedLong(value));
}
try {
configBuilder.setInitialWindowSize(value);
Expand All @@ -1305,25 +1301,24 @@ private void consumeSettingsFrame(final ByteBuffer payload) throws IOException {
}
break;
case MAX_HEADER_LIST_SIZE:
try {
configBuilder.setMaxHeaderListSize(value);
} catch (final IllegalArgumentException ex) {
throw new H2ConnectionException(H2Error.PROTOCOL_ERROR, ex.getMessage());
}
maxHeaderListSize = value;
break;
case SETTINGS_NO_RFC7540_PRIORITIES:
peerNoRfc7540Priorities = value == 1;
break;
}
}
}
applyRemoteSettings(configBuilder.build());
applyRemoteSettings(
configBuilder.build(),
headerTableSize,
maxConcurrentStreams,
maxHeaderListSize);
}

private void produceOutput() throws HttpException, IOException {
for (final Iterator<H2Stream> it = streams.iterator(); it.hasNext(); ) {
final H2Stream stream = it.next();
stream.resetIfCancelled();
if (!stream.isLocalClosed() && !stream.isReserved() && stream.getOutputWindow().get() > 0) {
stream.produceOutput();
}
Expand All @@ -1339,12 +1334,21 @@ private void produceOutput() throws HttpException, IOException {
}
}

private void applyRemoteSettings(final H2Config config) throws H2ConnectionException {
private void applyRemoteSettings(
final H2Config config,
final int headerTableSize,
final int maxConcurrentStreams,
final int maxHeaderListSize) throws H2ConnectionException {
remoteConfig = config;
remoteHeaderTableSize = headerTableSize;
remoteMaxConcurrentStreams = maxConcurrentStreams;
remoteMaxHeaderListSize = maxHeaderListSize;

// The peer's HEADER_TABLE_SIZE is an upper bound for the encoder. Keep the local
// dynamic table bounded to limit memory usage and lookup cost per connection.
hPackEncoder.setMaxTableSize(Math.min(remoteConfig.getHeaderTableSize(), H2Config.INIT.getHeaderTableSize()));
hPackEncoder.setMaxTableSize((int) Math.min(
Integer.toUnsignedLong(remoteHeaderTableSize),
H2Config.INIT.getHeaderTableSize()));
final int delta = remoteConfig.getInitialWindowSize() - initOutputWinSize;
initOutputWinSize = remoteConfig.getInitialWindowSize();

Expand Down
Loading
Loading