diff --git a/httpcore5-h2/src/main/java/org/apache/hc/core5/http2/frame/FramePrinter.java b/httpcore5-h2/src/main/java/org/apache/hc/core5/http2/frame/FramePrinter.java index 9466dce5e..bf2497f7f 100644 --- a/httpcore5-h2/src/main/java/org/apache/hc/core5/http2/frame/FramePrinter.java +++ b/httpcore5-h2/src/main/java/org/apache/hc/core5/http2/frame/FramePrinter.java @@ -134,7 +134,7 @@ public void printPayload(final RawFrame frame, final Appendable appendable) thro break; case GOAWAY: if (buf.remaining() >= 8) { - final int lastStream = buf.getInt(); + final int lastStream = buf.getInt() & 0x7fffffff; appendable.append("Last stream ").append(Integer.toString(lastStream)).append("\r\n"); appendable.append("Code "); final int code2 = buf.getInt(); @@ -155,7 +155,7 @@ public void printPayload(final RawFrame frame, final Appendable appendable) thro break; case WINDOW_UPDATE: if (buf.remaining() == 4) { - final int increment = buf.getInt(); + final int increment = buf.getInt() & 0x7fffffff; appendable.append("Increment ").append(Integer.toString(increment)).append("\r\n"); } else { appendable.append("Invalid\r\n"); 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 b0e1abb80..41dad3bae 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 @@ -464,7 +464,7 @@ public final void onInput(final ByteBuffer src) throws HttpException, IOExceptio final RawFrame frame = inputBuffer.read(src, ioSession); if (frame != null) { if (streamListener != null) { - streamListener.onFrameInput(this, frame.getStreamId(), frame); + streamListener.onFrameInput(this, frame.getStreamId() & 0x7fffffff, frame); } consumeFrame(frame); } else { @@ -770,7 +770,8 @@ public final void onException(final Exception cause) { private void consumeFrame(final RawFrame frame) throws HttpException, IOException { updateLastActivity(); final FrameType frameType = FrameType.valueOf(frame.getType()); - final int streamId = frame.getStreamId(); + final int streamId = frame.getStreamId() & 0x7fffffff; + if (continuation != null && frameType != FrameType.CONTINUATION) { throw new H2ConnectionException(H2Error.PROTOCOL_ERROR, "CONTINUATION frame expected"); } @@ -875,8 +876,8 @@ private void consumeFrame(final RawFrame frame) throws HttpException, IOExceptio if (payload == null || payload.remaining() != 4) { throw new H2ConnectionException(H2Error.FRAME_SIZE_ERROR, "Invalid WINDOW_UPDATE frame payload"); } - final int delta = payload.getInt(); - if (delta <= 0) { + final int delta = payload.getInt() & 0x7fffffff; + if (delta == 0) { throw new H2ConnectionException(H2Error.PROTOCOL_ERROR, "Invalid WINDOW_UPDATE delta"); } if (streamId == 0) { @@ -946,7 +947,6 @@ private void consumeFrame(final RawFrame frame) throws HttpException, IOExceptio throw new H2ConnectionException(H2Error.PROTOCOL_ERROR, "Illegal stream id"); } if (frame.isFlagSet(FrameFlag.ACK)) { - // RFC 9113, Section 6.5: SETTINGS with ACK set MUST have an empty payload. final ByteBuffer payload = frame.getPayload(); if (payload != null && payload.hasRemaining()) { throw new H2ConnectionException(H2Error.FRAME_SIZE_ERROR, "Invalid SETTINGS ACK payload"); @@ -997,7 +997,7 @@ private void consumeFrame(final RawFrame frame) throws HttpException, IOExceptio if (payload == null || payload.remaining() < 4) { throw new H2ConnectionException(H2Error.FRAME_SIZE_ERROR, "Invalid PUSH_PROMISE payload"); } - final int promisedStreamId = payload.getInt(); + final int promisedStreamId = payload.getInt() & 0x7fffffff; if (promisedStreamId == 0 || streams.isSameSide(promisedStreamId)) { throw new H2ConnectionException(H2Error.PROTOCOL_ERROR, "Illegal promised stream id: " + promisedStreamId); } @@ -1030,7 +1030,7 @@ private void consumeFrame(final RawFrame frame) throws HttpException, IOExceptio if (payload == null || payload.remaining() < 8) { throw new H2ConnectionException(H2Error.FRAME_SIZE_ERROR, "Invalid GOAWAY payload"); } - final int processedLocalStreamId = payload.getInt(); + final int processedLocalStreamId = payload.getInt() & 0x7fffffff; final int errorCode = payload.getInt(); goAwayReceived = true; if (errorCode == H2Error.NO_ERROR.getCode()) { @@ -1182,7 +1182,7 @@ private void consumeContinuationFrame(final RawFrame frame, final H2Stream strea if (stream.isRemoteClosed()) { throw new H2StreamResetException(H2Error.STREAM_CLOSED, "Stream already closed"); } - final int streamId = frame.getStreamId(); + final int streamId = frame.getStreamId() & 0x7fffffff; final ByteBuffer payload = frame.getPayload(); continuation.copyPayload(payload); if (frame.isFlagSet(FrameFlag.END_HEADERS)) { @@ -1227,10 +1227,14 @@ private void consumeSettingsFrame(final ByteBuffer payload) throws IOException { 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)); + } try { configBuilder.setInitialWindowSize(value); } catch (final IllegalArgumentException ex) { - throw new H2ConnectionException(H2Error.PROTOCOL_ERROR, ex.getMessage()); + throw new H2ConnectionException(H2Error.FLOW_CONTROL_ERROR, ex.getMessage()); } break; case MAX_FRAME_SIZE: 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 5590ededc..33b5978f3 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 @@ -385,6 +385,57 @@ void testZeroIncrement() throws Exception { Assertions.assertEquals(H2Error.PROTOCOL_ERROR, H2Error.getByCode(exception.getCode())); } + @Test + void testZeroIncrementConnectionWindowUpdate() 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(4); + payload.putInt(0); + payload.flip(); + final RawFrame incrementFrame = new RawFrame(FrameType.WINDOW_UPDATE.getValue(), 0, 0, payload); + + final H2ConnectionException exception = Assertions.assertThrows(H2ConnectionException.class, + () -> mux.onInput(ByteBuffer.wrap(encodeFrame(incrementFrame)))); + Assertions.assertEquals(H2Error.PROTOCOL_ERROR, H2Error.getByCode(exception.getCode())); + } finally { + mux.close(); + } + } + + @Test + void testInvalidInitialWindowSizeSettingIsFlowControlError() 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.INITIAL_WINDOW_SIZE.getCode()); + payload.putInt(-1); + payload.flip(); + final RawFrame settingsFrame = new RawFrame(FrameType.SETTINGS.getValue(), 0, 0, payload); + + final H2ConnectionException exception = Assertions.assertThrows(H2ConnectionException.class, + () -> mux.onInput(ByteBuffer.wrap(encodeFrame(settingsFrame)))); + Assertions.assertEquals(H2Error.FLOW_CONTROL_ERROR, H2Error.getByCode(exception.getCode())); + } finally { + mux.close(); + } + } + @Test void testIncrementOverflow() throws Exception { final H2Config h2Config = H2Config.custom() @@ -1605,5 +1656,158 @@ void testInputSettingsAckWithNonEmptyPayloadRejected() throws Exception { Assertions.assertEquals(H2Error.FRAME_SIZE_ERROR, H2Error.getByCode(ex.getCode())); } -} + @Test + void testWindowUpdateReservedBitIgnored() throws Exception { + final AbstractH2StreamMultiplexer mux = new H2StreamMultiplexerImpl( + protocolIOSession, + FRAME_FACTORY, + StreamIdGenerator.ODD, + httpProcessor, + CharCodingConfig.DEFAULT, + H2Config.custom().build(), + h2StreamListener, + () -> streamHandler); + + final ByteBuffer payload = ByteBuffer.allocate(4); + payload.putInt(0x80000001); + payload.flip(); + + final RawFrame windowUpdate = new RawFrame(FrameType.WINDOW_UPDATE.getValue(), 0, 0, payload); + + Assertions.assertDoesNotThrow(() -> mux.onInput(ByteBuffer.wrap(encodeFrame(windowUpdate)))); + } + + @Test + void testPingReservedBitInStreamIdIgnored() throws Exception { + final AbstractH2StreamMultiplexer mux = new H2StreamMultiplexerImpl( + protocolIOSession, + FRAME_FACTORY, + StreamIdGenerator.ODD, + httpProcessor, + CharCodingConfig.DEFAULT, + H2Config.custom().build(), + h2StreamListener, + () -> streamHandler); + + final ByteBuffer payload = ByteBuffer.wrap(new byte[8]); + final RawFrame ping = new RawFrame(FrameType.PING.getValue(), 0, 0x80000000, payload); + + Assertions.assertDoesNotThrow(() -> mux.onInput(ByteBuffer.wrap(encodeFrame(ping)))); + } + + @Test + void testPushPromiseReservedBitInPromisedStreamIdIgnored() throws Exception { + final H2Config h2Config = H2Config.custom() + .setPushEnabled(true) + .build(); + + final AbstractH2StreamMultiplexer mux = new H2StreamMultiplexerImpl( + protocolIOSession, + FRAME_FACTORY, + StreamIdGenerator.ODD, + httpProcessor, + CharCodingConfig.DEFAULT, + h2Config, + h2StreamListener, + () -> streamHandler); + + final H2StreamChannel s1 = mux.createChannel(1); + mux.createStream(s1, streamHandler); + + final ByteArrayBuffer hbuf = new ByteArrayBuffer(256); + final HPackEncoder encoder = new HPackEncoder( + H2Config.INIT.getHeaderTableSize(), + CharCodingSupport.createEncoder(CharCodingConfig.DEFAULT)); + + final List
reqHeaders = Arrays.asList( + new BasicHeader(":method", "GET"), + new BasicHeader(":scheme", "https"), + new BasicHeader(":authority", "example.test"), + new BasicHeader(":path", "/pushed")); + + encoder.encodeHeaders(hbuf, reqHeaders, h2Config.isCompressionEnabled()); + + final ByteBuffer payload = ByteBuffer.allocate(4 + hbuf.length()); + payload.putInt(0x80000002); + payload.put(hbuf.array(), 0, hbuf.length()); + payload.flip(); + + final RawFrame pp = new RawFrame( + FrameType.PUSH_PROMISE.getValue(), + FrameFlag.END_HEADERS.getValue(), + 1, + payload); + + Assertions.assertDoesNotThrow(() -> mux.onInput(ByteBuffer.wrap(encodeFrame(pp)))); + + Mockito.verify(h2StreamListener).onHeaderInput( + ArgumentMatchers.same(mux), + ArgumentMatchers.eq(2), + ArgumentMatchers.anyList()); + + Mockito.verify(streamHandler).consumePromise(ArgumentMatchers.anyList()); + } + + @Test + void testGoAwayReservedBitInLastStreamIdAffectsStreamCulling() throws Exception { + final H2Config h2Config = H2Config.custom().build(); + + final AbstractH2StreamMultiplexer mux = new H2StreamMultiplexerImpl( + protocolIOSession, + FRAME_FACTORY, + StreamIdGenerator.ODD, // local=odd, remote=even + httpProcessor, + CharCodingConfig.DEFAULT, + h2Config, + h2StreamListener, + () -> streamHandler); + + // Create 3 remote (even) streams by feeding inbound HEADERS on 2,4,6. + final ByteArrayBuffer headerBuf = new ByteArrayBuffer(256); + final HPackEncoder encoder = new HPackEncoder( + H2Config.INIT.getHeaderTableSize(), + CharCodingSupport.createEncoder(CharCodingConfig.DEFAULT)); + + final List
headers = Arrays.asList( + new BasicHeader(":method", "GET"), + new BasicHeader(":scheme", "https"), + new BasicHeader(":path", "/"), + new BasicHeader(":authority", "example.test")); + encoder.encodeHeaders(headerBuf, headers, h2Config.isCompressionEnabled()); + + final RawFrame h2 = FRAME_FACTORY.createHeaders(2, + ByteBuffer.wrap(headerBuf.array(), 0, headerBuf.length()), + true, // END_HEADERS + false // END_STREAM + ); + final RawFrame h4 = FRAME_FACTORY.createHeaders(4, + ByteBuffer.wrap(headerBuf.array(), 0, headerBuf.length()), + true, + false + ); + final RawFrame h6 = FRAME_FACTORY.createHeaders(6, + ByteBuffer.wrap(headerBuf.array(), 0, headerBuf.length()), + true, + false + ); + + feedFrame(mux, h2); + feedFrame(mux, h4); + feedFrame(mux, h6); + + // GOAWAY last-stream-id = 4, but with reserved MSB set. + // Correct masking keeps streams <= 4 (2 and 4) and drops only stream 6. + 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(streamHandler, Mockito.times(1)).failed(exceptionCaptor.capture()); + Assertions.assertInstanceOf(org.apache.hc.core5.http.RequestNotExecutedException.class, exceptionCaptor.getValue()); + } +} \ No newline at end of file