LOG4J2-2948 Replace HashSet with IdentityHashMap in ParameterFormatter to detect cycles. (#471)
diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/message/MapMessage.java b/log4j-api/src/main/java/org/apache/logging/log4j/message/MapMessage.java
index 0199e67..3609d8e 100644
--- a/log4j-api/src/main/java/org/apache/logging/log4j/message/MapMessage.java
+++ b/log4j-api/src/main/java/org/apache/logging/log4j/message/MapMessage.java
@@ -367,8 +367,8 @@
sb.append(" <Entry key=\"")
.append(data.getKeyAt(i))
.append("\">");
- int size = sb.length();
- ParameterFormatter.recursiveDeepToString(data.getValueAt(i), sb, null);
+ final int size = sb.length();
+ ParameterFormatter.recursiveDeepToString(data.getValueAt(i), sb);
StringBuilders.escapeXml(sb, size);
sb.append("</Entry>\n");
}
@@ -418,7 +418,7 @@
sb.append(' ');
}
sb.append(data.getKeyAt(i)).append(Chars.EQ).append(Chars.DQUOTE);
- ParameterFormatter.recursiveDeepToString(data.getValueAt(i), sb, null);
+ ParameterFormatter.recursiveDeepToString(data.getValueAt(i), sb);
sb.append(Chars.DQUOTE);
}
}
@@ -445,7 +445,7 @@
if (quoted) {
sb.append(Chars.DQUOTE);
}
- ParameterFormatter.recursiveDeepToString(data.getValueAt(i), sb, null);
+ ParameterFormatter.recursiveDeepToString(data.getValueAt(i), sb);
if (quoted) {
sb.append(Chars.DQUOTE);
}
@@ -474,7 +474,7 @@
}
@Override
- public void formatTo(String[] formats, StringBuilder buffer) {
+ public void formatTo(final String[] formats, final StringBuilder buffer) {
format(getFormat(formats), buffer);
}
diff --git a/log4j-api/src/main/java/org/apache/logging/log4j/message/ParameterFormatter.java b/log4j-api/src/main/java/org/apache/logging/log4j/message/ParameterFormatter.java
index c01e686..55982e0 100644
--- a/log4j-api/src/main/java/org/apache/logging/log4j/message/ParameterFormatter.java
+++ b/log4j-api/src/main/java/org/apache/logging/log4j/message/ParameterFormatter.java
@@ -20,8 +20,9 @@
import java.time.format.DateTimeFormatter;
import java.util.Arrays;
import java.util.Collection;
+import java.util.Collections;
import java.util.Date;
-import java.util.HashSet;
+import java.util.IdentityHashMap;
import java.util.Map;
import java.util.Set;
@@ -61,7 +62,9 @@
private static final char DELIM_STOP = '}';
private static final char ESCAPE_CHAR = '\\';
private static final DateTimeFormatter FORMATTER =
- DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss.SSSZ").withZone(ZoneId.systemDefault());
+ DateTimeFormatter
+ .ofPattern("yyyy-MM-dd'T'HH:mm:ss.SSSZ")
+ .withZone(ZoneId.systemDefault());
private ParameterFormatter() {
}
@@ -187,7 +190,7 @@
for (int i = 0; i < argCount; i++) {
buffer.append(messagePattern, previous, indices[i]);
previous = indices[i] + 2;
- recursiveDeepToString(arguments[i], buffer, null);
+ recursiveDeepToString(arguments[i], buffer);
}
buffer.append(messagePattern, previous, messagePattern.length());
}
@@ -212,7 +215,7 @@
for (int i = 0; i < argCount; i++) {
buffer.append(messagePattern, previous, indices[i]);
previous = indices[i] + 2;
- recursiveDeepToString(arguments[i], buffer, null);
+ recursiveDeepToString(arguments[i], buffer);
}
buffer.append(messagePattern, previous, patternLength);
}
@@ -364,7 +367,7 @@
private static void writeArgOrDelimPair(final Object[] arguments, final int argCount, final int currentArgument,
final StringBuilder buffer) {
if (currentArgument < argCount) {
- recursiveDeepToString(arguments[currentArgument], buffer, null);
+ recursiveDeepToString(arguments[currentArgument], buffer);
} else {
writeDelimPair(buffer);
}
@@ -421,35 +424,54 @@
return Byte.toString((Byte) o);
}
final StringBuilder str = new StringBuilder();
- recursiveDeepToString(o, str, null);
+ recursiveDeepToString(o, str);
return str.toString();
}
/**
- * This method performs a deep toString of the given Object.
- * Primitive arrays are converted using their respective Arrays.toString methods while
- * special handling is implemented for "container types", i.e. Object[], Map and Collection because those could
- * contain themselves.
+ * This method performs a deep {@code toString()} of the given {@code Object}.
* <p>
- * dejaVu is used in case of those container types to prevent an endless recursion.
- * </p>
+ * Primitive arrays are converted using their respective {@code Arrays.toString()} methods, while
+ * special handling is implemented for <i>container types</i>, i.e. {@code Object[]}, {@code Map} and {@code Collection},
+ * because those could contain themselves.
* <p>
- * It should be noted that neither AbstractMap.toString() nor AbstractCollection.toString() implement such a
- * behavior.
- * They only check if the container is directly contained in itself, but not if a contained container contains the
- * original one. Because of that, Arrays.toString(Object[]) isn't safe either.
- * Confusing? Just read the last paragraph again and check the respective toString() implementation.
- * </p>
+ * It should be noted that neither {@code AbstractMap.toString()} nor {@code AbstractCollection.toString()} implement such a behavior.
+ * They only check if the container is directly contained in itself, but not if a contained container contains the original one.
+ * Because of that, {@code Arrays.toString(Object[])} isn't safe either.
+ * Confusing? Just read the last paragraph again and check the respective {@code toString()} implementation.
* <p>
- * This means, in effect, that logging would produce a usable output even if an ordinary System.out.println(o)
- * would produce a relatively hard-to-debug StackOverflowError.
- * </p>
+ * This means, in effect, that logging would produce a usable output even if an ordinary {@code System.out.println(o)}
+ * would produce a relatively hard-to-debug {@code StackOverflowError}.
*
- * @param o the Object to convert into a String
- * @param str the StringBuilder that o will be appended to
- * @param dejaVu a list of container identities that were already used.
+ * @param o the {@code Object} to convert into a {@code String}
+ * @param str the {@code StringBuilder} that {@code o} will be appended to
*/
- static void recursiveDeepToString(final Object o, final StringBuilder str, final Set<String> dejaVu) {
+ static void recursiveDeepToString(final Object o, final StringBuilder str) {
+ recursiveDeepToString(o, str, null);
+ }
+
+ /**
+ * This method performs a deep {@code toString()} of the given {@code Object}.
+ * <p>
+ * Primitive arrays are converted using their respective {@code Arrays.toString()} methods, while
+ * special handling is implemented for <i>container types</i>, i.e. {@code Object[]}, {@code Map} and {@code Collection},
+ * because those could contain themselves.
+ * <p>
+ * {@code dejaVu} is used in case of those container types to prevent an endless recursion.
+ * <p>
+ * It should be noted that neither {@code AbstractMap.toString()} nor {@code AbstractCollection.toString()} implement such a behavior.
+ * They only check if the container is directly contained in itself, but not if a contained container contains the original one.
+ * Because of that, {@code Arrays.toString(Object[])} isn't safe either.
+ * Confusing? Just read the last paragraph again and check the respective {@code toString()} implementation.
+ * <p>
+ * This means, in effect, that logging would produce a usable output even if an ordinary {@code System.out.println(o)}
+ * would produce a relatively hard-to-debug {@code StackOverflowError}.
+ *
+ * @param o the {@code Object} to convert into a {@code String}
+ * @param str the {@code StringBuilder} that {@code o} will be appended to
+ * @param dejaVu a set of container objects directly or transitively containing {@code o}
+ */
+ private static void recursiveDeepToString(final Object o, final StringBuilder str, final Set<Object> dejaVu) {
if (appendSpecialTypes(o, str)) {
return;
}
@@ -479,8 +501,10 @@
return o.getClass().isArray() || o instanceof Map || o instanceof Collection;
}
- private static void appendPotentiallyRecursiveValue(final Object o, final StringBuilder str,
- final Set<String> dejaVu) {
+ private static void appendPotentiallyRecursiveValue(
+ final Object o,
+ final StringBuilder str,
+ final Set<Object> dejaVu) {
final Class<?> oClass = o.getClass();
if (oClass.isArray()) {
appendArray(o, str, dejaVu, oClass);
@@ -488,10 +512,15 @@
appendMap(o, str, dejaVu);
} else if (o instanceof Collection) {
appendCollection(o, str, dejaVu);
+ } else {
+ throw new IllegalArgumentException("was expecting a container, found " + oClass);
}
}
- private static void appendArray(final Object o, final StringBuilder str, Set<String> dejaVu,
+ private static void appendArray(
+ final Object o,
+ final StringBuilder str,
+ final Set<Object> dejaVu,
final Class<?> oClass) {
if (oClass == byte[].class) {
str.append(Arrays.toString((byte[]) o));
@@ -510,15 +539,13 @@
} else if (oClass == char[].class) {
str.append(Arrays.toString((char[]) o));
} else {
- if (dejaVu == null) {
- dejaVu = new HashSet<>();
- }
// special handling of container Object[]
- final String id = identityToString(o);
- if (dejaVu.contains(id)) {
+ final Set<Object> effectiveDejaVu = getOrCreateDejaVu(dejaVu);
+ final boolean seen = !effectiveDejaVu.add(o);
+ if (seen) {
+ final String id = identityToString(o);
str.append(RECURSION_PREFIX).append(id).append(RECURSION_SUFFIX);
} else {
- dejaVu.add(id);
final Object[] oArray = (Object[]) o;
str.append('[');
boolean first = true;
@@ -528,24 +555,26 @@
} else {
str.append(", ");
}
- recursiveDeepToString(current, str, new HashSet<>(dejaVu));
+ recursiveDeepToString(current, str, cloneDejaVu(effectiveDejaVu));
}
str.append(']');
}
- //str.append(Arrays.deepToString((Object[]) o));
}
}
- private static void appendMap(final Object o, final StringBuilder str, Set<String> dejaVu) {
- // special handling of container Map
- if (dejaVu == null) {
- dejaVu = new HashSet<>();
- }
- final String id = identityToString(o);
- if (dejaVu.contains(id)) {
+ /**
+ * Specialized handler for {@link Map}s.
+ */
+ private static void appendMap(
+ final Object o,
+ final StringBuilder str,
+ final Set<Object> dejaVu) {
+ final Set<Object> effectiveDejaVu = getOrCreateDejaVu(dejaVu);
+ final boolean seen = !effectiveDejaVu.add(o);
+ if (seen) {
+ final String id = identityToString(o);
str.append(RECURSION_PREFIX).append(id).append(RECURSION_SUFFIX);
} else {
- dejaVu.add(id);
final Map<?, ?> oMap = (Map<?, ?>) o;
str.append('{');
boolean isFirst = true;
@@ -558,24 +587,27 @@
}
final Object key = current.getKey();
final Object value = current.getValue();
- recursiveDeepToString(key, str, new HashSet<>(dejaVu));
+ recursiveDeepToString(key, str, cloneDejaVu(effectiveDejaVu));
str.append('=');
- recursiveDeepToString(value, str, new HashSet<>(dejaVu));
+ recursiveDeepToString(value, str, cloneDejaVu(effectiveDejaVu));
}
str.append('}');
}
}
- private static void appendCollection(final Object o, final StringBuilder str, Set<String> dejaVu) {
- // special handling of container Collection
- if (dejaVu == null) {
- dejaVu = new HashSet<>();
- }
- final String id = identityToString(o);
- if (dejaVu.contains(id)) {
+ /**
+ * Specialized handler for {@link Collection}s.
+ */
+ private static void appendCollection(
+ final Object o,
+ final StringBuilder str,
+ final Set<Object> dejaVu) {
+ final Set<Object> effectiveDejaVu = getOrCreateDejaVu(dejaVu);
+ final boolean seen = !effectiveDejaVu.add(o);
+ if (seen) {
+ final String id = identityToString(o);
str.append(RECURSION_PREFIX).append(id).append(RECURSION_SUFFIX);
} else {
- dejaVu.add(id);
final Collection<?> oCol = (Collection<?>) o;
str.append('[');
boolean isFirst = true;
@@ -585,12 +617,28 @@
} else {
str.append(", ");
}
- recursiveDeepToString(anOCol, str, new HashSet<>(dejaVu));
+ recursiveDeepToString(anOCol, str, cloneDejaVu(effectiveDejaVu));
}
str.append(']');
}
}
+ private static Set<Object> getOrCreateDejaVu(Set<Object> dejaVu) {
+ return dejaVu == null
+ ? createDejaVu()
+ : dejaVu;
+ }
+
+ private static Set<Object> createDejaVu() {
+ return Collections.newSetFromMap(new IdentityHashMap<>());
+ }
+
+ private static Set<Object> cloneDejaVu(Set<Object> dejaVu) {
+ Set<Object> clonedDejaVu = createDejaVu();
+ clonedDejaVu.addAll(dejaVu);
+ return clonedDejaVu;
+ }
+
private static void tryObjectToString(final Object o, final StringBuilder str) {
// it's just some other Object, we can only use toString().
try {
diff --git a/log4j-api/src/test/java/org/apache/logging/log4j/message/ParameterFormatterTest.java b/log4j-api/src/test/java/org/apache/logging/log4j/message/ParameterFormatterTest.java
index 92d4afb..ba793e3 100644
--- a/log4j-api/src/test/java/org/apache/logging/log4j/message/ParameterFormatterTest.java
+++ b/log4j-api/src/test/java/org/apache/logging/log4j/message/ParameterFormatterTest.java
@@ -19,17 +19,18 @@
import org.junit.jupiter.api.Test;
import java.util.ArrayList;
+import java.util.Collections;
import java.util.List;
-import static org.junit.jupiter.api.Assertions.*;
+import static org.junit.jupiter.api.Assertions.assertEquals;
/**
- * Tests ParameterFormatter.
+ * Tests {@link ParameterFormatter}.
*/
public class ParameterFormatterTest {
@Test
- public void testCountArgumentPlaceholders() throws Exception {
+ public void testCountArgumentPlaceholders() {
assertEquals(0, ParameterFormatter.countArgumentPlaceholders(""));
assertEquals(0, ParameterFormatter.countArgumentPlaceholders("aaa"));
assertEquals(0, ParameterFormatter.countArgumentPlaceholders("\\{}"));
@@ -169,9 +170,10 @@
}
@Test
- public void testDeepToString() throws Exception {
+ public void testDeepToString() {
final List<Object> list = new ArrayList<>();
list.add(1);
+ // noinspection CollectionAddedToSelf
list.add(list);
list.add(2);
final String actual = ParameterFormatter.deepToString(list);
@@ -180,13 +182,29 @@
}
@Test
- public void testIdentityToString() throws Exception {
+ public void testDeepToStringUsingNonRecursiveButConsequentObjects() {
+ final List<Object> list = new ArrayList<>();
+ final Object item = Collections.singletonList(0);
+ list.add(1);
+ list.add(item);
+ list.add(2);
+ list.add(item);
+ list.add(3);
+ final String actual = ParameterFormatter.deepToString(list);
+ final String expected = "[1, [0], 2, [0], 3]";
+ assertEquals(expected, actual);
+ }
+
+ @Test
+ public void testIdentityToString() {
final List<Object> list = new ArrayList<>();
list.add(1);
+ // noinspection CollectionAddedToSelf
list.add(list);
list.add(2);
final String actual = ParameterFormatter.identityToString(list);
final String expected = list.getClass().getName() + "@" + Integer.toHexString(System.identityHashCode(list));
assertEquals(expected, actual);
}
+
}
diff --git a/src/changes/changes.xml b/src/changes/changes.xml
index f36f1df..15faa0b 100644
--- a/src/changes/changes.xml
+++ b/src/changes/changes.xml
@@ -31,6 +31,9 @@
- "remove" - Removed
-->
<release version="3.0.0" date="2021-MM-DD" description="GA Release 3.0.0">
+ <action issue="LOG4J2-2948" dev="vy" type="fix">
+ Replace HashSet with IdentityHashMap in ParameterFormatter to detect cycles.
+ </action>
<action issue="LOG4J2-2624" dev="mattsicker" type="fix" due-to="Tim Perry">
Allow auto-shutdown of log4j in log4j-web to be turned off and provide a
ServletContextListener "Log4jShutdownOnContextDestroyedListener" to stop log4j.
