diff --git a/src/main/java/org/apache/commons/lang3/builder/HashCodeBuilder.java b/src/main/java/org/apache/commons/lang3/builder/HashCodeBuilder.java index c8edf4878fb..218fdfb2191 100644 --- a/src/main/java/org/apache/commons/lang3/builder/HashCodeBuilder.java +++ b/src/main/java/org/apache/commons/lang3/builder/HashCodeBuilder.java @@ -169,6 +169,13 @@ public Builder setMultiplierOddNumber(final int multiplierOddNumber) { */ private static final ThreadLocal> REGISTRY = ThreadLocal.withInitial(HashSet::new); + /** + * A registry of objects being appended by {@link #append(Object)}, kept separate from {@link #REGISTRY} so that + * guarding {@code append} against its own re-entrant cycles does not trip the reflection cycle guard checked by + * {@link #reflectionAppend(Object, Class, HashCodeBuilder, boolean, String[], boolean)}. + */ + private static final ThreadLocal> APPEND_REGISTRY = ThreadLocal.withInitial(HashSet::new); + /** * Constructs a new Builder. * @@ -541,6 +548,38 @@ private static void unregister(final Object value) { } } + /** + * Tests whether the append registry contains the given object. Used by {@link #append(Object)} to break its own re-entrant cycles. + * + * @param value The object to look up in the append registry. + * @return {@code true} if the append registry contains the given object. + */ + private static boolean isAppendRegistered(final Object value) { + return APPEND_REGISTRY.get().contains(new IDKey(value)); + } + + /** + * Registers the given object in the append registry. + * + * @param value The object to register. + */ + private static void appendRegister(final Object value) { + APPEND_REGISTRY.get().add(new IDKey(value)); + } + + /** + * Unregisters the given object from the append registry. + * + * @param value The object to unregister. + */ + private static void appendUnregister(final Object value) { + final Set registry = APPEND_REGISTRY.get(); + registry.remove(new IDKey(value)); + if (registry.isEmpty()) { + APPEND_REGISTRY.remove(); + } + } + /** * Constant to use in building the hashCode. */ @@ -820,22 +859,22 @@ public HashCodeBuilder append(final long[] array) { public HashCodeBuilder append(final Object object) { if (object == null) { total = total * constant; - } else if (isRegistered(object)) { + } else if (isRegistered(object) || isAppendRegistered(object)) { // Cycle detected: skip to avoid infinite recursion (mirrors reflectionAppend). total = total * constant; } else if (ObjectUtils.isArray(object)) { try { - register(object); + appendRegister(object); appendArray(object); } finally { - unregister(object); + appendUnregister(object); } } else { try { - register(object); + appendRegister(object); total = total * constant + object.hashCode(); } finally { - unregister(object); + appendUnregister(object); } } return this; diff --git a/src/test/java/org/apache/commons/lang3/builder/HashCodeBuilderTest.java b/src/test/java/org/apache/commons/lang3/builder/HashCodeBuilderTest.java index 01f0e775bce..ae50382489a 100644 --- a/src/test/java/org/apache/commons/lang3/builder/HashCodeBuilderTest.java +++ b/src/test/java/org/apache/commons/lang3/builder/HashCodeBuilderTest.java @@ -66,6 +66,48 @@ public int hashCode() { } } + /** + * A reflection test fixture whose {@code hashCode()} is reflection based. + */ + static final class ReflectionHashCodeMember { + final int value; + + ReflectionHashCodeMember(final int value) { + this.value = value; + } + + @Override + public boolean equals(final Object o) { + return EqualsBuilder.reflectionEquals(this, o); + } + + @Override + public int hashCode() { + return HashCodeBuilder.reflectionHashCode(this); + } + } + + /** + * Holds a {@link ReflectionHashCodeMember} and is itself reflection hashed. + */ + static final class ReflectionHashCodeHolder { + final ReflectionHashCodeMember member; + + ReflectionHashCodeHolder(final ReflectionHashCodeMember member) { + this.member = member; + } + + @Override + public boolean equals(final Object o) { + return EqualsBuilder.reflectionEquals(this, o); + } + + @Override + public int hashCode() { + return HashCodeBuilder.reflectionHashCode(this); + } + } + static class TestObject { private int a; @@ -636,4 +678,23 @@ void testToHashCodeExclude() { assertEquals(INITIAL, HashCodeBuilder.reflectionHashCode(two)); } + /** + * {@code append(Object)} must not collapse an object whose own {@code hashCode()} is reflection based to the bare + * initial value. Regression for the cycle guard leaking into the reflection registry. + */ + @Test + void testAppendObjectReflectionHashCodeNotPoisoned() { + final ReflectionHashCodeMember member = new ReflectionHashCodeMember(42); + assertEquals(INITIAL * CONSTANT + member.hashCode(), + new HashCodeBuilder(INITIAL, CONSTANT).append((Object) member).toHashCode()); + + // The same defect reached through a normal, acyclic graph: a nested object's content must still influence + // the enclosing reflection hash. + final int h1 = new ReflectionHashCodeHolder(new ReflectionHashCodeMember(1)).hashCode(); + final int h2 = new ReflectionHashCodeHolder(new ReflectionHashCodeMember(2)).hashCode(); + assertNotEquals(h1, h2); + + assertTrue(HashCodeBuilder.getRegistry().isEmpty()); + } + }