(tomcat) branch 10.1.x updated: Minor fixes from code review
[email protected] Sun, 02 Aug 2026 21:34:04 +0000
| Newsgroups | gmane.comp.jakarta.tomcat.devel |
|---|---|
| Message-ID | <178570644406.3728358.16597052319420162319@gitbox3-he-fi.apache.org> |
This is an automated email from the ASF dual-hosted git repository.
rmaucher pushed a commit to branch 10.1.x
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/10.1.x by this push:
new 40ed67ef85 Minor fixes from code review
40ed67ef85 is described below
commit 40ed67ef8544cfbbd857cef935bcb781391d89f5
Author: remm <[email protected]>
AuthorDate: Sun Aug 2 23:31:04 2026 +0200
Minor fixes from code review
Set the group info on the UpgradeInfo (rather than the other way
around).
Single byte write in gzip output filter seemed suspicious (unused).
Double count update in upgrade IS readLine.
Detect some errors in AJP cleanly.
Reading the colon in the chunk header should likely throw the bad
exception if it goes over the extension size.
Changes reviewed using OpenCode and the testsuite.
---
java/org/apache/coyote/AbstractProcessor.java | 4 ++--
java/org/apache/coyote/ActionCode.java | 2 +-
java/org/apache/coyote/CompressionConfig.java | 4 ++--
java/org/apache/coyote/ContinueResponseTiming.java | 3 +++
.../org/apache/coyote/ajp/AbstractAjpProtocol.java | 4 ++--
java/org/apache/coyote/ajp/AjpMessage.java | 14 ++++++------
java/org/apache/coyote/ajp/AjpProcessor.java | 25 +++++++++++++++-------
java/org/apache/coyote/ajp/Constants.java | 14 ++++++++----
java/org/apache/coyote/ajp/LocalStrings.properties | 2 ++
.../coyote/http11/AbstractHttp11Protocol.java | 2 ++
.../coyote/http11/filters/ChunkedInputFilter.java | 4 ++--
.../coyote/http11/filters/GzipOutputFilter.java | 3 ++-
.../coyote/http11/upgrade/UpgradeGroupInfo.java | 5 +++--
.../http11/upgrade/UpgradeProcessorExternal.java | 2 +-
.../http11/upgrade/UpgradeServletInputStream.java | 3 +--
.../http11/upgrade/UpgradeServletOutputStream.java | 1 -
.../apache/tomcat/util/http/parser/HttpParser.java | 4 ++--
17 files changed, 60 insertions(+), 36 deletions(-)
diff --git a/java/org/apache/coyote/AbstractProcessor.java b/java/org/apache/coyote/AbstractProcessor.java
index 4003bf2314..7a20e10630 100644
--- a/java/org/apache/coyote/AbstractProcessor.java
+++ b/java/org/apache/coyote/AbstractProcessor.java
@@ -124,8 +124,8 @@ public abstract class AbstractProcessor extends AbstractProcessorLight implement
if (getLog().isDebugEnabled()) {
getLog().debug(sm.getString("abstractProcessor.setErrorState", errorState), t);
}
- // Use the return value to avoid processing more than one async error
- // in a single async cycle.
+ // Mark the response as errored. Async error deduplication is
+ // handled by asyncStateMachine.asyncError() below.
response.setError();
boolean blockIo = this.errorState.isIoAllowed() && !errorState.isIoAllowed();
this.errorState = this.errorState.getMostSevere(errorState);
diff --git a/java/org/apache/coyote/ActionCode.java b/java/org/apache/coyote/ActionCode.java
index 0a9aeb8a99..b5609f3e94 100644
--- a/java/org/apache/coyote/ActionCode.java
+++ b/java/org/apache/coyote/ActionCode.java
@@ -111,7 +111,7 @@ public enum ActionCode {
REQ_LOCAL_ADDR_ATTRIBUTE,
/**
- * Callback for lazy evaluation - local address.
+ * Callback for lazy evaluation - local name (reverse DNS hostname).
*/
REQ_LOCAL_NAME_ATTRIBUTE,
diff --git a/java/org/apache/coyote/CompressionConfig.java b/java/org/apache/coyote/CompressionConfig.java
index bd283e6a7c..4ed3482210 100644
--- a/java/org/apache/coyote/CompressionConfig.java
+++ b/java/org/apache/coyote/CompressionConfig.java
@@ -38,8 +38,8 @@ import org.apache.tomcat.util.http.parser.TokenList;
import org.apache.tomcat.util.res.StringManager;
/**
- * Configuration for HTTP response compression settings.
- */
+ * Configuration for HTTP response compression settings.
+ */
public class CompressionConfig {
/**
diff --git a/java/org/apache/coyote/ContinueResponseTiming.java b/java/org/apache/coyote/ContinueResponseTiming.java
index 56aa3ef60b..1b1ac551ba 100644
--- a/java/org/apache/coyote/ContinueResponseTiming.java
+++ b/java/org/apache/coyote/ContinueResponseTiming.java
@@ -60,6 +60,9 @@ public enum ContinueResponseTiming {
* Do this for two reasons: - Not all of the Enum values are intended to be used in configuration - the naming
* convention for Enum constants and configuration values - is not consistent
*/
+ if (value == null) {
+ throw new IllegalArgumentException(sm.getString("continueResponseTiming.invalid", value));
+ }
if (IMMEDIATELY.toString().equalsIgnoreCase(value)) {
return IMMEDIATELY;
} else if (ON_REQUEST_BODY_READ.toString().equalsIgnoreCase(value)) {
diff --git a/java/org/apache/coyote/ajp/AbstractAjpProtocol.java b/java/org/apache/coyote/ajp/AbstractAjpProtocol.java
index c930c4ccd9..625673305c 100644
--- a/java/org/apache/coyote/ajp/AbstractAjpProtocol.java
+++ b/java/org/apache/coyote/ajp/AbstractAjpProtocol.java
@@ -103,14 +103,14 @@ public abstract class AbstractAjpProtocol<S> extends AbstractProtocol<S> {
/**
* Gets whether AJP flush packets are used.
*
- * @return <code>true</code> if flush packets are used
+ * @return {@code true} if flush packets are used
*/
public boolean getAjpFlush() {
return ajpFlush;
}
/**
- * Configure whether to aend an AJP flush packet when flushing. A flush packet is a zero byte AJP13 SEND_BODY_CHUNK
+ * Configure whether to send an AJP flush packet when flushing. A flush packet is a zero byte AJP13 SEND_BODY_CHUNK
* packet. mod_jk and mod_proxy_ajp interpret this as a request to flush data to the client. AJP always does flush
* at the end of the response, so if it is not important, that the packets get streamed up to the client, do not use
* extra flush packets. For compatibility and to stay on the safe side, flush packets are enabled by default.
diff --git a/java/org/apache/coyote/ajp/AjpMessage.java b/java/org/apache/coyote/ajp/AjpMessage.java
index 163d410c13..ff92e1de6f 100644
--- a/java/org/apache/coyote/ajp/AjpMessage.java
+++ b/java/org/apache/coyote/ajp/AjpMessage.java
@@ -240,8 +240,9 @@ public class AjpMessage {
/**
- * Read an integer from packet, and advance the read position past it. Integers are encoded as two unsigned bytes
- * with the high-order byte first, and, as far as I can tell, in little-endian order within each byte.
+ * Read an integer from packet, and advance the read position past it.
+ * Integers are encoded as two unsigned bytes in big-endian order
+ * (high-order byte first).
*
* @return The integer value read from the message
*/
@@ -298,7 +299,7 @@ public class AjpMessage {
private void doGetBytes(MessageBytes mb, boolean terminated) {
int length = getInt();
- if (length == 0xFFFF || length == -1) {
+ if (length == 0xFFFF) {
mb.recycle();
return;
}
@@ -317,10 +318,11 @@ public class AjpMessage {
/**
- * Read a 32 bits integer from packet, and advance the read position past it. Integers are encoded as four unsigned
- * bytes with the high-order byte first, and, as far as I can tell, in little-endian order within each byte.
+ * Read a 32-bit integer from packet, and advance the read position past it.
+ * The integer is encoded as four unsigned bytes in big-endian order
+ * (high-order byte first).
*
- * @return The long value read from the message
+ * @return The integer value read from the message
*/
public int getLongInt() {
int b1 = buf[pos++] & 0xFF; // No swap, Java order
diff --git a/java/org/apache/coyote/ajp/AjpProcessor.java b/java/org/apache/coyote/ajp/AjpProcessor.java
index 553880eb93..4b6fc30b40 100644
--- a/java/org/apache/coyote/ajp/AjpProcessor.java
+++ b/java/org/apache/coyote/ajp/AjpProcessor.java
@@ -647,6 +647,9 @@ public class AjpProcessor extends AbstractProcessor {
byte methodCode = requestHeaderMessage.getByte();
if (methodCode != Constants.SC_M_JK_STORED) {
String methodName = Constants.getMethodForCode(methodCode - 1);
+ if (methodName == null) {
+ throw new IllegalArgumentException(sm.getString("ajpprocessor.request.invalidMethod", String.valueOf(methodCode)));
+ }
request.setMethod(methodName);
}
@@ -689,6 +692,9 @@ public class AjpProcessor extends AbstractProcessor {
if (0xA000 == isc) {
requestHeaderMessage.getInt(); // To advance the read position
hName = Constants.getHeaderForCode(hId - 1);
+ if (hName == null) {
+ throw new IllegalArgumentException(sm.getString("ajpprocessor.request.invalidHeader", String.valueOf(hId)));
+ }
vMB = headers.addValue(hName);
} else {
// reset hId -- if the header currently being read
@@ -706,14 +712,18 @@ public class AjpProcessor extends AbstractProcessor {
requestHeaderMessage.getBytes(vMB);
if (hId == Constants.SC_REQ_CONTENT_LENGTH || (hId == -1 && tmpMB.equalsIgnoreCase("Content-Length"))) {
- long cl = vMB.getLong();
- if (contentLengthSet) {
+ try {
+ long cl = vMB.getLong();
+ if (contentLengthSet) {
+ response.setStatus(HttpServletResponse.SC_BAD_REQUEST);
+ setErrorState(ErrorState.CLOSE_CLEAN, null);
+ } else {
+ contentLengthSet = true;
+ request.setContentLength(cl);
+ }
+ } catch (NumberFormatException e) {
response.setStatus(HttpServletResponse.SC_BAD_REQUEST);
- setErrorState(ErrorState.CLOSE_CLEAN, null);
- } else {
- contentLengthSet = true;
- // Set the content-length header for the request
- request.setContentLength(cl);
+ setErrorState(ErrorState.CLOSE_CLEAN, e);
}
} else if (hId == Constants.SC_REQ_CONTENT_TYPE || (hId == -1 && tmpMB.equalsIgnoreCase("Content-Type"))) {
// just read the content-type header, so set it
@@ -1013,7 +1023,6 @@ public class AjpProcessor extends AbstractProcessor {
protected final void flush() throws IOException {
// Calling code should ensure that there is no data in the buffers for
// non-blocking writes.
- // TODO Validate the assertion above
if (!responseFinished) {
if (protocol.getAjpFlush()) {
// Send the flush message
diff --git a/java/org/apache/coyote/ajp/Constants.java b/java/org/apache/coyote/ajp/Constants.java
index eb757940ad..1ba3f13eac 100644
--- a/java/org/apache/coyote/ajp/Constants.java
+++ b/java/org/apache/coyote/ajp/Constants.java
@@ -47,12 +47,12 @@ public final class Constants {
/**
* AJP13 shutdown message prefix code.
*/
- public static final byte JK_AJP13_SHUTDOWN = 7; // XXX Unused
+ public static final byte JK_AJP13_SHUTDOWN = 7; // Unused
/**
* AJP13 ping request message prefix code.
*/
- public static final byte JK_AJP13_PING_REQUEST = 8; // XXX Unused
+ public static final byte JK_AJP13_PING_REQUEST = 8; // Unused
/**
* AJP13 cping request message prefix code.
@@ -153,12 +153,12 @@ public final class Constants {
/**
* Attribute code for context.
*/
- public static final byte SC_A_CONTEXT = 1; // XXX Unused
+ public static final byte SC_A_CONTEXT = 1; // Unused
/**
* Attribute code for servlet path.
*/
- public static final byte SC_A_SERVLET_PATH = 2; // XXX Unused
+ public static final byte SC_A_SERVLET_PATH = 2; // Unused
/**
* Attribute code for remote user.
@@ -283,6 +283,9 @@ public final class Constants {
* @return the string value of the method
*/
public static String getMethodForCode(final int code) {
+ if (code < 0 || code >= methodTransArray.length) {
+ return null;
+ }
return methodTransArray[code];
}
@@ -376,6 +379,9 @@ public final class Constants {
* @return the string value of the header name
*/
public static String getHeaderForCode(final int code) {
+ if (code < 0 || code >= headerTransArray.length) {
+ return null;
+ }
return headerTransArray[code];
}
diff --git a/java/org/apache/coyote/ajp/LocalStrings.properties b/java/org/apache/coyote/ajp/LocalStrings.properties
index f9de35dd4b..2dc83e01ac 100644
--- a/java/org/apache/coyote/ajp/LocalStrings.properties
+++ b/java/org/apache/coyote/ajp/LocalStrings.properties
@@ -28,6 +28,8 @@ ajpprocessor.header.error=Header message parsing failed
ajpprocessor.header.tooLong=Header message of length [{0}] received but the packetSize is only [{1}]
ajpprocessor.pongFail=Pong message failed
ajpprocessor.readtimeout=Timeout attempting to read data from the socket
+ajpprocessor.request.invalidHeader=Header code [{0}] was invalid
+ajpprocessor.request.invalidMethod=Method code [{0}] was invalid
ajpprocessor.request.prepare=Error preparing request
ajpprocessor.request.process=Error processing request
ajpprocessor.response.invalidHeader=The HTTP response header [{0}] with value [{1}] has been removed from the response because it is invalid
diff --git a/java/org/apache/coyote/http11/AbstractHttp11Protocol.java b/java/org/apache/coyote/http11/AbstractHttp11Protocol.java
index 7d85ef6514..b9f5d0cbbf 100644
--- a/java/org/apache/coyote/http11/AbstractHttp11Protocol.java
+++ b/java/org/apache/coyote/http11/AbstractHttp11Protocol.java
@@ -788,6 +788,8 @@ public abstract class AbstractHttp11Protocol<S> extends AbstractProtocol<S> {
}
}
allowedTrailerHeaders.removeAll(toRemove);
+ } else {
+ allowedTrailerHeaders.clear();
}
}
diff --git a/java/org/apache/coyote/http11/filters/ChunkedInputFilter.java b/java/org/apache/coyote/http11/filters/ChunkedInputFilter.java
index eaa8fd3332..aa658830f8 100644
--- a/java/org/apache/coyote/http11/filters/ChunkedInputFilter.java
+++ b/java/org/apache/coyote/http11/filters/ChunkedInputFilter.java
@@ -40,7 +40,7 @@ import org.apache.tomcat.util.res.StringManager;
/**
* Chunked input filter. Parses chunked data according to <a href=
- * "http://www.w3.org/Protocols/rfc2616/rfc2616-sec3.html#sec3.6.1">http://www.w3.org/Protocols/rfc2616/rfc2616-sec3.html#sec3.6.1</a><br>
+ * "https://www.rfc-editor.org/info/rfc9112/#section-7.1">https://www.rfc-editor.org/info/rfc9112/#section-7.1</a><br>
*/
public class ChunkedInputFilter implements InputFilter, ApplicationBufferHandler, HeaderDataSource {
@@ -405,7 +405,7 @@ public class ChunkedInputFilter implements InputFilter, ApplicationBufferHandler
extensionState = State.PRE_NAME;
long extSize = extensionSize.incrementAndGet();
if (maxExtensionSize > -1 && extSize > maxExtensionSize) {
- return false;
+ throwBadRequestException(sm.getString("chunkedInputFilter.maxExtension"));
}
} else {
int charValue = HexUtils.getDec(chr);
diff --git a/java/org/apache/coyote/http11/filters/GzipOutputFilter.java b/java/org/apache/coyote/http11/filters/GzipOutputFilter.java
index 3164c04cc0..24132e05c0 100644
--- a/java/org/apache/coyote/http11/filters/GzipOutputFilter.java
+++ b/java/org/apache/coyote/http11/filters/GzipOutputFilter.java
@@ -167,7 +167,8 @@ public class GzipOutputFilter implements OutputFilter {
public void write(int b) throws IOException {
// Shouldn't get used for good performance, but is needed for
// compatibility with Sun JDK 1.4.0
- outputChunk.put(0, (byte) (b & 0xff));
+ outputChunk.clear();
+ outputChunk.put((byte) (b & 0xff));
buffer.doWrite(outputChunk);
}
diff --git a/java/org/apache/coyote/http11/upgrade/UpgradeGroupInfo.java b/java/org/apache/coyote/http11/upgrade/UpgradeGroupInfo.java
index 5f216de39c..03999b9eda 100644
--- a/java/org/apache/coyote/http11/upgrade/UpgradeGroupInfo.java
+++ b/java/org/apache/coyote/http11/upgrade/UpgradeGroupInfo.java
@@ -102,11 +102,12 @@ public class UpgradeGroupInfo extends BaseModelMBean {
/**
* Resets the bytes received counter.
*
- * @param bytesReceived the new value (ignored, used for MBean reset)
+ * @param bytesReceived the value to restore (0 to reset completely,
+ * non-zero values are added to the dead counter)
*/
public void setBytesReceived(long bytesReceived) {
deadBytesReceived.reset();
- if (bytesReceived != 0) {
+ if (bytesReceived > 0) {
deadBytesReceived.add(bytesReceived);
}
for (UpgradeInfo ui : upgradeInfos) {
diff --git a/java/org/apache/coyote/http11/upgrade/UpgradeProcessorExternal.java b/java/org/apache/coyote/http11/upgrade/UpgradeProcessorExternal.java
index 2bd9e14b3e..500d7eb2cc 100644
--- a/java/org/apache/coyote/http11/upgrade/UpgradeProcessorExternal.java
+++ b/java/org/apache/coyote/http11/upgrade/UpgradeProcessorExternal.java
@@ -65,7 +65,7 @@ public class UpgradeProcessorExternal extends UpgradeProcessorBase {
super(upgradeToken);
this.upgradeInfo = new UpgradeInfo();
if (upgradeGroupInfo != null) {
- upgradeGroupInfo.addUpgradeInfo(upgradeInfo);
+ upgradeInfo.setGroupInfo(upgradeGroupInfo);
}
this.upgradeServletInputStream = new UpgradeServletInputStream(this, wrapper, upgradeInfo);
this.upgradeServletOutputStream = new UpgradeServletOutputStream(this, wrapper, upgradeInfo);
diff --git a/java/org/apache/coyote/http11/upgrade/UpgradeServletInputStream.java b/java/org/apache/coyote/http11/upgrade/UpgradeServletInputStream.java
index 742968e443..9483ae4559 100644
--- a/java/org/apache/coyote/http11/upgrade/UpgradeServletInputStream.java
+++ b/java/org/apache/coyote/http11/upgrade/UpgradeServletInputStream.java
@@ -175,7 +175,6 @@ public class UpgradeServletInputStream extends ServletInputStream {
}
if (count > 0) {
- upgradeInfo.addBytesReceived(count);
return count;
} else {
return -1;
@@ -233,7 +232,7 @@ public class UpgradeServletInputStream extends ServletInputStream {
throw ioe;
}
if (result == 0) {
- return -1;
+ throw new IllegalStateException(sm.getString("upgrade.sis.read.ise"));
} else if (result == -1) {
eof = true;
return -1;
diff --git a/java/org/apache/coyote/http11/upgrade/UpgradeServletOutputStream.java b/java/org/apache/coyote/http11/upgrade/UpgradeServletOutputStream.java
index 07858a5b08..ed16ed44ec 100644
--- a/java/org/apache/coyote/http11/upgrade/UpgradeServletOutputStream.java
+++ b/java/org/apache/coyote/http11/upgrade/UpgradeServletOutputStream.java
@@ -152,7 +152,6 @@ public class UpgradeServletOutputStream extends ServletOutputStream {
// Container is responsible for first call to onWritePossible().
synchronized (registeredLock) {
registered = true;
- // Container is responsible for first call to onDataAvailable().
Request request = processor.getRequest();
if (request != null && request.isRequestThread()) {
processor.addDispatch(DispatchType.NON_BLOCKING_WRITE);
diff --git a/java/org/apache/tomcat/util/http/parser/HttpParser.java b/java/org/apache/tomcat/util/http/parser/HttpParser.java
index 8f47c8c47c..c82fa9e403 100644
--- a/java/org/apache/tomcat/util/http/parser/HttpParser.java
+++ b/java/org/apache/tomcat/util/http/parser/HttpParser.java
@@ -201,7 +201,7 @@ public class HttpParser {
* @return {@code true} if the character is valid for an absolute path
*/
public boolean isAbsolutePathRelaxed(int c) {
- // Fast for valid user info characters, slower for some incorrect
+ // Fast for valid absolute path characters, slower for some incorrect
// ones
try {
return IS_ABSOLUTEPATH_RELAXED[c];
@@ -218,7 +218,7 @@ public class HttpParser {
* @return {@code true} if the character is valid for a query string
*/
public boolean isQueryRelaxed(int c) {
- // Fast for valid user info characters, slower for some incorrect
+ // Fast for valid query string characters, slower for some incorrect
// ones
try {
return IS_QUERY_RELAXED[c];