diff --git a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/appender/nosql/NoSqlDatabaseManagerTest.java b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/appender/nosql/NoSqlDatabaseManagerTest.java index e9c53a37064..d53769e8b8d 100644 --- a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/appender/nosql/NoSqlDatabaseManagerTest.java +++ b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/appender/nosql/NoSqlDatabaseManagerTest.java @@ -25,6 +25,7 @@ import static org.mockito.BDDMockito.then; import static org.mockito.Mockito.mock; +import foo.TestFriendlyException; import java.io.IOException; import java.sql.SQLException; import java.util.Collection; @@ -428,4 +429,28 @@ public void testWriteInternal03() { assertEquals("The context stack is not correct.", stack.asList(), object.get("contextStack")); } } + + @Test + public void testWriteInternalWithCyclicCause() { + given(connection.isClosed()).willReturn(false); + final Throwable exception = TestFriendlyException.INSTANCE; + + try (final NoSqlDatabaseManager manager = + NoSqlDatabaseManager.getNoSqlDatabaseManager("name", 0, provider, null, null)) { + manager.startup(); + manager.connectAndStart(); + manager.writeInternal( + Log4jLogEvent.newBuilder().setThrown(exception).build(), null); + + then(connection).should().insertObject(captor.capture()); + final Map thrown = + (Map) captor.getValue().unwrap().get("thrown"); + final Map cause = (Map) thrown.get("cause"); + final Map nestedCause = (Map) cause.get("cause"); + assertEquals(exception.getMessage(), thrown.get("message")); + assertEquals(exception.getCause().getMessage(), cause.get("message")); + assertEquals(exception.getCause().getCause().getMessage(), nestedCause.get("message")); + assertNull("The cycle should not be serialized.", nestedCause.get("cause")); + } + } } diff --git a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/util/ThrowablesTest.java b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/util/ThrowablesTest.java index 912d62a1d90..b1a2370aaea 100644 --- a/log4j-core-test/src/test/java/org/apache/logging/log4j/core/util/ThrowablesTest.java +++ b/log4j-core-test/src/test/java/org/apache/logging/log4j/core/util/ThrowablesTest.java @@ -17,8 +17,10 @@ package org.apache.logging.log4j.core.util; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertSame; import static org.junit.jupiter.api.Assertions.assertThrows; +import foo.TestFriendlyException; import org.junit.jupiter.api.Test; class ThrowablesTest { @@ -53,6 +55,13 @@ void testGetRootCauseLoop() { assertEquals(cause1, Throwables.getRootCause(cause3)); } + @Test + void testGetRootCauseWithCollidingExceptions() { + final Throwable throwable = TestFriendlyException.INSTANCE; + final Throwable rootCause = throwable.getCause().getCause(); + assertSame(rootCause, Throwables.getRootCause(throwable)); + } + @Test void testRethrowRuntimeException() { assertThrows(NullPointerException.class, () -> Throwables.rethrow(new NullPointerException())); diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/nosql/NoSqlDatabaseManager.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/nosql/NoSqlDatabaseManager.java index e053944a0c8..edfa07178ce 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/nosql/NoSqlDatabaseManager.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/appender/nosql/NoSqlDatabaseManager.java @@ -17,7 +17,10 @@ package org.apache.logging.log4j.core.appender.nosql; import java.io.Serializable; +import java.util.Collections; +import java.util.IdentityHashMap; import java.util.Objects; +import java.util.Set; import java.util.stream.Stream; import org.apache.logging.log4j.Marker; import org.apache.logging.log4j.ThreadContext; @@ -223,8 +226,11 @@ private void setFields(final LogEvent event, final NoSqlObject entity) { exceptionEntity.set("type", thrown.getClass().getName()); exceptionEntity.set("message", thrown.getMessage()); exceptionEntity.set("stackTrace", this.convertStackTrace(thrown.getStackTrace())); - while (thrown.getCause() != null) { - thrown = thrown.getCause(); + final Set visitedThrowables = Collections.newSetFromMap(new IdentityHashMap<>()); + visitedThrowables.add(thrown); + Throwable cause; + while ((cause = thrown.getCause()) != null && visitedThrowables.add(cause)) { + thrown = cause; final NoSqlObject causingExceptionEntity = this.connection.createObject(); causingExceptionEntity.set("type", thrown.getClass().getName()); causingExceptionEntity.set("message", thrown.getMessage()); diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/ThrowableExtendedStackTraceRenderer.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/ThrowableExtendedStackTraceRenderer.java index c70e57d4d46..114bea94f5a 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/ThrowableExtendedStackTraceRenderer.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/pattern/ThrowableExtendedStackTraceRenderer.java @@ -20,7 +20,7 @@ import java.util.Collections; import java.util.Deque; import java.util.HashMap; -import java.util.HashSet; +import java.util.IdentityHashMap; import java.util.List; import java.util.Map; import java.util.Queue; @@ -112,7 +112,7 @@ private static Map createClassResourceInfoByName( final Map classResourceInfoByName = new HashMap<>(); // Walk over the causal chain - final Set visitedThrowables = new HashSet<>(); + final Set visitedThrowables = Collections.newSetFromMap(new IdentityHashMap<>()); final Queue pendingThrowables = new ArrayDeque<>(Collections.singleton(rootThrowable)); Throwable throwable; while ((throwable = pendingThrowables.poll()) != null && visitedThrowables.add(throwable)) { diff --git a/log4j-core/src/main/java/org/apache/logging/log4j/core/util/Throwables.java b/log4j-core/src/main/java/org/apache/logging/log4j/core/util/Throwables.java index cf00631f9f8..a6730006741 100644 --- a/log4j-core/src/main/java/org/apache/logging/log4j/core/util/Throwables.java +++ b/log4j-core/src/main/java/org/apache/logging/log4j/core/util/Throwables.java @@ -25,7 +25,8 @@ import java.io.StringReader; import java.io.StringWriter; import java.util.ArrayList; -import java.util.HashSet; +import java.util.Collections; +import java.util.IdentityHashMap; import java.util.List; import java.util.Set; import org.apache.logging.log4j.core.internal.annotation.SuppressFBWarnings; @@ -46,7 +47,7 @@ private Throwables() {} */ public static Throwable getRootCause(final Throwable throwable) { requireNonNull(throwable, "throwable"); - final Set visitedThrowables = new HashSet<>(); + final Set visitedThrowables = Collections.newSetFromMap(new IdentityHashMap<>()); Throwable prevCause = throwable; visitedThrowables.add(prevCause); Throwable nextCause; diff --git a/log4j-jpa/src/main/java/org/apache/logging/log4j/core/appender/db/jpa/converter/ThrowableAttributeConverter.java b/log4j-jpa/src/main/java/org/apache/logging/log4j/core/appender/db/jpa/converter/ThrowableAttributeConverter.java index 693d05dcf2e..1448d3b2871 100644 --- a/log4j-jpa/src/main/java/org/apache/logging/log4j/core/appender/db/jpa/converter/ThrowableAttributeConverter.java +++ b/log4j-jpa/src/main/java/org/apache/logging/log4j/core/appender/db/jpa/converter/ThrowableAttributeConverter.java @@ -20,8 +20,11 @@ import java.lang.reflect.Field; import java.util.ArrayList; import java.util.Arrays; +import java.util.Collections; +import java.util.IdentityHashMap; import java.util.List; import java.util.ListIterator; +import java.util.Set; import javax.persistence.AttributeConverter; import javax.persistence.Converter; import org.apache.logging.log4j.util.LoaderUtil; @@ -64,13 +67,18 @@ public String convertToDatabaseColumn(final Throwable throwable) { } private void convertThrowable(final StringBuilder builder, final Throwable throwable) { - builder.append(throwable.toString()).append('\n'); - for (final StackTraceElement element : throwable.getStackTrace()) { - builder.append("\tat ").append(element).append('\n'); - } - if (throwable.getCause() != null) { + final Set visitedThrowables = Collections.newSetFromMap(new IdentityHashMap<>()); + for (Throwable currentThrowable = throwable; visitedThrowables.add(currentThrowable); ) { + builder.append(currentThrowable).append('\n'); + for (final StackTraceElement element : currentThrowable.getStackTrace()) { + builder.append("\tat ").append(element).append('\n'); + } + final Throwable cause = currentThrowable.getCause(); + if (cause == null || visitedThrowables.contains(cause)) { + break; + } builder.append("Caused by "); - this.convertThrowable(builder, throwable.getCause()); + currentThrowable = cause; } } diff --git a/log4j-jpa/src/test/java/org/apache/logging/log4j/core/appender/db/jpa/converter/ThrowableAttributeConverterTest.java b/log4j-jpa/src/test/java/org/apache/logging/log4j/core/appender/db/jpa/converter/ThrowableAttributeConverterTest.java index f57d1762edf..5ad40fd144c 100644 --- a/log4j-jpa/src/test/java/org/apache/logging/log4j/core/appender/db/jpa/converter/ThrowableAttributeConverterTest.java +++ b/log4j-jpa/src/test/java/org/apache/logging/log4j/core/appender/db/jpa/converter/ThrowableAttributeConverterTest.java @@ -19,8 +19,10 @@ import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; import java.sql.SQLException; +import org.apache.commons.lang3.StringUtils; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Tag; import org.junit.jupiter.api.Test; @@ -70,6 +72,18 @@ void testConvert02() { assertEquals(stackTrace, getStackTrace(reversed), "The reversed value is not correct."); } + @Test + void testConvertCyclicCause() { + final Exception exception1 = new Exception("exception1"); + final Exception exception2 = new Exception("exception2"); + exception1.initCause(exception2); + exception2.initCause(exception1); + final String converted = converter.convertToDatabaseColumn(exception1); + assertTrue(converted.contains("exception1")); + assertTrue(converted.contains("exception2")); + assertEquals(1, StringUtils.countMatches(converted, "Caused by ")); + } + @Test void testConvertNullToDatabaseColumn() { assertNull(this.converter.convertToDatabaseColumn(null), "The converted value should be null."); diff --git a/src/changelog/.2.x.x/4249_fix-circular-exception.xml b/src/changelog/.2.x.x/4249_fix-circular-exception.xml new file mode 100644 index 00000000000..2b27b6f9dd6 --- /dev/null +++ b/src/changelog/.2.x.x/4249_fix-circular-exception.xml @@ -0,0 +1,12 @@ + + + + + Fix `Throwable` causal-chain handling for cyclic and identity-malfunctioning exceptions + +