You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomcat.apache.org by re...@apache.org on 2026/07/15 15:06:52 UTC
(tomcat) branch main updated: Code review fixes
This is an automated email from the ASF dual-hosted git repository.
rmaucher pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/tomcat.git
The following commit(s) were added to refs/heads/main by this push:
new 60008aaaf7 Code review fixes
60008aaaf7 is described below
commit 60008aaaf73a5e864f382f4667d576ea2d5b2f2a
Author: remm <re...@apache.org>
AuthorDate: Wed Jul 15 17:06:29 2026 +0200
Code review fixes
Some refinements for DOMWriter, co authored by OpenCode.
Similar in XMLWriter.
---
.../apache/catalina/servlets/DefaultServlet.java | 8 +++--
.../catalina/servlets/LocalStrings.properties | 1 +
java/org/apache/catalina/util/ContextName.java | 2 +-
.../catalina/util/CustomObjectInputStream.java | 4 +--
java/org/apache/catalina/util/DOMWriter.java | 37 +++++++++++++++++++---
.../org/apache/catalina/util/ErrorPageSupport.java | 12 +++----
.../org/apache/catalina/util/ExactRateLimiter.java | 2 +-
java/org/apache/catalina/util/IOTools.java | 2 +-
java/org/apache/catalina/util/Introspection.java | 5 +--
java/org/apache/catalina/util/NetMask.java | 23 +++++++++++---
java/org/apache/catalina/util/NetMaskSet.java | 2 +-
java/org/apache/catalina/util/RateLimiterBase.java | 2 +-
java/org/apache/catalina/util/ResourceSet.java | 26 ++++++++++++---
java/org/apache/catalina/util/ServerInfo.java | 2 +-
java/org/apache/catalina/util/SessionConfig.java | 2 +-
.../catalina/util/SessionIdGeneratorBase.java | 2 +-
java/org/apache/catalina/util/Strftime.java | 2 +-
.../apache/catalina/util/TimeBucketCounter.java | 2 +-
java/org/apache/catalina/util/XMLWriter.java | 14 ++++++--
19 files changed, 114 insertions(+), 36 deletions(-)
diff --git a/java/org/apache/catalina/servlets/DefaultServlet.java b/java/org/apache/catalina/servlets/DefaultServlet.java
index 0d8a1b1f8e..0642052c06 100644
--- a/java/org/apache/catalina/servlets/DefaultServlet.java
+++ b/java/org/apache/catalina/servlets/DefaultServlet.java
@@ -2128,8 +2128,12 @@ public class DefaultServlet extends HttpServlet {
log(sm.getString("defaultServlet.globalXSLTTooBig", f.getAbsolutePath()));
} else {
try (FileInputStream fis = new FileInputStream(f)) {
- byte[] b = new byte[(int) f.length()];
- IOTools.readFully(fis, b);
+ int xsltLength = (int) globalXsltFileSize;
+ byte[] b = new byte[xsltLength];
+ if (xsltLength != IOTools.readFully(fis, b)) {
+ log(sm.getString("defaultServlet.truncatedXSLT", f.getAbsolutePath()));
+ return null;
+ }
return new StreamSource(new ByteArrayInputStream(b));
}
}
diff --git a/java/org/apache/catalina/servlets/LocalStrings.properties b/java/org/apache/catalina/servlets/LocalStrings.properties
index 2628a1e8e2..896d0c0aa1 100644
--- a/java/org/apache/catalina/servlets/LocalStrings.properties
+++ b/java/org/apache/catalina/servlets/LocalStrings.properties
@@ -53,6 +53,7 @@ defaultServlet.resource.lastModified=Last Modified
defaultServlet.resource.name=Name
defaultServlet.resource.size=Size
defaultServlet.skipfail=Read failed because only [{0}] bytes were available but needed to skip [{1}] bytes to reach the start of the requested range
+defaultServlet.truncatedXSLT=The global XSLT file [{0}] was truncated
defaultServlet.unknownBomConfig=Unrecognised value of [{0}] provided for useBomIfPresent initialization parameter
defaultServlet.wrongByteCountForRange=An invalid amount [{0}] of bytes were received for range with length [{1}]
defaultServlet.xslError=XSL transformer error
diff --git a/java/org/apache/catalina/util/ContextName.java b/java/org/apache/catalina/util/ContextName.java
index 18c224c431..34d2e38f5b 100644
--- a/java/org/apache/catalina/util/ContextName.java
+++ b/java/org/apache/catalina/util/ContextName.java
@@ -100,7 +100,7 @@ public final class ContextName {
/**
* Construct an instance from a path and version.
*
- * @param path Context path to use
+ * @param path Context path to use, which must not contain a version marker
* @param version Context version to use
*/
public ContextName(String path, String version) {
diff --git a/java/org/apache/catalina/util/CustomObjectInputStream.java b/java/org/apache/catalina/util/CustomObjectInputStream.java
index 80f869b997..79b3de1ed6 100644
--- a/java/org/apache/catalina/util/CustomObjectInputStream.java
+++ b/java/org/apache/catalina/util/CustomObjectInputStream.java
@@ -73,8 +73,8 @@ public final class CustomObjectInputStream extends ObjectInputStream {
* @param log The logger to use to report any issues. It may only be null if the filterMode does
* not require logging
* @param allowedClassNamePattern The regular expression to use to filter deserialized classes. The fully qualified
- * class name must match this pattern for deserialization to be allowed if
- * filtering is enabled.
+ * class name must match this pattern for deserialization to be allowed.
+ * If null, filtering will be disabled.
* @param warnOnFailure Should any failures be logged?
*
* @exception IOException if an input/output error occurs
diff --git a/java/org/apache/catalina/util/DOMWriter.java b/java/org/apache/catalina/util/DOMWriter.java
index 6218dfb4e2..8f47594394 100644
--- a/java/org/apache/catalina/util/DOMWriter.java
+++ b/java/org/apache/catalina/util/DOMWriter.java
@@ -112,7 +112,6 @@ public class DOMWriter {
case Node.PROCESSING_INSTRUCTION_NODE:
out.print("<?");
out.print(node.getLocalName());
-
String piData = node.getNodeValue();
if (piData != null && !piData.isEmpty()) {
out.print(' ');
@@ -122,15 +121,45 @@ public class DOMWriter {
int start = 0;
int end = piData.indexOf("?>");
while (end >= 0) {
- out.print(piData.substring(start, end + 1));
- out.print(' ');
- start = end + 1;
+ out.print(piData.substring(start, end));
+ out.print("? >");
+ start = end + 2;
end = piData.indexOf("?>", start);
}
out.print(piData.substring(start));
}
out.print("?>");
break;
+
+ // print comment
+ case Node.COMMENT_NODE:
+ out.print("<!--");
+ String commentValue = node.getNodeValue();
+ if (commentValue != null) {
+ // The only illegal sequence in comment data is --> which
+ // would terminate the comment early. Break it with a space.
+ int start = 0;
+ int end = commentValue.indexOf("-->");
+ while (end >= 0) {
+ out.print(commentValue.substring(start, end));
+ out.print("- >");
+ start = end + 3;
+ end = commentValue.indexOf("-->", start);
+ }
+ out.print(commentValue.substring(start));
+ }
+ out.print("-->");
+ break;
+
+ // print document fragment (just its children)
+ case Node.DOCUMENT_FRAGMENT_NODE:
+ printChildren(node);
+ break;
+
+ default:
+ // Unhandled node types (ENTITY_NODE, NOTATION_NODE,
+ // ATTRIBUTE_NODE) are silently skipped.
+ break;
}
if (type == Node.ELEMENT_NODE) {
diff --git a/java/org/apache/catalina/util/ErrorPageSupport.java b/java/org/apache/catalina/util/ErrorPageSupport.java
index 7e85599f55..af87b33a02 100644
--- a/java/org/apache/catalina/util/ErrorPageSupport.java
+++ b/java/org/apache/catalina/util/ErrorPageSupport.java
@@ -97,18 +97,18 @@ public class ErrorPageSupport {
/**
- * Find the ErrorPage, if any, for the given exception type, searching up the
+ * Find the ErrorPage, if any, for the given exception, searching up the
* exception's class hierarchy.
*
- * @param exceptionType The exception instance
+ * @param exception The exception instance
*
- * @return The ErrorPage for the exception type, or {@code null} if none is configured
+ * @return The ErrorPage for the exception, or {@code null} if none is configured
*/
- public ErrorPage find(Throwable exceptionType) {
- if (exceptionType == null) {
+ public ErrorPage find(Throwable exception) {
+ if (exception == null) {
return null;
}
- Class<?> clazz = exceptionType.getClass();
+ Class<?> clazz = exception.getClass();
String name = clazz.getName();
while (!Object.class.equals(clazz)) {
ErrorPage errorPage = exceptionPages.get(name);
diff --git a/java/org/apache/catalina/util/ExactRateLimiter.java b/java/org/apache/catalina/util/ExactRateLimiter.java
index 246b5e8a77..8fb5cd9eb4 100644
--- a/java/org/apache/catalina/util/ExactRateLimiter.java
+++ b/java/org/apache/catalina/util/ExactRateLimiter.java
@@ -19,7 +19,7 @@ package org.apache.catalina.util;
import java.util.concurrent.ScheduledExecutorService;
/**
- * A RateLimiter that compromises efficiency for accuracy in order to provide exact rate limiting.
+ * A RateLimiter that trades efficiency for accuracy in order to provide exact rate limiting.
*/
public class ExactRateLimiter extends RateLimiterBase {
diff --git a/java/org/apache/catalina/util/IOTools.java b/java/org/apache/catalina/util/IOTools.java
index c6cd67ccf8..e16cf76054 100644
--- a/java/org/apache/catalina/util/IOTools.java
+++ b/java/org/apache/catalina/util/IOTools.java
@@ -94,7 +94,7 @@ public class IOTools {
* @param is The source to read from
* @param buf The buffer to write to
*
- * @return The number of bytes read
+ * @return The number of bytes actually read
*
* @throws IOException If an I/O error occurs during the read
*/
diff --git a/java/org/apache/catalina/util/Introspection.java b/java/org/apache/catalina/util/Introspection.java
index 8dc41fbcd3..5a27ca39b8 100644
--- a/java/org/apache/catalina/util/Introspection.java
+++ b/java/org/apache/catalina/util/Introspection.java
@@ -41,8 +41,9 @@ public class Introspection {
/**
- * Extract the Java Bean property name from the setter name. Note: This method assumes that the method name has
- * already been checked for correctness.
+ * Extract the Java Bean property name from the setter name. Note: This
+ * method assumes that the method name starts with "set" and has a length
+ * greater than 3.
*
* @param setter The setter method
*
diff --git a/java/org/apache/catalina/util/NetMask.java b/java/org/apache/catalina/util/NetMask.java
index 51c53769c8..8b5f7e02ce 100644
--- a/java/org/apache/catalina/util/NetMask.java
+++ b/java/org/apache/catalina/util/NetMask.java
@@ -19,6 +19,7 @@ package org.apache.catalina.util;
import java.net.InetAddress;
import java.net.UnknownHostException;
import java.util.Arrays;
+import java.util.Objects;
import java.util.regex.Pattern;
import java.util.regex.PatternSyntaxException;
@@ -296,13 +297,27 @@ public final class NetMask {
return false;
}
NetMask other = (NetMask) o;
- return nrBytes == other.nrBytes && lastByteShift == other.lastByteShift &&
- Arrays.equals(netaddr, other.netaddr);
+ if (foundPort != other.foundPort) {
+ return false;
+ }
+ if (nrBytes != other.nrBytes || lastByteShift != other.lastByteShift) {
+ return false;
+ }
+ if (!Arrays.equals(netaddr, other.netaddr)) {
+ return false;
+ }
+ if (foundPort) {
+ return Objects.equals(portPattern, other.portPattern);
+ }
+ return true;
}
@Override
public int hashCode() {
- return 31 * Arrays.hashCode(netaddr) + lastByteShift;
+ int result = 31 * (31 * Arrays.hashCode(netaddr) + nrBytes) + lastByteShift;
+ if (foundPort) {
+ result = 31 * result + portPattern.hashCode();
+ }
+ return result;
}
-
}
diff --git a/java/org/apache/catalina/util/NetMaskSet.java b/java/org/apache/catalina/util/NetMaskSet.java
index 91d7067b05..6a1e6cd042 100644
--- a/java/org/apache/catalina/util/NetMaskSet.java
+++ b/java/org/apache/catalina/util/NetMaskSet.java
@@ -104,7 +104,7 @@ public class NetMaskSet {
}
/**
- * removes all entries from the set
+ * Removes all entries from the set.
*/
public void clear() {
netmasks.clear();
diff --git a/java/org/apache/catalina/util/RateLimiterBase.java b/java/org/apache/catalina/util/RateLimiterBase.java
index cfd8b4fc6b..5ff2f7cb27 100644
--- a/java/org/apache/catalina/util/RateLimiterBase.java
+++ b/java/org/apache/catalina/util/RateLimiterBase.java
@@ -24,7 +24,7 @@ import java.util.concurrent.atomic.AtomicInteger;
import jakarta.servlet.FilterConfig;
/**
- * Base implementation of {@link RateLimiter}, provides runtime data maintenance mechanism monitor.
+ * Base implementation of {@link RateLimiter}, provides a runtime data maintenance mechanism and monitor.
*/
public abstract class RateLimiterBase implements RateLimiter {
diff --git a/java/org/apache/catalina/util/ResourceSet.java b/java/org/apache/catalina/util/ResourceSet.java
index f7a82f7451..8fcc5cdc81 100644
--- a/java/org/apache/catalina/util/ResourceSet.java
+++ b/java/org/apache/catalina/util/ResourceSet.java
@@ -19,6 +19,7 @@ package org.apache.catalina.util;
import java.io.Serial;
import java.util.Collection;
+import java.util.Comparator;
import java.util.HashSet;
import java.util.Iterator;
@@ -89,15 +90,15 @@ public final class ResourceSet<T> extends HashSet<T> {
/**
- * The current lock state of this parameter map.
+ * The current lock state of this set.
*/
private boolean locked = false;
/**
- * Return the locked state of this parameter map.
+ * Return the locked state of this set.
*
- * @return the locked state of this parameter map
+ * @return the locked state of this set
*/
public boolean isLocked() {
return this.locked;
@@ -105,7 +106,7 @@ public final class ResourceSet<T> extends HashSet<T> {
/**
- * Set the locked state of this parameter map.
+ * Set the locked state of this set.
*
* @param locked The new locked state
*/
@@ -221,6 +222,23 @@ public final class ResourceSet<T> extends HashSet<T> {
}
+ @Override
+ public java.util.Spliterator<T> spliterator() {
+ if (locked) {
+ java.util.Spliterator<T> base = super.spliterator();
+ return new java.util.Spliterator<T>() {
+ @Override public boolean tryAdvance(java.util.function.Consumer<? super T> action) { return base.tryAdvance(action); }
+ @Override public java.util.Spliterator<T> trySplit() { return null; }
+ @Override public long estimateSize() { return base.estimateSize(); }
+ @Override public int characteristics() { return base.characteristics() & ~java.util.Spliterator.SORTED; }
+ @Override public void forEachRemaining(java.util.function.Consumer<? super T> action) { base.forEachRemaining(action); }
+ @Override public Comparator<? super T> getComparator() { return base.getComparator(); }
+ };
+ }
+ return super.spliterator();
+ }
+
+
/**
* {@inheritDoc}
*
diff --git a/java/org/apache/catalina/util/ServerInfo.java b/java/org/apache/catalina/util/ServerInfo.java
index 5c8090d550..2a1e32f2fe 100644
--- a/java/org/apache/catalina/util/ServerInfo.java
+++ b/java/org/apache/catalina/util/ServerInfo.java
@@ -54,7 +54,7 @@ public class ServerInfo {
private static final String serverBuilt;
/**
- * The server built String, in ISO-8604 date format.
+ * The server built String, in ISO-8601 date format.
*/
private static final String serverBuiltIso;
diff --git a/java/org/apache/catalina/util/SessionConfig.java b/java/org/apache/catalina/util/SessionConfig.java
index a2106fb817..9c55cf5cbc 100644
--- a/java/org/apache/catalina/util/SessionConfig.java
+++ b/java/org/apache/catalina/util/SessionConfig.java
@@ -77,7 +77,7 @@ public class SessionConfig {
*
* @param context The context
*
- * @return the parameter name for the session
+ * @return the cookie path for the session
*/
public static String getSessionCookiePath(Context context) {
diff --git a/java/org/apache/catalina/util/SessionIdGeneratorBase.java b/java/org/apache/catalina/util/SessionIdGeneratorBase.java
index 45bf5bb34b..5785f4ea8b 100644
--- a/java/org/apache/catalina/util/SessionIdGeneratorBase.java
+++ b/java/org/apache/catalina/util/SessionIdGeneratorBase.java
@@ -277,7 +277,7 @@ public abstract class SessionIdGeneratorBase extends LifecycleBase implements Se
try {
result = SecureRandom.getInstance(DEFAULT_SECURE_RANDOM_ALGORITHM);
} catch (NoSuchAlgorithmException e) {
- log.error(sm.getString("sessionIdGeneratorBase.randomAlgorithm", secureRandomAlgorithm), e);
+ log.error(sm.getString("sessionIdGeneratorBase.randomAlgorithm", DEFAULT_SECURE_RANDOM_ALGORITHM), e);
}
}
diff --git a/java/org/apache/catalina/util/Strftime.java b/java/org/apache/catalina/util/Strftime.java
index c1dd436670..c9b56fab45 100644
--- a/java/org/apache/catalina/util/Strftime.java
+++ b/java/org/apache/catalina/util/Strftime.java
@@ -29,7 +29,7 @@ import java.util.TimeZone;
* were literals.</li>
* <li>Certain complicated commands, like those dealing with the week of the year probably don't have exactly the same
* behavior as strftime.</li>
- * <li>These limitations are due to use SimpleDateTime. If the conversion was done manually, all these limitations could
+ * <li>These limitations are due to use SimpleDateFormat. If the conversion was done manually, all these limitations could
* be eliminated.</li>
* <li>The interface looks like a subset of DateFormat. Maybe someday someone will make this class extend
* DateFormat.</li>
diff --git a/java/org/apache/catalina/util/TimeBucketCounter.java b/java/org/apache/catalina/util/TimeBucketCounter.java
index 91e96aa150..4f54ade92f 100644
--- a/java/org/apache/catalina/util/TimeBucketCounter.java
+++ b/java/org/apache/catalina/util/TimeBucketCounter.java
@@ -126,7 +126,7 @@ public class TimeBucketCounter extends TimeBucketCounterBase {
/**
* Returns the ratio to the next power of 2 so that we can adjust the value.
*
- * @param value of target duration in seconds
+ * @param value of target duration in milliseconds
*
* @return the ratio to the next power of 2 so that we can adjust the value
*/
diff --git a/java/org/apache/catalina/util/XMLWriter.java b/java/org/apache/catalina/util/XMLWriter.java
index 146136cccc..a2eb721e8f 100644
--- a/java/org/apache/catalina/util/XMLWriter.java
+++ b/java/org/apache/catalina/util/XMLWriter.java
@@ -111,7 +111,7 @@ public class XMLWriter {
*/
public void writeProperty(String namespace, String name, String value) {
writeElement(namespace, name, OPENING);
- buffer.append(value);
+ buffer.append(Escape.xml(value));
writeElement(namespace, name, CLOSING);
}
@@ -243,7 +243,17 @@ public class XMLWriter {
* @param data Data to append
*/
public void writeData(String data) {
- buffer.append("<![CDATA[").append(data).append("]]>");
+ buffer.append("<![CDATA[");
+ int start = 0;
+ int idx;
+ while ((idx = data.indexOf("]]>", start)) >= 0) {
+ buffer.append(data, start, idx);
+ // We terminate, then append the ]]>, and restart the sequence
+ buffer.append("]]>]]><![CDATA[");
+ start = idx + 3;
+ }
+ buffer.append(data.substring(start));
+ buffer.append("]]>");
}
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: (tomcat) branch main updated: Code review fixes
Posted by Rémy Maucherat <re...@apache.org>.
On Mon, Jul 20, 2026 at 5:52 PM Mark Thomas <ma...@apache.org> wrote:
>
> On 20/07/2026 15:42, Rémy Maucherat wrote:
> > On Mon, Jul 20, 2026 at 12:01 PM Mark Thomas <ma...@apache.org> wrote:
> >>
> >> On 15/07/2026 16:06, remm@apache.org wrote:
> >>> This is an automated email from the ASF dual-hosted git repository.
> >>>
> >>> rmaucher pushed a commit to branch main
> >>> in repository https://gitbox.apache.org/repos/asf/tomcat.git
> >>>
> >>>
> >>> The following commit(s) were added to refs/heads/main by this push:
> >>> new 60008aaaf7 Code review fixes
> >>> 60008aaaf7 is described below
> >>>
> >>> commit 60008aaaf73a5e864f382f4667d576ea2d5b2f2a
> >>> Author: remm <re...@apache.org>
> >>> AuthorDate: Wed Jul 15 17:06:29 2026 +0200
> >>>
> >>> Code review fixes
> >>
> >> <snip/>
> >>
> >>> diff --git a/java/org/apache/catalina/util/ResourceSet.java b/java/org/apache/catalina/util/ResourceSet.java
> >>> index f7a82f7451..8fcc5cdc81 100644
> >>> --- a/java/org/apache/catalina/util/ResourceSet.java
> >>> +++ b/java/org/apache/catalina/util/ResourceSet.java
> >>
> >> <snip/>
> >>
> >>> @@ -221,6 +222,23 @@ public final class ResourceSet<T> extends HashSet<T> {
> >>> }
> >>>
> >>>
> >>> + @Override
> >>> + public java.util.Spliterator<T> spliterator() {
> >>> + if (locked) {
> >>> + java.util.Spliterator<T> base = super.spliterator();
> >>> + return new java.util.Spliterator<T>() {
> >>> + @Override public boolean tryAdvance(java.util.function.Consumer<? super T> action) { return base.tryAdvance(action); }
> >>> + @Override public java.util.Spliterator<T> trySplit() { return null; }
> >>> + @Override public long estimateSize() { return base.estimateSize(); }
> >>> + @Override public int characteristics() { return base.characteristics() & ~java.util.Spliterator.SORTED; }
> >>> + @Override public void forEachRemaining(java.util.function.Consumer<? super T> action) { base.forEachRemaining(action); }
> >>> + @Override public Comparator<? super T> getComparator() { return base.getComparator(); }
> >>> + };
> >>> + }
> >>> + return super.spliterator();
> >>> + }
> >>
> >> What was the intention here? The above code is a NO-OP. ResourceSet
> >> extends HashSet which never returns the SORTED characteristic. Also, it
> >> is not clear why this is conditional on the status of locked.
> >>
> >> I did wonder if the intention was to add IMMTABLE when the Set was
> >> locked but the logic for that is sufficiently different that I am really
> >> not sure.
> >
> > Code review had a defect like this:
> > "ResourceSet overrides iterator() to return a LockedIterator wh
> > locked (line 230-235), which throws on remove(). However,
> > spliterator() is not overridden. When the set is locked, calling
> > spliterator() returns the inherited HashSet.spliterator(), whose
> > iterator allows remove() operations, bypassing the lock."
> >
> > I don't have experience using spliterator. iterator() returns a
> > LockedIterator when locked, and a regular iterator otherwise.
>
> That looks like an hallucination. HashSet.spliterator() does not have an
> iterator. Nor does it have a remove() method.
>
> I don't see a way to mutate the underlying Set via the object returned
> from spliterator().
>
> I'm going to remove that code as unnecessary.
+1
Rémy
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: (tomcat) branch main updated: Code review fixes
Posted by Mark Thomas <ma...@apache.org>.
On 20/07/2026 15:42, Rémy Maucherat wrote:
> On Mon, Jul 20, 2026 at 12:01 PM Mark Thomas <ma...@apache.org> wrote:
>>
>> On 15/07/2026 16:06, remm@apache.org wrote:
>>> This is an automated email from the ASF dual-hosted git repository.
>>>
>>> rmaucher pushed a commit to branch main
>>> in repository https://gitbox.apache.org/repos/asf/tomcat.git
>>>
>>>
>>> The following commit(s) were added to refs/heads/main by this push:
>>> new 60008aaaf7 Code review fixes
>>> 60008aaaf7 is described below
>>>
>>> commit 60008aaaf73a5e864f382f4667d576ea2d5b2f2a
>>> Author: remm <re...@apache.org>
>>> AuthorDate: Wed Jul 15 17:06:29 2026 +0200
>>>
>>> Code review fixes
>>
>> <snip/>
>>
>>> diff --git a/java/org/apache/catalina/util/ResourceSet.java b/java/org/apache/catalina/util/ResourceSet.java
>>> index f7a82f7451..8fcc5cdc81 100644
>>> --- a/java/org/apache/catalina/util/ResourceSet.java
>>> +++ b/java/org/apache/catalina/util/ResourceSet.java
>>
>> <snip/>
>>
>>> @@ -221,6 +222,23 @@ public final class ResourceSet<T> extends HashSet<T> {
>>> }
>>>
>>>
>>> + @Override
>>> + public java.util.Spliterator<T> spliterator() {
>>> + if (locked) {
>>> + java.util.Spliterator<T> base = super.spliterator();
>>> + return new java.util.Spliterator<T>() {
>>> + @Override public boolean tryAdvance(java.util.function.Consumer<? super T> action) { return base.tryAdvance(action); }
>>> + @Override public java.util.Spliterator<T> trySplit() { return null; }
>>> + @Override public long estimateSize() { return base.estimateSize(); }
>>> + @Override public int characteristics() { return base.characteristics() & ~java.util.Spliterator.SORTED; }
>>> + @Override public void forEachRemaining(java.util.function.Consumer<? super T> action) { base.forEachRemaining(action); }
>>> + @Override public Comparator<? super T> getComparator() { return base.getComparator(); }
>>> + };
>>> + }
>>> + return super.spliterator();
>>> + }
>>
>> What was the intention here? The above code is a NO-OP. ResourceSet
>> extends HashSet which never returns the SORTED characteristic. Also, it
>> is not clear why this is conditional on the status of locked.
>>
>> I did wonder if the intention was to add IMMTABLE when the Set was
>> locked but the logic for that is sufficiently different that I am really
>> not sure.
>
> Code review had a defect like this:
> "ResourceSet overrides iterator() to return a LockedIterator wh
> locked (line 230-235), which throws on remove(). However,
> spliterator() is not overridden. When the set is locked, calling
> spliterator() returns the inherited HashSet.spliterator(), whose
> iterator allows remove() operations, bypassing the lock."
>
> I don't have experience using spliterator. iterator() returns a
> LockedIterator when locked, and a regular iterator otherwise.
That looks like an hallucination. HashSet.spliterator() does not have an
iterator. Nor does it have a remove() method.
I don't see a way to mutate the underlying Set via the object returned
from spliterator().
I'm going to remove that code as unnecessary.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: (tomcat) branch main updated: Code review fixes
Posted by Rémy Maucherat <re...@apache.org>.
On Mon, Jul 20, 2026 at 12:01 PM Mark Thomas <ma...@apache.org> wrote:
>
> On 15/07/2026 16:06, remm@apache.org wrote:
> > This is an automated email from the ASF dual-hosted git repository.
> >
> > rmaucher pushed a commit to branch main
> > in repository https://gitbox.apache.org/repos/asf/tomcat.git
> >
> >
> > The following commit(s) were added to refs/heads/main by this push:
> > new 60008aaaf7 Code review fixes
> > 60008aaaf7 is described below
> >
> > commit 60008aaaf73a5e864f382f4667d576ea2d5b2f2a
> > Author: remm <re...@apache.org>
> > AuthorDate: Wed Jul 15 17:06:29 2026 +0200
> >
> > Code review fixes
>
> <snip/>
>
> > diff --git a/java/org/apache/catalina/util/ResourceSet.java b/java/org/apache/catalina/util/ResourceSet.java
> > index f7a82f7451..8fcc5cdc81 100644
> > --- a/java/org/apache/catalina/util/ResourceSet.java
> > +++ b/java/org/apache/catalina/util/ResourceSet.java
>
> <snip/>
>
> > @@ -221,6 +222,23 @@ public final class ResourceSet<T> extends HashSet<T> {
> > }
> >
> >
> > + @Override
> > + public java.util.Spliterator<T> spliterator() {
> > + if (locked) {
> > + java.util.Spliterator<T> base = super.spliterator();
> > + return new java.util.Spliterator<T>() {
> > + @Override public boolean tryAdvance(java.util.function.Consumer<? super T> action) { return base.tryAdvance(action); }
> > + @Override public java.util.Spliterator<T> trySplit() { return null; }
> > + @Override public long estimateSize() { return base.estimateSize(); }
> > + @Override public int characteristics() { return base.characteristics() & ~java.util.Spliterator.SORTED; }
> > + @Override public void forEachRemaining(java.util.function.Consumer<? super T> action) { base.forEachRemaining(action); }
> > + @Override public Comparator<? super T> getComparator() { return base.getComparator(); }
> > + };
> > + }
> > + return super.spliterator();
> > + }
>
> What was the intention here? The above code is a NO-OP. ResourceSet
> extends HashSet which never returns the SORTED characteristic. Also, it
> is not clear why this is conditional on the status of locked.
>
> I did wonder if the intention was to add IMMTABLE when the Set was
> locked but the logic for that is sufficiently different that I am really
> not sure.
Code review had a defect like this:
"ResourceSet overrides iterator() to return a LockedIterator when
locked (line 230-235), which throws on remove(). However,
spliterator() is not overridden. When the set is locked, calling
spliterator() returns the inherited HashSet.spliterator(), whose
iterator allows remove() operations, bypassing the lock."
I don't have experience using spliterator. iterator() returns a
LockedIterator when locked, and a regular iterator otherwise.
Rémy
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org
Re: (tomcat) branch main updated: Code review fixes
Posted by Mark Thomas <ma...@apache.org>.
On 15/07/2026 16:06, remm@apache.org wrote:
> This is an automated email from the ASF dual-hosted git repository.
>
> rmaucher pushed a commit to branch main
> in repository https://gitbox.apache.org/repos/asf/tomcat.git
>
>
> The following commit(s) were added to refs/heads/main by this push:
> new 60008aaaf7 Code review fixes
> 60008aaaf7 is described below
>
> commit 60008aaaf73a5e864f382f4667d576ea2d5b2f2a
> Author: remm <re...@apache.org>
> AuthorDate: Wed Jul 15 17:06:29 2026 +0200
>
> Code review fixes
<snip/>
> diff --git a/java/org/apache/catalina/util/ResourceSet.java b/java/org/apache/catalina/util/ResourceSet.java
> index f7a82f7451..8fcc5cdc81 100644
> --- a/java/org/apache/catalina/util/ResourceSet.java
> +++ b/java/org/apache/catalina/util/ResourceSet.java
<snip/>
> @@ -221,6 +222,23 @@ public final class ResourceSet<T> extends HashSet<T> {
> }
>
>
> + @Override
> + public java.util.Spliterator<T> spliterator() {
> + if (locked) {
> + java.util.Spliterator<T> base = super.spliterator();
> + return new java.util.Spliterator<T>() {
> + @Override public boolean tryAdvance(java.util.function.Consumer<? super T> action) { return base.tryAdvance(action); }
> + @Override public java.util.Spliterator<T> trySplit() { return null; }
> + @Override public long estimateSize() { return base.estimateSize(); }
> + @Override public int characteristics() { return base.characteristics() & ~java.util.Spliterator.SORTED; }
> + @Override public void forEachRemaining(java.util.function.Consumer<? super T> action) { base.forEachRemaining(action); }
> + @Override public Comparator<? super T> getComparator() { return base.getComparator(); }
> + };
> + }
> + return super.spliterator();
> + }
What was the intention here? The above code is a NO-OP. ResourceSet
extends HashSet which never returns the SORTED characteristic. Also, it
is not clear why this is conditional on the status of locked.
I did wonder if the intention was to add IMMTABLE when the Set was
locked but the logic for that is sufficiently different that I am really
not sure.
Mark
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org