This is an automated email from the ASF dual-hosted git repository.
garydgregory pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/commons-lang.git
The following commit(s) were added to refs/heads/master by this push:
new 8e2d1bed8 CompareToBuilder lacks cycle guard on Object[] recursion.
(#1669)
8e2d1bed8 is described below
commit 8e2d1bed8623e0f5b281504d151476689f21690a
Author: Gary Gregory <[email protected]>
AuthorDate: Sun May 24 09:01:39 2026 -0400
CompareToBuilder lacks cycle guard on Object[] recursion. (#1669)
---
.../commons/lang3/builder/AbstractReflection.java | 30 +++++++
.../commons/lang3/builder/CompareToBuilder.java | 93 +++++++++++++++++++---
.../commons/lang3/builder/EqualsBuilder.java | 25 +-----
.../lang3/builder/CompareToBuilderTest.java | 29 +++++++
4 files changed, 142 insertions(+), 35 deletions(-)
diff --git
a/src/main/java/org/apache/commons/lang3/builder/AbstractReflection.java
b/src/main/java/org/apache/commons/lang3/builder/AbstractReflection.java
index 169cd6ef7..3a33788c6 100644
--- a/src/main/java/org/apache/commons/lang3/builder/AbstractReflection.java
+++ b/src/main/java/org/apache/commons/lang3/builder/AbstractReflection.java
@@ -19,9 +19,11 @@
import java.lang.reflect.AccessibleObject;
import java.lang.reflect.Field;
+import java.util.Set;
import java.util.function.Supplier;
import org.apache.commons.lang3.SystemProperties;
+import org.apache.commons.lang3.tuple.Pair;
/**
* Abstracts reflection access for reflection-based classes in this package.
@@ -111,6 +113,16 @@ static boolean getForceAccessible() {
return SystemProperties.getBoolean(AbstractReflection.class,
"forceAccessible", () -> true);
}
+ static boolean isRegistered(final Object lhs, final Object rhs, final
Set<Pair<IDKey, IDKey>> registry) {
+ final Pair<IDKey, IDKey> pair = toRegisterPair(lhs, rhs);
+ final Pair<IDKey, IDKey> swappedPair = Pair.of(pair.getRight(),
pair.getLeft());
+ return registry != null && (registry.contains(pair) ||
registry.contains(swappedPair));
+ }
+
+ static void register(final Object lhs, final Object rhs, final
Set<Pair<IDKey, IDKey>> registry) {
+ registry.add(toRegisterPair(lhs, rhs));
+ }
+
/**
* If {@code forceAccessible} flag is true, then the field is made
accessible by calling {@link AccessibleObject#setAccessible(boolean)
* AccessibleObject#setAccessible(true)} but <em>only</em> if a field is
not already accessible.
@@ -147,6 +159,24 @@ private static boolean setAccessibleTrue(final Field
field) {
return false;
}
+ /**
+ * Converters value pair into a register pair.
+ *
+ * @param lhs {@code this} object.
+ * @param rhs the other object.
+ * @return the pair.
+ */
+ static Pair<IDKey, IDKey> toRegisterPair(final Object lhs, final Object
rhs) {
+ return Pair.of(new IDKey(lhs), new IDKey(rhs));
+ }
+
+ static void unregister(final Object lhs, final Object rhs, final
Set<Pair<IDKey, IDKey>> registry, final ThreadLocal<Set<Pair<IDKey, IDKey>>>
registryTL) {
+ registry.remove(toRegisterPair(lhs, rhs));
+ if (registry.isEmpty()) {
+ registryTL.remove();
+ }
+ }
+
/**
* Whether to call {@link AccessibleObject#setAccessible(boolean)
AccessibleObject#setAccessible(true)} on inaccessible fields.
*/
diff --git
a/src/main/java/org/apache/commons/lang3/builder/CompareToBuilder.java
b/src/main/java/org/apache/commons/lang3/builder/CompareToBuilder.java
index a11307c58..e7cc0cfa9 100644
--- a/src/main/java/org/apache/commons/lang3/builder/CompareToBuilder.java
+++ b/src/main/java/org/apache/commons/lang3/builder/CompareToBuilder.java
@@ -20,11 +20,14 @@
import java.lang.reflect.Modifier;
import java.util.Collection;
import java.util.Comparator;
+import java.util.HashSet;
import java.util.Objects;
+import java.util.Set;
import org.apache.commons.lang3.ArrayUtils;
import org.apache.commons.lang3.ObjectUtils;
import org.apache.commons.lang3.builder.AbstractReflection.AbstractBuilder;
+import org.apache.commons.lang3.tuple.Pair;
/**
* Assists in implementing {@link Comparable#compareTo(Object)} methods.
@@ -118,6 +121,11 @@ public CompareToBuilder get() {
}
+ /**
+ * A registry of objects to detect cyclical object references, avoid
infinite loops, and stack overflows.
+ */
+ private static final ThreadLocal<Set<Pair<IDKey, IDKey>>> REGISTRY =
ThreadLocal.withInitial(HashSet::new);
+
/**
* Constructs a new Builder.
*
@@ -127,6 +135,32 @@ public static Builder builder() {
return new Builder();
}
+ /**
+ * Gets the registry of object pairs being traversed by the reflection
+ * methods in the current thread.
+ *
+ * @return Set the registry of objects being traversed
+ */
+ static Set<Pair<IDKey, IDKey>> getRegistry() {
+ return REGISTRY.get();
+ }
+
+ /**
+ * Tests whether the registry contains the given object pair.
+ * <p>
+ * Used by the reflection methods to avoid infinite loops.
+ * Objects might be swapped therefore a check is needed if the object pair
+ * is registered in the given or swapped order.
+ * </p>
+ *
+ * @param lhs {@code this} object to lookup in registry
+ * @param rhs the other object to lookup on registry
+ * @return boolean {@code true} if the registry contains the given object.
+ */
+ static boolean isRegistered(final Object lhs, final Object rhs) {
+ return isRegistered(lhs, rhs, getRegistry());
+ }
+
/**
* Appends to {@code builder} the comparison of {@code lhs}
* to {@code rhs} using the fields defined in {@code clazz}.
@@ -349,6 +383,31 @@ public static int reflectionCompare(final Object lhs,
final Object rhs, final St
return reflectionCompare(lhs, rhs, false, null, excludeFields);
}
+ /**
+ * Registers the given object pair.
+ * Used by the reflection methods to avoid infinite loops.
+ *
+ * @param lhs {@code this} object to register
+ * @param rhs the other object to register
+ */
+ private static void register(final Object lhs, final Object rhs) {
+ register(lhs, rhs, getRegistry());
+ }
+
+ /**
+ * Unregisters the given object pair.
+ *
+ * <p>
+ * Used by the reflection methods to avoid infinite loops.
+ * </p>
+ *
+ * @param lhs {@code this} object to unregister
+ * @param rhs the other object to unregister
+ */
+ private static void unregister(final Object lhs, final Object rhs) {
+ unregister(lhs, rhs, getRegistry(), REGISTRY);
+ }
+
/**
* Current state of the comparison as appended fields are checked.
*/
@@ -842,20 +901,28 @@ public CompareToBuilder append(final Object lhs, final
Object rhs, final Compara
comparison = 1;
return this;
}
- if (ObjectUtils.isArray(lhs)) {
- // factor out array case in order to keep method small enough to
be inlined
- appendArray(lhs, rhs, comparator);
- } else // the simple case, not an array, just test the element
- if (comparator == null) {
- @SuppressWarnings("unchecked") // assume this can be done; if not
throw CCE as per Javadoc
- final Comparable<Object> comparable = (Comparable<Object>) lhs;
- comparison = comparable.compareTo(rhs);
- } else {
- @SuppressWarnings("unchecked") // assume this can be done; if not
throw CCE as per Javadoc
- final Comparator<Object> comparator2 = (Comparator<Object>)
comparator;
- comparison = comparator2.compare(lhs, rhs);
+ if (isRegistered(lhs, rhs)) {
+ return this;
+ }
+ try {
+ register(lhs, rhs);
+ if (ObjectUtils.isArray(lhs)) {
+ // factor out array case in order to keep method small enough
to be inlined
+ appendArray(lhs, rhs, comparator);
+ } else // the simple case, not an array, just test the element
+ if (comparator == null) {
+ @SuppressWarnings("unchecked") // assume this can be done; if
not throw CCE as per Javadoc
+ final Comparable<Object> comparable = (Comparable<Object>) lhs;
+ comparison = comparable.compareTo(rhs);
+ } else {
+ @SuppressWarnings("unchecked") // assume this can be done; if
not throw CCE as per Javadoc
+ final Comparator<Object> comparator2 = (Comparator<Object>)
comparator;
+ comparison = comparator2.compare(lhs, rhs);
+ }
+ return this;
+ } finally {
+ unregister(lhs, rhs);
}
- return this;
}
/**
diff --git a/src/main/java/org/apache/commons/lang3/builder/EqualsBuilder.java
b/src/main/java/org/apache/commons/lang3/builder/EqualsBuilder.java
index f665f4175..746edb944 100644
--- a/src/main/java/org/apache/commons/lang3/builder/EqualsBuilder.java
+++ b/src/main/java/org/apache/commons/lang3/builder/EqualsBuilder.java
@@ -141,17 +141,6 @@ public static Builder builder() {
* to disambiguate the duplicate ids.
*/
- /**
- * Converters value pair into a register pair.
- *
- * @param lhs {@code this} object
- * @param rhs the other object
- * @return the pair
- */
- static Pair<IDKey, IDKey> getRegisterPair(final Object lhs, final Object
rhs) {
- return Pair.of(new IDKey(lhs), new IDKey(rhs));
- }
-
/**
* Gets the registry of object pairs being traversed by the reflection
* methods in the current thread.
@@ -173,13 +162,9 @@ static Set<Pair<IDKey, IDKey>> getRegistry() {
* @param lhs {@code this} object to lookup in registry
* @param rhs the other object to lookup on registry
* @return boolean {@code true} if the registry contains the given object.
- * @since 3.0
*/
static boolean isRegistered(final Object lhs, final Object rhs) {
- final Set<Pair<IDKey, IDKey>> registry = getRegistry();
- final Pair<IDKey, IDKey> pair = getRegisterPair(lhs, rhs);
- final Pair<IDKey, IDKey> swappedPair = Pair.of(pair.getRight(),
pair.getLeft());
- return registry != null && (registry.contains(pair) ||
registry.contains(swappedPair));
+ return isRegistered(lhs, rhs, getRegistry());
}
/**
@@ -353,7 +338,7 @@ public static boolean reflectionEquals(final Object lhs,
final Object rhs, final
* @param rhs the other object to register
*/
private static void register(final Object lhs, final Object rhs) {
- getRegistry().add(getRegisterPair(lhs, rhs));
+ register(lhs, rhs, getRegistry());
}
/**
@@ -367,11 +352,7 @@ private static void register(final Object lhs, final
Object rhs) {
* @param rhs the other object to unregister
*/
private static void unregister(final Object lhs, final Object rhs) {
- final Set<Pair<IDKey, IDKey>> registry = getRegistry();
- registry.remove(getRegisterPair(lhs, rhs));
- if (registry.isEmpty()) {
- REGISTRY.remove();
- }
+ unregister(lhs, rhs, getRegistry(), REGISTRY);
}
/**
diff --git
a/src/test/java/org/apache/commons/lang3/builder/CompareToBuilderTest.java
b/src/test/java/org/apache/commons/lang3/builder/CompareToBuilderTest.java
index b3c3c4c04..c49d0e217 100644
--- a/src/test/java/org/apache/commons/lang3/builder/CompareToBuilderTest.java
+++ b/src/test/java/org/apache/commons/lang3/builder/CompareToBuilderTest.java
@@ -96,6 +96,35 @@ public int hashCode() {
}
}
+ /**
+ * Mutually-referential {@code Object[]}s: {@code a[0] = b}, {@code b[0] =
a}, with {@code a != b}. The recursion is
+ * {@code append(a,b) -> append(a[0]=b, b[0]=a) -> append(b,a) ->
+ * append(b[0]=a, a[0]=b) -> append(a,b)}, never terminating.
+ */
+ @Test
+ void testCycleMutuallyReferentialObjectArrays() {
+ final Object[] a = new Object[1];
+ final Object[] b = new Object[1];
+ a[0] = b;
+ b[0] = a;
+ assertEquals(0, new CompareToBuilder().append(a, b,
null).toComparison());
+ assertEquals(0, new CompareToBuilder().append((Object) a, (Object) b,
null).toComparison());
+ }
+
+ /**
+ * Two distinct self-referential {@code Object[]}s: {@code a[0] = a},
{@code b[0] = b}, with {@code a != b}. The recursion is
+ * {@code append(a,b) -> append(a[0]=a, b[0]=b) -> append(a,b)}, never
terminating.
+ */
+ @Test
+ void testCycleTwoDistinctSelfReferentialObjectArrays() {
+ final Object[] a = new Object[1];
+ final Object[] b = new Object[1];
+ a[0] = a;
+ b[0] = b;
+ assertEquals(0, new CompareToBuilder().append(a, b,
null).toComparison());
+ assertEquals(0, new CompareToBuilder().append((Object) a, (Object) b,
null).toComparison());
+ }
+
static class TestTransientSubObject extends TestObject {
@SuppressWarnings("unused")
private final transient int t;
