From 620ac681b3571c083d2cb4637f8eefa117daa9b8 Mon Sep 17 00:00:00 2001 From: Arturo Bernal Date: Wed, 30 Sep 2026 13:23:58 +0200 Subject: [PATCH] Correct handling of unsigned HTTP/2 SETTINGS values --- .../hc/core5/http2/config/H2Config.java | 5 +- .../impl/nio/AbstractH2StreamMultiplexer.java | 13 +- .../nio/TestAbstractH2StreamMultiplexer.java | 299 +++++++----------- 3 files changed, 122 insertions(+), 195 deletions(-) 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 89b538860..b8a2c376d 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 @@ -168,7 +168,6 @@ public static class Builder { } public Builder setHeaderTableSize(final int headerTableSize) { - Args.notNegative(headerTableSize, "Header table size"); this.headerTableSize = headerTableSize; return this; } @@ -179,7 +178,7 @@ public Builder setPushEnabled(final boolean pushEnabled) { } public Builder setMaxConcurrentStreams(final int maxConcurrentStreams) { - this.maxConcurrentStreams = Args.checkRange(maxConcurrentStreams, 0, Integer.MAX_VALUE, "Max concurrent streams"); + this.maxConcurrentStreams = maxConcurrentStreams; return this; } @@ -195,7 +194,7 @@ public Builder setMaxFrameSize(final int maxFrameSize) { } public Builder setMaxHeaderListSize(final int maxHeaderListSize) { - this.maxHeaderListSize = Args.checkRange(maxHeaderListSize, 0, Integer.MAX_VALUE, "Max header list size"); + this.maxHeaderListSize = maxHeaderListSize; return this; } 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 c71026148..751bcb6e4 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 @@ -576,7 +576,7 @@ public final void onOutput() throws HttpException, IOException { }))); return; } - while (streams.getLocalCount() < remoteConfig.getMaxConcurrentStreams()) { + while (streams.getLocalCount() < Integer.toUnsignedLong(remoteConfig.getMaxConcurrentStreams())) { final Command command = ioSession.poll(); if (command == null) { break; @@ -1289,7 +1289,7 @@ private void consumeSettingsFrame(final ByteBuffer payload) throws IOException { 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); @@ -1319,7 +1319,6 @@ private void consumeSettingsFrame(final ByteBuffer payload) throws IOException { } applyRemoteSettings(configBuilder.build()); } - private void produceOutput() throws HttpException, IOException { for (final Iterator it = streams.iterator(); it.hasNext(); ) { final H2Stream stream = it.next(); @@ -1341,13 +1340,13 @@ private void produceOutput() throws HttpException, IOException { private void applyRemoteSettings(final H2Config config) throws H2ConnectionException { remoteConfig = config; - // 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(remoteConfig.getHeaderTableSize()), + H2Config.INIT.getHeaderTableSize())); final int delta = remoteConfig.getInitialWindowSize() - initOutputWinSize; initOutputWinSize = remoteConfig.getInitialWindowSize(); - final int maxFrameSize = remoteConfig.getMaxFrameSize(); if (maxFrameSize < outputBuffer.getMaxFramePayloadSize()) { try { @@ -1356,7 +1355,6 @@ private void applyRemoteSettings(final H2Config config) throws H2ConnectionExcep throw new H2ConnectionException(H2Error.INTERNAL_ERROR, "Failure resizing the frame output buffer"); } } - if (delta != 0) { if (!streams.isEmpty()) { for (final Iterator it = streams.iterator(); it.hasNext(); ) { @@ -1370,7 +1368,6 @@ private void applyRemoteSettings(final H2Config config) throws H2ConnectionExcep } } } - private void applyLocalSettings() throws H2ConnectionException { hPackDecoder.setMaxTableSize(localConfig.getHeaderTableSize()); hPackDecoder.setMaxListSize(localConfig.getMaxHeaderListSize()); diff --git a/httpcore5-h2/src/test/java/org/apache/hc/core5/http2/impl/nio/TestAbstractH2StreamMultiplexer.java b/httpcore5-h2/src/test/java/org/apache/hc/core5/http2/impl/nio/TestAbstractH2StreamMultiplexer.java index 6b665c3f0..51af5efbd 100644 --- a/httpcore5-h2/src/test/java/org/apache/hc/core5/http2/impl/nio/TestAbstractH2StreamMultiplexer.java +++ b/httpcore5-h2/src/test/java/org/apache/hc/core5/http2/impl/nio/TestAbstractH2StreamMultiplexer.java @@ -42,7 +42,6 @@ import org.apache.hc.core5.http.Header; import org.apache.hc.core5.http.HttpException; import org.apache.hc.core5.http.HttpHeaders; -import org.apache.hc.core5.http.RequestNotExecutedException; import org.apache.hc.core5.http.config.CharCodingConfig; import org.apache.hc.core5.http.impl.CharCodingSupport; import org.apache.hc.core5.http.message.BasicHeader; @@ -74,7 +73,6 @@ import org.apache.hc.core5.reactor.ProtocolIOSession; import org.apache.hc.core5.util.ByteArrayBuffer; import org.apache.hc.core5.util.Timeout; -import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; @@ -104,20 +102,12 @@ class TestAbstractH2StreamMultiplexer { @Captor ArgumentCaptor exceptionCaptor; - AutoCloseable closeable; - @BeforeEach void prepareMocks() { - closeable = MockitoAnnotations.openMocks(this); + MockitoAnnotations.openMocks(this); Mockito.when(protocolIOSession.getLock()).thenReturn(lock); } - @AfterEach - void releaseMocks() throws Exception { - if (closeable != null) { - closeable.close(); - } - } static class H2StreamMultiplexerImpl extends AbstractH2StreamMultiplexer { private Supplier streamHandlerSupplier; @@ -780,7 +770,7 @@ void testInputHeaderContinuationFramesMaxLimit() throws Exception { outBuffer.write(continuationFrame3, writableChannel); Assertions.assertThrows(H2ConnectionException.class, () -> - streamMultiplexer.onInput(ByteBuffer.wrap(writableChannel.toByteArray()))); + streamMultiplexer.onInput(ByteBuffer.wrap(writableChannel.toByteArray()))); } @Test @@ -1027,7 +1017,7 @@ void testSubmitWithPriorityHeaderEmitsPriorityUpdateBeforeHeaders() throws Excep new BasicHeader(":authority", "example.test"), new BasicHeader(HttpHeaders.PRIORITY, "u=3,i") ); - mux.createStream(ch, new PriorityHeaderSender(ch, reqHeaders, true)); + mux.createStream(ch, new PriorityHeaderSender(ch, reqHeaders, true)); // Drive output so the handler submits mux.onOutput(); @@ -1197,118 +1187,6 @@ void testStreamIdleTimeoutTriggersH2StreamTimeoutException() throws Exception { } - @Test - void testAbortAfterLocalEndStreamSendsRstStreamWithoutInboundFrames() throws Exception { - final List writes = new ArrayList<>(); - Mockito.when(protocolIOSession.write(ArgumentMatchers.any(ByteBuffer.class))) - .thenAnswer(inv -> { - final ByteBuffer b = inv.getArgument(0, ByteBuffer.class); - final byte[] copy = new byte[b.remaining()]; - b.get(copy); - writes.add(copy); - return copy.length; - }); - Mockito.doNothing().when(protocolIOSession).setEvent(ArgumentMatchers.anyInt()); - Mockito.doNothing().when(protocolIOSession).clearEvent(ArgumentMatchers.anyInt()); - - final H2Config h2Config = H2Config.custom().build(); - final H2StreamMultiplexerImpl mux = new H2StreamMultiplexerImpl( - protocolIOSession, FRAME_FACTORY, StreamIdGenerator.ODD, - httpProcessor, CharCodingConfig.DEFAULT, h2Config, h2StreamListener, () -> streamHandler); - - mux.onConnect(); - final WritableByteChannelMock writable = new WritableByteChannelMock(256); - final FrameOutputBuffer fob = new FrameOutputBuffer(16 * 1024); - fob.write(new RawFrame(FrameType.SETTINGS.getValue(), 0, 0, null), writable); - mux.onInput(ByteBuffer.wrap(writable.toByteArray())); - writes.clear(); - - // A request without a body: HEADERS carry END_STREAM, so the stream is half-closed (local) - final H2StreamChannel channel = mux.createChannel(1); - final List
requestHeaders = Arrays.asList( - new BasicHeader(":method", "GET"), - new BasicHeader(":scheme", "https"), - new BasicHeader(":path", "/"), - new BasicHeader(":authority", "example.test")); - final H2Stream stream = mux.createStream(channel, new PriorityHeaderSender(channel, requestHeaders, true)); - mux.onOutput(); - Assertions.assertTrue(stream.isLocalClosed()); - writes.clear(); - - // The request gets cancelled while the peer stays silent - stream.abort(); - mux.onOutput(); - - final List frames = parseFrames(concat(writes)); - final FrameStub rst = frames.stream() - .filter(f -> f.type == FrameType.RST_STREAM.getValue() && f.streamId == 1) - .findFirst() - .orElse(null); - Assertions.assertNotNull(rst, "RST_STREAM not emitted for the cancelled stream"); - Assertions.assertEquals(H2Error.CANCEL.getCode(), ByteBuffer.wrap(rst.payload).getInt()); - Assertions.assertTrue(channel.isLocalReset()); - } - - @Test - void testAbortAfterLocalEndStreamSendsRstStreamWithNoConnectionWindow() throws Exception { - final List writes = new ArrayList<>(); - Mockito.when(protocolIOSession.write(ArgumentMatchers.any(ByteBuffer.class))) - .thenAnswer(inv -> { - final ByteBuffer b = inv.getArgument(0, ByteBuffer.class); - final byte[] copy = new byte[b.remaining()]; - b.get(copy); - writes.add(copy); - return copy.length; - }); - Mockito.doNothing().when(protocolIOSession).setEvent(ArgumentMatchers.anyInt()); - Mockito.doNothing().when(protocolIOSession).clearEvent(ArgumentMatchers.anyInt()); - - final H2Config h2Config = H2Config.custom().build(); - final H2StreamMultiplexerImpl mux = new H2StreamMultiplexerImpl( - protocolIOSession, FRAME_FACTORY, StreamIdGenerator.ODD, - httpProcessor, CharCodingConfig.DEFAULT, h2Config, h2StreamListener, () -> streamHandler); - - mux.onConnect(); - final WritableByteChannelMock writable = new WritableByteChannelMock(256); - final FrameOutputBuffer fob = new FrameOutputBuffer(16 * 1024); - fob.write(new RawFrame(FrameType.SETTINGS.getValue(), 0, 0, null), writable); - mux.onInput(ByteBuffer.wrap(writable.toByteArray())); - - final List
requestHeaders = Arrays.asList( - new BasicHeader(":method", "GET"), - new BasicHeader(":scheme", "https"), - new BasicHeader(":path", "/"), - new BasicHeader(":authority", "example.test")); - // Stream 1: a request without a body, half-closed (local) once its HEADERS are sent - final H2StreamChannel channel = mux.createChannel(1); - final H2Stream stream = mux.createStream(channel, new PriorityHeaderSender(channel, requestHeaders, true)); - // Stream 3: a request with a body that uses up the connection output window - final H2StreamChannel uploadChannel = mux.createChannel(3); - mux.createStream(uploadChannel, new PriorityHeaderSender(uploadChannel, requestHeaders, false)); - mux.onOutput(); - Assertions.assertTrue(stream.isLocalClosed()); - - final ByteBuffer body = ByteBuffer.allocate(h2Config.getInitialWindowSize()); - while (body.hasRemaining()) { - Assertions.assertTrue(uploadChannel.write(body) > 0, "connection window used up too early"); - } - Assertions.assertEquals(0, uploadChannel.write(ByteBuffer.allocate(1)), "connection window must be 0"); - writes.clear(); - - // The request gets cancelled while the peer stays silent and grants no window - stream.abort(); - mux.onOutput(); - - final List frames = parseFrames(concat(writes)); - final FrameStub rst = frames.stream() - .filter(f -> f.type == FrameType.RST_STREAM.getValue() && f.streamId == 1) - .findFirst() - .orElse(null); - Assertions.assertNotNull(rst, "RST_STREAM not emitted while the connection window is 0"); - Assertions.assertEquals(H2Error.CANCEL.getCode(), ByteBuffer.wrap(rst.payload).getInt()); - Assertions.assertTrue(channel.isLocalReset()); - } - @Test void testResetIfExpiredResetsStreamPastDeadline() throws Exception { final H2Config h2Config = H2Config.custom().build(); @@ -2139,81 +2017,38 @@ void testPushPromiseReservedBitInPromisedStreamIdIgnored() throws Exception { } @Test - void testGoAwayReservedBitInLastStreamIdAffectsStreamCulling() throws Exception { - final H2Config h2Config = H2Config.custom().build(); - + void testGoAwayReservedBitInLastStreamIdIgnored() throws Exception { final AbstractH2StreamMultiplexer mux = new H2StreamMultiplexerImpl( protocolIOSession, FRAME_FACTORY, - StreamIdGenerator.ODD, // local=odd, remote=even - httpProcessor, - CharCodingConfig.DEFAULT, - h2Config, - h2StreamListener, - () -> streamHandler); - - // Create 3 local (odd) streams: 1, 3, 5. - final H2StreamHandler streamHandler1 = Mockito.mock(H2StreamHandler.class); - final H2StreamHandler streamHandler3 = Mockito.mock(H2StreamHandler.class); - final H2StreamHandler streamHandler5 = Mockito.mock(H2StreamHandler.class); - mux.createStream(mux.createChannel(1), streamHandler1); - mux.createStream(mux.createChannel(3), streamHandler3); - mux.createStream(mux.createChannel(5), streamHandler5); - - // GOAWAY last-stream-id = 4, but with reserved MSB set. - // Correct masking keeps streams <= 4 (1 and 3) and drops only stream 5. - final ByteBuffer goAwayPayload = ByteBuffer.allocate(8); - goAwayPayload.putInt(0x80000004); // reserved bit set, last-stream-id = 4 - goAwayPayload.putInt(H2Error.NO_ERROR.getCode()); - goAwayPayload.flip(); - - final RawFrame goAway = new RawFrame(FrameType.GOAWAY.getValue(), 0, 0, goAwayPayload); - - Assertions.assertDoesNotThrow(() -> mux.onInput(ByteBuffer.wrap(encodeFrame(goAway)))); - - Mockito.verify(streamHandler5, Mockito.times(1)).failed(exceptionCaptor.capture()); - Assertions.assertInstanceOf(RequestNotExecutedException.class, exceptionCaptor.getValue()); - Mockito.verify(streamHandler1, Mockito.never()).failed(ArgumentMatchers.any()); - Mockito.verify(streamHandler3, Mockito.never()).failed(ArgumentMatchers.any()); - } - - @Test - void testGoAwayNoErrorFailsUnprocessedLocalStreamsOnly() throws Exception { - final H2Config h2Config = H2Config.custom().build(); - - final AbstractH2StreamMultiplexer mux = new H2StreamMultiplexerImpl( - protocolIOSession, - FRAME_FACTORY, - StreamIdGenerator.ODD, // local=odd, remote=even + StreamIdGenerator.ODD, httpProcessor, CharCodingConfig.DEFAULT, - h2Config, + H2Config.custom().build(), h2StreamListener, () -> streamHandler); - // Create 2 local (odd) streams: 1, 3 and 1 remote (even) stream: 2. - final H2StreamHandler streamHandler1 = Mockito.mock(H2StreamHandler.class); - final H2StreamHandler streamHandler2 = Mockito.mock(H2StreamHandler.class); - final H2StreamHandler streamHandler3 = Mockito.mock(H2StreamHandler.class); - mux.createStream(mux.createChannel(1), streamHandler1); - mux.createStream(mux.createChannel(2), streamHandler2); - mux.createStream(mux.createChannel(3), streamHandler3); + final H2StreamHandler stream1Handler = Mockito.mock(H2StreamHandler.class); + final H2StreamHandler stream3Handler = Mockito.mock(H2StreamHandler.class); + final H2StreamHandler stream5Handler = Mockito.mock(H2StreamHandler.class); + mux.createStream(mux.createChannel(1), stream1Handler); + mux.createStream(mux.createChannel(3), stream3Handler); + mux.createStream(mux.createChannel(5), stream5Handler); - // GOAWAY last-stream-id = 1: local stream 3 was not processed and must fail. - // Last-stream-id does not apply to remote streams, so stream 2 must not fail. final ByteBuffer goAwayPayload = ByteBuffer.allocate(8); - goAwayPayload.putInt(1); // last-stream-id = 1 + goAwayPayload.putInt(0x80000003); // reserved bit set, last-stream-id = 3 goAwayPayload.putInt(H2Error.NO_ERROR.getCode()); goAwayPayload.flip(); - final RawFrame goAway = new RawFrame(FrameType.GOAWAY.getValue(), 0, 0, goAwayPayload); Assertions.assertDoesNotThrow(() -> mux.onInput(ByteBuffer.wrap(encodeFrame(goAway)))); - Mockito.verify(streamHandler3, Mockito.times(1)).failed(exceptionCaptor.capture()); - Assertions.assertInstanceOf(RequestNotExecutedException.class, exceptionCaptor.getValue()); - Mockito.verify(streamHandler1, Mockito.never()).failed(ArgumentMatchers.any()); - Mockito.verify(streamHandler2, Mockito.never()).failed(ArgumentMatchers.any()); + Mockito.verify(stream1Handler, Mockito.never()).failed(ArgumentMatchers.any(Exception.class)); + Mockito.verify(stream3Handler, Mockito.never()).failed(ArgumentMatchers.any(Exception.class)); + Mockito.verify(stream5Handler).failed(exceptionCaptor.capture()); + Assertions.assertInstanceOf( + org.apache.hc.core5.http.RequestNotExecutedException.class, + exceptionCaptor.getValue()); } @Test @@ -2531,6 +2366,102 @@ void testRemoteHeaderTableSizeChangesRemainLocallyBounded() throws Exception { } } + + @Test + void testUnsignedHeaderTableSizeSettingAccepted() throws Exception { + final AbstractH2StreamMultiplexer mux = new H2StreamMultiplexerImpl( + protocolIOSession, + FRAME_FACTORY, + StreamIdGenerator.ODD, + httpProcessor, + CharCodingConfig.DEFAULT, + H2Config.custom().build(), + h2StreamListener, + () -> streamHandler); + try { + final ByteBuffer payload = ByteBuffer.allocate(6); + payload.putShort((short) H2Param.HEADER_TABLE_SIZE.getCode()); + payload.putInt(-1); // 0xffffffff + payload.flip(); + + final RawFrame settingsFrame = + new RawFrame(FrameType.SETTINGS.getValue(), 0, 0, payload); + + Assertions.assertDoesNotThrow( + () -> mux.onInput(ByteBuffer.wrap(encodeFrame(settingsFrame)))); + Assertions.assertEquals(-1, getRemoteConfig(mux).getHeaderTableSize()); + Assertions.assertEquals( + H2Config.INIT.getHeaderTableSize(), + getHPackEncoder(mux).getMaxTableSize()); + } finally { + mux.close(); + } + } + + @Test + void testUnsignedMaxConcurrentStreamsSettingAccepted() throws Exception { + final AbstractH2StreamMultiplexer mux = new H2StreamMultiplexerImpl( + protocolIOSession, + FRAME_FACTORY, + StreamIdGenerator.ODD, + httpProcessor, + CharCodingConfig.DEFAULT, + H2Config.custom().build(), + h2StreamListener, + () -> streamHandler); + try { + final ByteBuffer payload = ByteBuffer.allocate(6); + payload.putShort((short) H2Param.MAX_CONCURRENT_STREAMS.getCode()); + payload.putInt(Integer.MIN_VALUE); // 0x80000000 + payload.flip(); + + final RawFrame settingsFrame = + new RawFrame(FrameType.SETTINGS.getValue(), 0, 0, payload); + + Assertions.assertDoesNotThrow( + () -> mux.onInput(ByteBuffer.wrap(encodeFrame(settingsFrame)))); + Assertions.assertEquals( + Integer.MIN_VALUE, + getRemoteConfig(mux).getMaxConcurrentStreams()); + } finally { + mux.close(); + } + } + + @Test + void testUnsignedMaxHeaderListSizeSettingAccepted() throws Exception { + final AbstractH2StreamMultiplexer mux = new H2StreamMultiplexerImpl( + protocolIOSession, + FRAME_FACTORY, + StreamIdGenerator.ODD, + httpProcessor, + CharCodingConfig.DEFAULT, + H2Config.custom().build(), + h2StreamListener, + () -> streamHandler); + try { + final ByteBuffer payload = ByteBuffer.allocate(6); + payload.putShort((short) H2Param.MAX_HEADER_LIST_SIZE.getCode()); + payload.putInt(-1); // 0xffffffff + payload.flip(); + + final RawFrame settingsFrame = + new RawFrame(FrameType.SETTINGS.getValue(), 0, 0, payload); + + Assertions.assertDoesNotThrow( + () -> mux.onInput(ByteBuffer.wrap(encodeFrame(settingsFrame)))); + Assertions.assertEquals(-1, getRemoteConfig(mux).getMaxHeaderListSize()); + } finally { + mux.close(); + } + } + + private static H2Config getRemoteConfig(final AbstractH2StreamMultiplexer multiplexer) throws Exception { + final Field field = AbstractH2StreamMultiplexer.class.getDeclaredField("remoteConfig"); + field.setAccessible(true); + return (H2Config) field.get(multiplexer); + } + private static HPackEncoder getHPackEncoder(final AbstractH2StreamMultiplexer multiplexer) throws Exception { final Field field = AbstractH2StreamMultiplexer.class.getDeclaredField("hPackEncoder"); field.setAccessible(true);