This is an automated email from the ASF dual-hosted git repository. markt-asf pushed a commit to branch 9.0.x in repository https://gitbox.apache.org/repos/asf/tomcat.git
commit 4368e74fe3da36183bbfaa8fe4d8dfd259ebe427 Author: Mark Thomas <[email protected]> AuthorDate: Wed Nov 11 14:23:34 2026 +0000 Fix BZ 70288. Possible request body truncation with HTTP/2 Based on a patch by Alberto Limón. Test case written by Claude / Sonnet 5.5 --- java/org/apache/coyote/http2/Stream.java | 16 ++- .../apache/coyote/http2/TestStreamInputBuffer.java | 124 +++++++++++++++++++++ webapps/docs/changelog.xml | 4 + 3 files changed, 143 insertions(+), 1 deletion(-) diff --git a/java/org/apache/coyote/http2/Stream.java b/java/org/apache/coyote/http2/Stream.java index e28f7b1aee..02d029fe06 100644 --- a/java/org/apache/coyote/http2/Stream.java +++ b/java/org/apache/coyote/http2/Stream.java @@ -1518,6 +1518,15 @@ class Stream extends AbstractNonZeroStream implements HeaderEmitter { } if (!isRequestBodyFullyRead()) { + /* + * Data is written to the buffer without holding readStateLock so it may have arrived since the + * check above. If it has, report it now rather than registering read interest which would trigger + * an unnecessary dispatch. Any data that arrives after this point will trigger a call to + * onDataAvailable() which is blocked on readStateLock until this method returns. + */ + if (available() > 0) { + return true; + } readInterest = true; long readTimeout = handler.getProtocol().getStreamReadTimeout(); if (readTimeout > 0) { @@ -1537,7 +1546,12 @@ class Stream extends AbstractNonZeroStream implements HeaderEmitter { final boolean isRequestBodyFullyRead() { readStateLock.lock(); try { - return (inBuffer == null || inBuffer.position() == 0) && isInputFinished(); + /* + * Check for end of input first. The producer buffers data before it records end of stream so, once + * input is finished, the buffer check is reliable. The reverse order allows an old "buffer empty" + * observation to be combined with a newer "input finished" observation. + */ + return isInputFinished() && (inBuffer == null || inBuffer.position() == 0); } finally { readStateLock.unlock(); } diff --git a/test/org/apache/coyote/http2/TestStreamInputBuffer.java b/test/org/apache/coyote/http2/TestStreamInputBuffer.java new file mode 100644 index 0000000000..38f430c1b4 --- /dev/null +++ b/test/org/apache/coyote/http2/TestStreamInputBuffer.java @@ -0,0 +1,124 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +package org.apache.coyote.http2; + +import java.nio.ByteBuffer; +import java.util.concurrent.atomic.AtomicReference; + +import jakarta.servlet.ServletConnection; + +import org.junit.Assert; +import org.junit.Test; + +import org.apache.tomcat.util.net.SocketWrapperBase; +import org.easymock.EasyMock; + +public class TestStreamInputBuffer { + + /* + * The buffer check and the end-of-input check in isRequestBodyFullyRead() must not combine an old "buffer empty" + * observation with a newer "input finished" observation. Otherwise a body that still has unread data can be + * reported as fully read. + */ + @Test + public void testRequestBodyFullyReadRace() throws Exception { + SocketWrapperBase<?> socket = EasyMock.createNiceMock(SocketWrapperBase.class); + ServletConnection connection = EasyMock.createNiceMock(ServletConnection.class); + EasyMock.expect(socket.getServletConnection("h2c", "")).andStubReturn(connection); + EasyMock.expect(connection.getConnectionId()).andStubReturn("0"); + EasyMock.replay(socket, connection); + + Http2UpgradeHandler handler = new Http2UpgradeHandler(new Http2Protocol(), null, null, socket); + + for (int i = 0; i < 20; i++) { + Stream stream = new Stream(Integer.valueOf(3), handler); + stream.state.receivedStartOfHeaders(); + stream.getCoyoteRequest().setContentLength(1); + ByteBuffer data = stream.getInputByteBuffer(true); + Assert.assertFalse(stream.getInputBuffer().isRequestBodyFullyRead()); + + AtomicReference<Boolean> result = new AtomicReference<>(); + Thread consumer = + new Thread(() -> result.set(Boolean.valueOf(stream.getInputBuffer().isRequestBodyFullyRead()))); + + // Hold the state monitor so that the consumer blocks part way through the check + synchronized (stream.state) { + consumer.start(); + long timeout = System.nanoTime() + 5_000_000_000L; + while (consumer.getState() != Thread.State.BLOCKED) { + Assert.assertTrue("Consumer did not block", System.nanoTime() < timeout); + Thread.sleep(1); + } + // Producer: one byte of body followed by end of stream + synchronized (data) { + stream.receivedData(1); + data.put((byte) 'A'); + stream.receivedEndOfStream(); + } + } + consumer.join(5000); + Assert.assertFalse(consumer.isAlive()); + + Assert.assertEquals(1, data.position()); + Assert.assertEquals("Body reported as fully read with unread data", Boolean.FALSE, result.get()); + Assert.assertFalse(stream.getInputBuffer().isRequestBodyFullyRead()); + } + } + + /* + * If data arrives between the initial available() check and the completion check in isReadyForRead(), the method + * should report that data is available rather than registering read interest (which would trigger an unnecessary + * dispatch). + */ + @Test + public void testIsReadyForReadDataArrivesDuringCheck() throws Exception { + SocketWrapperBase<?> socket = EasyMock.createNiceMock(SocketWrapperBase.class); + ServletConnection connection = EasyMock.createNiceMock(ServletConnection.class); + EasyMock.expect(socket.getServletConnection("h2c", "")).andStubReturn(connection); + EasyMock.expect(connection.getConnectionId()).andStubReturn("0"); + EasyMock.replay(socket, connection); + + Http2UpgradeHandler handler = new Http2UpgradeHandler(new Http2Protocol(), null, null, socket); + + Stream stream = new Stream(Integer.valueOf(3), handler); + stream.state.receivedStartOfHeaders(); + stream.getCoyoteRequest().setContentLength(1); + ByteBuffer data = stream.getInputByteBuffer(true); + + AtomicReference<Boolean> result = new AtomicReference<>(); + Thread consumer = new Thread(() -> result.set(Boolean.valueOf(stream.getInputBuffer().isReadyForRead()))); + + synchronized (stream.state) { + consumer.start(); + long timeout = System.nanoTime() + 5_000_000_000L; + while (consumer.getState() != Thread.State.BLOCKED) { + Assert.assertTrue("Consumer did not block", System.nanoTime() < timeout); + Thread.sleep(1); + } + synchronized (data) { + stream.receivedData(1); + data.put((byte) 'A'); + stream.receivedEndOfStream(); + } + } + consumer.join(5000); + Assert.assertFalse(consumer.isAlive()); + + Assert.assertEquals("Data arrived during the check so the stream should be ready for read", Boolean.TRUE, + result.get()); + } +} diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml index f13b1e4903..f4ebd0dc1e 100644 --- a/webapps/docs/changelog.xml +++ b/webapps/docs/changelog.xml @@ -114,6 +114,10 @@ <bug>70287</bug>: Restore the keep-alive timeout after a sendfile write. Patch provided by Artur Shellunts. (markt) </fix> + <fix> + <bug>70288</bug>: Fix bug that could cause request body truncation with + HTTP/2. Based on a patch by Alberto Limón. (markt) + </fix> </changelog> </subsection> <subsection name="Jasper"> --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
