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
The following commit(s) were added to refs/heads/9.0.x by this push:
new 8859852728 Correct a narrow race condition when processing HTTP/2
headers
8859852728 is described below
commit 885985272887e9a5d47245cec240a5d1d0e39b5b
Author: Mark Thomas <[email protected]>
AuthorDate: Thu Sep 10 14:33:28 2026 +0100
Correct a narrow race condition when processing HTTP/2 headers
---
java/org/apache/coyote/http2/HpackDecoder.java | 17 ++++-
java/org/apache/coyote/http2/Http2Parser.java | 38 +++++++----
.../apache/coyote/http2/Http2UpgradeHandler.java | 73 +++++++++++++---------
test/org/apache/coyote/http2/Http2TestBase.java | 4 +-
webapps/docs/changelog.xml | 3 +
5 files changed, 91 insertions(+), 44 deletions(-)
diff --git a/java/org/apache/coyote/http2/HpackDecoder.java
b/java/org/apache/coyote/http2/HpackDecoder.java
index bf8b34d77a..2e7071fc00 100644
--- a/java/org/apache/coyote/http2/HpackDecoder.java
+++ b/java/org/apache/coyote/http2/HpackDecoder.java
@@ -18,6 +18,7 @@ package org.apache.coyote.http2;
import java.nio.ByteBuffer;
import java.util.concurrent.atomic.AtomicReference;
+import java.util.concurrent.locks.ReentrantLock;
import org.apache.juli.logging.Log;
import org.apache.juli.logging.LogFactory;
@@ -37,6 +38,10 @@ public class HpackDecoder {
* The object that receives the headers that are emitted from this decoder
*/
private final AtomicReference<HeaderEmitter> headerEmitter = new
AtomicReference<>();
+ /**
+ * The lock used to avoid a race between replacing the stream and emitting
a header.
+ */
+ private final ReentrantLock streamEmitterLock = new ReentrantLock();
/**
* The header table
@@ -449,11 +454,21 @@ public class HpackDecoder {
if (log.isTraceEnabled()) {
log.trace(sm.getString("hpackdecoder.emitHeader", name,
value));
}
- headerEmitter.get().emitHeader(name, value);
+ streamEmitterLock.lock();
+ try {
+ headerEmitter.get().emitHeader(name, value);
+ } finally {
+ streamEmitterLock.unlock();
+ }
}
}
+ ReentrantLock getStreamEmitterLock() {
+ return streamEmitterLock;
+ }
+
+
boolean isHeaderCountExceeded() {
if (maxHeaderCount < 0) {
return false;
diff --git a/java/org/apache/coyote/http2/Http2Parser.java
b/java/org/apache/coyote/http2/Http2Parser.java
index cacc2aea45..fcb424f240 100644
--- a/java/org/apache/coyote/http2/Http2Parser.java
+++ b/java/org/apache/coyote/http2/Http2Parser.java
@@ -27,7 +27,6 @@ import java.nio.charset.StandardCharsets;
import javax.servlet.http.WebConnection;
import org.apache.coyote.ProtocolException;
-import org.apache.coyote.http2.HpackDecoder.HeaderEmitter;
import org.apache.juli.logging.Log;
import org.apache.juli.logging.LogFactory;
import org.apache.tomcat.util.buf.ByteBufferUtils;
@@ -255,11 +254,7 @@ class Http2Parser {
headersEndStream = Flags.isEndOfStream(flags);
- try {
- hpackDecoder.setHeaderEmitter(output.headersStart(streamId,
headersEndStream));
- } catch (StreamException se) {
- hpackDecoder.setHeaderEmitter(new HeaderSink(se));
- }
+ output.headersStart(streamId, headersEndStream);
int padLength = 0;
boolean padding = Flags.hasPadding(flags);
@@ -312,7 +307,12 @@ class Http2Parser {
if (Flags.isEndOfHeaders(flags)) {
// Validate the headers once complete
- hpackDecoder.getHeaderEmitter().validateHeaders();
+ hpackDecoder.getStreamEmitterLock().lock();
+ try {
+ hpackDecoder.getHeaderEmitter().validateHeaders();
+ } finally {
+ hpackDecoder.getStreamEmitterLock().unlock();
+ }
onHeadersComplete(streamId);
} else {
headersCurrentStream = streamId;
@@ -482,8 +482,12 @@ class Http2Parser {
headersCurrentStream = -1;
// Validate the headers once complete
- hpackDecoder.getHeaderEmitter().validateHeaders();
-
+ hpackDecoder.getStreamEmitterLock().lock();
+ try {
+ hpackDecoder.getHeaderEmitter().validateHeaders();
+ } finally {
+ hpackDecoder.getStreamEmitterLock().unlock();
+ }
onHeadersComplete(streamId);
}
}
@@ -580,14 +584,24 @@ class Http2Parser {
StreamException headerException = new StreamException(
sm.getString("http2Parser.headerLimitCount",
connectionId, Integer.valueOf(streamId)),
Http2Error.ENHANCE_YOUR_CALM, streamId);
-
hpackDecoder.getHeaderEmitter().setHeaderException(headerException);
+ hpackDecoder.getStreamEmitterLock().lock();
+ try {
+
hpackDecoder.getHeaderEmitter().setHeaderException(headerException);
+ } finally {
+ hpackDecoder.getStreamEmitterLock().unlock();
+ }
}
if
(hpackDecoder.isHeaderSizeExceeded(headerReadBuffer.position())) {
StreamException headerException = new StreamException(
sm.getString("http2Parser.headerLimitSize",
connectionId, Integer.valueOf(streamId)),
Http2Error.ENHANCE_YOUR_CALM, streamId);
-
hpackDecoder.getHeaderEmitter().setHeaderException(headerException);
+ hpackDecoder.getStreamEmitterLock().lock();
+ try {
+
hpackDecoder.getHeaderEmitter().setHeaderException(headerException);
+ } finally {
+ hpackDecoder.getStreamEmitterLock().unlock();
+ }
}
if
(hpackDecoder.isHeaderSwallowSizeExceeded(headerReadBuffer.position())) {
@@ -864,7 +878,7 @@ class Http2Parser {
throws ConnectionException, IOException;
// Header frames
- HeaderEmitter headersStart(int streamId, boolean headersEndStream)
throws Http2Exception, IOException;
+ void headersStart(int streamId, boolean headersEndStream) throws
Http2Exception, IOException;
void headersContinue(int payloadSize, boolean endOfHeaders);
diff --git a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
index c6aa5fb38e..124bded377 100644
--- a/java/org/apache/coyote/http2/Http2UpgradeHandler.java
+++ b/java/org/apache/coyote/http2/Http2UpgradeHandler.java
@@ -41,7 +41,6 @@ import org.apache.coyote.Adapter;
import org.apache.coyote.ProtocolException;
import org.apache.coyote.Request;
import org.apache.coyote.http11.upgrade.InternalHttpUpgradeHandler;
-import org.apache.coyote.http2.HpackDecoder.HeaderEmitter;
import org.apache.coyote.http2.HpackEncoder.State;
import org.apache.coyote.http2.Http2Parser.Input;
import org.apache.coyote.http2.Http2Parser.Output;
@@ -1764,39 +1763,50 @@ class Http2UpgradeHandler extends AbstractStream
implements InternalHttpUpgradeH
@Override
- public HeaderEmitter headersStart(int streamId, boolean headersEndStream)
throws Http2Exception, IOException {
+ public void headersStart(int streamId, boolean headersEndStream) throws
Http2Exception, IOException {
- Stream stream = getStream(streamId, false);
- if (stream == null) {
- // New stream
+ getHpackDecoder().getStreamEmitterLock().lock();
+ try {
+ Stream stream = getStream(streamId, false);
+ if (stream == null) {
+ // New stream
- // Check the pause state before processing headers since the pause
state
- // determines if a new stream is created or if this stream is
ignored.
- checkPauseState();
+ // Check the pause state before processing headers since the
pause state
+ // determines if a new stream is created or if this stream is
ignored.
+ checkPauseState();
- if (connectionState.get().isNewStreamAllowed()) {
- if (streamId > maxProcessedStreamId) {
- stream = createRemoteStream(streamId);
- activeRemoteStreamCount.incrementAndGet();
- maxProcessedStreamId = streamId;
+ if (connectionState.get().isNewStreamAllowed()) {
+ if (streamId > maxProcessedStreamId) {
+ stream = createRemoteStream(streamId);
+ activeRemoteStreamCount.incrementAndGet();
+ maxProcessedStreamId = streamId;
+ } else {
+ // ID for new stream must always be greater than any
previous stream
+ throw new
ConnectionException(sm.getString("upgradeHandler.stream.old",
Integer.valueOf(streamId),
+ Integer.valueOf(maxProcessedStreamId)),
Http2Error.PROTOCOL_ERROR);
+ }
} else {
- // ID for new stream must always be greater than any
previous stream
- throw new
ConnectionException(sm.getString("upgradeHandler.stream.old",
Integer.valueOf(streamId),
- Integer.valueOf(maxProcessedStreamId)),
Http2Error.PROTOCOL_ERROR);
- }
- } else {
- if (log.isTraceEnabled()) {
- log.trace(sm.getString("upgradeHandler.noNewStreams",
connectionId, Integer.toString(streamId)));
+ if (log.isTraceEnabled()) {
+ log.trace(sm.getString("upgradeHandler.noNewStreams",
connectionId, Integer.toString(streamId)));
+ }
+ reduceOverheadCount(FrameType.HEADERS);
+ // Stateless so a static can be used to save on GC
+ getHpackDecoder().setHeaderEmitter(HEADER_SINK);
+ return;
}
- reduceOverheadCount(FrameType.HEADERS);
- // Stateless so a static can be used to save on GC
- return HEADER_SINK;
}
- }
- stream.checkState(FrameType.HEADERS);
- stream.receivedStartOfHeaders(headersEndStream);
- return stream;
+ try {
+ stream.checkState(FrameType.HEADERS);
+ } catch (StreamException se) {
+ getHpackDecoder().setHeaderEmitter(new HeaderSink(se));
+ return;
+ }
+ stream.receivedStartOfHeaders(headersEndStream);
+ getHpackDecoder().setHeaderEmitter(stream);
+ } finally {
+ getHpackDecoder().getStreamEmitterLock().unlock();
+ }
}
@@ -2065,8 +2075,13 @@ class Http2UpgradeHandler extends AbstractStream
implements InternalHttpUpgradeH
if (log.isTraceEnabled()) {
log.trace(sm.getString("upgradeHandler.replace.first",
getConnectionId(), original.getIdAsString()));
}
- streams.put(original.getIdentifier(), replacement);
- getHpackDecoder().compareAndSetHeaderEmitter((Stream) current,
HEADER_SINK);
+ getHpackDecoder().getStreamEmitterLock().lock();
+ try {
+ streams.put(original.getIdentifier(), replacement);
+ getHpackDecoder().compareAndSetHeaderEmitter((Stream) current,
HEADER_SINK);
+ } finally {
+ getHpackDecoder().getStreamEmitterLock().unlock();
+ }
int made;
original.windowAllocationLock.lock();
diff --git a/test/org/apache/coyote/http2/Http2TestBase.java
b/test/org/apache/coyote/http2/Http2TestBase.java
index fd8186fb9c..f75302bc66 100644
--- a/test/org/apache/coyote/http2/Http2TestBase.java
+++ b/test/org/apache/coyote/http2/Http2TestBase.java
@@ -1172,10 +1172,10 @@ public abstract class Http2TestBase extends
TomcatBaseTest {
@Override
- public HeaderEmitter headersStart(int streamId, boolean
headersEndStream) {
+ public void headersStart(int streamId, boolean headersEndStream) {
lastStreamId = Integer.toString(streamId);
trace.append(lastStreamId + "-HeadersStart\n");
- return this;
+ getHpackDecoder().setHeaderEmitter(this);
}
diff --git a/webapps/docs/changelog.xml b/webapps/docs/changelog.xml
index 97cca2b243..ac51ae2889 100644
--- a/webapps/docs/changelog.xml
+++ b/webapps/docs/changelog.xml
@@ -208,6 +208,9 @@
Additional checks (enabled by default) have also been added for the
directory where the Unix Domain Socket will be created.(markt)
</add>
+ <fix>
+ Correct a narrow race condition when processing HTTP/2 headers. (markt)
+ </fix>
</changelog>
</subsection>
<subsection name="Jasper">
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
