HTTPCORE-606: HTTP/2 protocol handler incorrectly uses larger frame size prior to SETTING handshake completion; revision of SETTINGS handshake implementation
diff --git a/httpcore5-h2/src/main/java/org/apache/hc/core5/http2/config/H2Config.java b/httpcore5-h2/src/main/java/org/apache/hc/core5/http2/config/H2Config.java
index 6fdbd35..27f8a34 100644
--- a/httpcore5-h2/src/main/java/org/apache/hc/core5/http2/config/H2Config.java
+++ b/httpcore5-h2/src/main/java/org/apache/hc/core5/http2/config/H2Config.java
@@ -40,7 +40,8 @@
@Contract(threading = ThreadingBehavior.IMMUTABLE)
public class H2Config {
- public static final H2Config DEFAULT = new Builder().build();
+ public static final H2Config DEFAULT = custom().build();
+ public static final H2Config INIT = initial().build();
private final int headerTableSize;
private final boolean pushEnabled;
@@ -101,6 +102,21 @@
return new Builder();
}
+ private static final int INIT_HEADER_TABLE_SIZE = 4096;
+ private static final boolean INIT_ENABLE_PUSH = true;
+ private static final int INIT_MAX_FRAME_SIZE = FrameConsts.MIN_FRAME_SIZE;
+ private static final int INIT_WINDOW_SIZE = 65535;
+
+ public static H2Config.Builder initial() {
+ return new Builder()
+ .setHeaderTableSize(INIT_HEADER_TABLE_SIZE)
+ .setPushEnabled(INIT_ENABLE_PUSH)
+ .setMaxConcurrentStreams(Integer.MAX_VALUE) // no limit
+ .setMaxFrameSize(INIT_MAX_FRAME_SIZE)
+ .setInitialWindowSize(INIT_WINDOW_SIZE)
+ .setHeaderTableSize(Integer.MAX_VALUE); // unlimited
+ }
+
public static H2Config.Builder copy(final H2Config config) {
Args.notNull(config, "Connection config");
return new Builder()
@@ -122,10 +138,10 @@
private int maxHeaderListSize;
Builder() {
- this.headerTableSize = 8192;
- this.pushEnabled = false;
- this.maxConcurrentStreams = 100;
- this.initialWindowSize = 65535;
+ this.headerTableSize = INIT_HEADER_TABLE_SIZE * 2;
+ this.pushEnabled = INIT_ENABLE_PUSH;
+ this.maxConcurrentStreams = 250;
+ this.initialWindowSize = INIT_WINDOW_SIZE;
this.maxFrameSize = FrameConsts.MIN_FRAME_SIZE * 4;
this.maxHeaderListSize = FrameConsts.MAX_FRAME_SIZE;
}
@@ -170,7 +186,7 @@
headerTableSize,
pushEnabled,
maxConcurrentStreams,
- initialWindowSize > 0 ? initialWindowSize : 65535,
+ initialWindowSize,
maxFrameSize,
maxHeaderListSize);
}
diff --git a/httpcore5-h2/src/main/java/org/apache/hc/core5/http2/impl/nio/AbstractH2StreamMultiplexer.java b/httpcore5-h2/src/main/java/org/apache/hc/core5/http2/impl/nio/AbstractH2StreamMultiplexer.java
index 9ebfaf4..7e6bb0b 100644
--- a/httpcore5-h2/src/main/java/org/apache/hc/core5/http2/impl/nio/AbstractH2StreamMultiplexer.java
+++ b/httpcore5-h2/src/main/java/org/apache/hc/core5/http2/impl/nio/AbstractH2StreamMultiplexer.java
@@ -122,7 +122,12 @@
private ConnectionHandshake connState = ConnectionHandshake.READY;
private SettingsHandshake localSettingState = SettingsHandshake.READY;
private SettingsHandshake remoteSettingState = SettingsHandshake.READY;
- private H2Config remoteConfig;
+
+ private int initInputWinSize;
+ private int initOutputWinSize;
+
+ private volatile H2Config remoteConfig;
+
private int lowMark;
private Continuation continuation;
@@ -145,7 +150,7 @@
this.localConfig = h2Config != null ? h2Config : H2Config.DEFAULT;
this.inputMetrics = new BasicH2TransportMetrics();
this.outputMetrics = new BasicH2TransportMetrics();
- this.connMetrics = new BasicHttpConnectionMetrics(inputMetrics, outputMetrics);
+ this.connMetrics = new BasicHttpConnectionMetrics(this.inputMetrics, this.outputMetrics);
this.inputBuffer = new FrameInputBuffer(this.inputMetrics, this.localConfig.getMaxFrameSize());
this.outputBuffer = new FrameOutputBuffer(this.outputMetrics, this.localConfig.getMaxFrameSize());
this.outputQueue = new ConcurrentLinkedDeque<>();
@@ -155,14 +160,17 @@
this.hPackEncoder = new HPackEncoder(CharCodingSupport.createEncoder(charCodingConfig));
this.hPackDecoder = new HPackDecoder(CharCodingSupport.createDecoder(charCodingConfig));
this.streamMap = new ConcurrentHashMap<>();
- this.connInputWindow = new AtomicInteger(localConfig.getInitialWindowSize());
- this.connOutputWindow = new AtomicInteger(H2Config.DEFAULT.getInitialWindowSize());
+ this.remoteConfig = H2Config.INIT;
+ this.connInputWindow = new AtomicInteger(H2Config.INIT.getInitialWindowSize());
+ this.connOutputWindow = new AtomicInteger(H2Config.INIT.getInitialWindowSize());
- this.hPackDecoder.setMaxTableSize(H2Config.DEFAULT.getInitialWindowSize());
- this.hPackEncoder.setMaxTableSize(this.localConfig.getHeaderTableSize());
+ this.initInputWinSize = H2Config.INIT.getInitialWindowSize();
+ this.initOutputWinSize = H2Config.INIT.getInitialWindowSize();
- this.remoteConfig = H2Config.DEFAULT;
- this.lowMark = this.remoteConfig.getInitialWindowSize() / 2;
+ this.hPackDecoder.setMaxTableSize(H2Config.INIT.getHeaderTableSize());
+ this.hPackEncoder.setMaxTableSize(H2Config.INIT.getHeaderTableSize());
+
+ this.lowMark = H2Config.INIT.getInitialWindowSize() / 2;
this.streamListener = streamListener;
}
@@ -341,8 +349,7 @@
if (capacity <= 0) {
return 0;
}
- final int frameSize = Math.max(localConfig.getMaxFrameSize(), remoteConfig.getMaxFrameSize());
- final int maxPayloadSize = Math.min(capacity, frameSize);
+ final int maxPayloadSize = Math.min(capacity, remoteConfig.getMaxFrameSize());
final int chunk;
if (payload.remaining() <= maxPayloadSize) {
chunk = payload.remaining();
@@ -461,7 +468,7 @@
final int connWinSize = connInputWindow.get();
if (connWinSize < lowMark) {
- final int delta = this.remoteConfig.getInitialWindowSize() - connWinSize;
+ final int delta = initInputWinSize - connWinSize;
if (delta > 0) {
final RawFrame windowUpdateFrame = frameFactory.createWindowUpdate(0, delta);
commitFrame(windowUpdateFrame);
@@ -610,11 +617,7 @@
} else if (command instanceof ExecutableCommand) {
final int streamId = generateStreamId();
final H2StreamChannelImpl channel = new H2StreamChannelImpl(
- streamId,
- true,
- localConfig.getInitialWindowSize(),
- remoteConfig.getInitialWindowSize());
-
+ streamId, true, initInputWinSize, initOutputWinSize);
final ExecutableCommand executableCommand = (ExecutableCommand) command;
final H2StreamHandler streamHandler = createLocallyInitiatedStream(
executableCommand, channel, httpProcessor, connMetrics);
@@ -757,10 +760,7 @@
updateLastStreamId(streamId);
final H2StreamChannelImpl channel = new H2StreamChannelImpl(
- streamId,
- false,
- localConfig.getInitialWindowSize(),
- remoteConfig.getInitialWindowSize());
+ streamId, false, initInputWinSize, initOutputWinSize);
final H2StreamHandler streamHandler = createRemotelyInitiatedStream(
channel, httpProcessor, connMetrics, null);
stream = new H2Stream(channel, streamHandler, true);
@@ -891,6 +891,7 @@
if (localSettingState == SettingsHandshake.TRANSMITTED) {
localSettingState = SettingsHandshake.ACKED;
ioSession.setEvent(SelectionKey.OP_WRITE);
+ applyLocalSettings();
}
} else {
final ByteBuffer payload = frame.getPayload();
@@ -939,10 +940,7 @@
updateLastStreamId(promisedStreamId);
final H2StreamChannelImpl channel = new H2StreamChannelImpl(
- promisedStreamId,
- false,
- localConfig.getInitialWindowSize(),
- remoteConfig.getInitialWindowSize());
+ promisedStreamId, false, initInputWinSize, initOutputWinSize);
final H2StreamHandler streamHandler = createRemotelyInitiatedStream(
channel, httpProcessor, connMetrics, stream.getPushHandlerFactory());
final H2Stream promisedStream = new H2Stream(channel, streamHandler, true);
@@ -1119,7 +1117,7 @@
}
private void consumeSettingsFrame(final ByteBuffer payload) throws HttpException, IOException {
- final H2Config.Builder configBuilder = H2Config.custom();
+ final H2Config.Builder configBuilder = H2Config.initial();
while (payload.hasRemaining()) {
final int code = payload.getShort();
final H2Param param = H2Param.valueOf(code);
@@ -1127,8 +1125,11 @@
final int value = payload.getInt();
switch (param) {
case HEADER_TABLE_SIZE:
- configBuilder.setHeaderTableSize(value);
- hPackDecoder.setMaxTableSize(value);
+ try {
+ configBuilder.setHeaderTableSize(value);
+ } catch (final IllegalArgumentException ex) {
+ throw new H2ConnectionException(H2Error.PROTOCOL_ERROR, ex.getMessage());
+ }
break;
case MAX_CONCURRENT_STREAMS:
try {
@@ -1146,21 +1147,6 @@
} catch (final IllegalArgumentException ex) {
throw new H2ConnectionException(H2Error.PROTOCOL_ERROR, ex.getMessage());
}
- final int delta = value - remoteConfig.getInitialWindowSize();
- if (delta != 0) {
- updateOutputWindow(0, connOutputWindow, delta);
- if (!streamMap.isEmpty()) {
- for (final Iterator<Map.Entry<Integer, H2Stream>> it = streamMap.entrySet().iterator(); it.hasNext(); ) {
- final Map.Entry<Integer, H2Stream> entry = it.next();
- final H2Stream stream = entry.getValue();
- try {
- updateOutputWindow(stream.getId(), stream.getOutputWindow(), delta);
- } catch (final ArithmeticException ex) {
- throw new H2ConnectionException(H2Error.FLOW_CONTROL_ERROR, ex.getMessage());
- }
- }
- }
- }
break;
case MAX_FRAME_SIZE:
try {
@@ -1179,8 +1165,7 @@
}
}
}
- remoteConfig = configBuilder.build();
- lowMark = remoteConfig.getInitialWindowSize() / 2;
+ applyRemoteSettings(configBuilder.build());
}
private void produceOutput() throws HttpException, IOException {
@@ -1200,6 +1185,52 @@
}
}
+ private void applyRemoteSettings(final H2Config config) throws H2ConnectionException {
+ remoteConfig = config;
+
+ hPackDecoder.setMaxTableSize(remoteConfig.getMaxHeaderListSize());
+ final int delta = remoteConfig.getInitialWindowSize() - initOutputWinSize;
+ initOutputWinSize = remoteConfig.getInitialWindowSize();
+
+ if (delta != 0) {
+ updateOutputWindow(0, connOutputWindow, delta);
+ if (!streamMap.isEmpty()) {
+ for (final Iterator<Map.Entry<Integer, H2Stream>> it = streamMap.entrySet().iterator(); it.hasNext(); ) {
+ final Map.Entry<Integer, H2Stream> entry = it.next();
+ final H2Stream stream = entry.getValue();
+ try {
+ updateOutputWindow(stream.getId(), stream.getOutputWindow(), delta);
+ } catch (final ArithmeticException ex) {
+ throw new H2ConnectionException(H2Error.FLOW_CONTROL_ERROR, ex.getMessage());
+ }
+ }
+ }
+ }
+ lowMark = initOutputWinSize / 2;
+ }
+
+ private void applyLocalSettings() throws H2ConnectionException {
+ hPackEncoder.setMaxTableSize(localConfig.getMaxHeaderListSize());
+
+ final int delta = localConfig.getInitialWindowSize() - initInputWinSize;
+ initInputWinSize = localConfig.getInitialWindowSize();
+
+ if (delta != 0) {
+ updateInputWindow(0, connInputWindow, delta);
+ if (!streamMap.isEmpty()) {
+ for (final Iterator<Map.Entry<Integer, H2Stream>> it = streamMap.entrySet().iterator(); it.hasNext(); ) {
+ final Map.Entry<Integer, H2Stream> entry = it.next();
+ final H2Stream stream = entry.getValue();
+ try {
+ updateInputWindow(stream.getId(), stream.getInputWindow(), delta);
+ } catch (final ArithmeticException ex) {
+ throw new H2ConnectionException(H2Error.FLOW_CONTROL_ERROR, ex.getMessage());
+ }
+ }
+ }
+ }
+ }
+
@Override
public void close() throws IOException {
ioSession.enqueue(ShutdownCommand.GRACEFUL, Command.Priority.IMMEDIATE);
diff --git a/httpcore5-h2/src/main/java/org/apache/hc/core5/http2/impl/nio/FrameInputBuffer.java b/httpcore5-h2/src/main/java/org/apache/hc/core5/http2/impl/nio/FrameInputBuffer.java
index 87576a3..cb5e381 100644
--- a/httpcore5-h2/src/main/java/org/apache/hc/core5/http2/impl/nio/FrameInputBuffer.java
+++ b/httpcore5-h2/src/main/java/org/apache/hc/core5/http2/impl/nio/FrameInputBuffer.java
@@ -65,7 +65,7 @@
Args.notNull(metrics, "HTTP2 transport metrcis");
Args.positive(maxFramePayloadSize, "Maximum payload size");
this.metrics = metrics;
- this.maxFramePayloadSize = maxFramePayloadSize;
+ this.maxFramePayloadSize = Math.max(maxFramePayloadSize, FrameConsts.MIN_FRAME_SIZE);
this.bytes = new byte[bufferLen];
this.buffer = ByteBuffer.wrap(bytes);
this.buffer.flip();
