From e5753f3ecfe2682e45c0b2c875369368816dee13 Mon Sep 17 00:00:00 2001 From: Jack Smith Date: Wed, 23 Aug 2023 16:30:05 -0400 Subject: [PATCH 1/3] Prevent stackoverflow on hashCode by using a semaphore to prevent recursion --- .../jinjava/objects/collections/PyList.java | 17 ++++++ .../jinjava/objects/collections/PyMap.java | 18 ++++-- .../objects/collections/PyMapTest.java | 59 +++++++++++++++++++ 3 files changed, 89 insertions(+), 5 deletions(-) diff --git a/src/main/java/com/hubspot/jinjava/objects/collections/PyList.java b/src/main/java/com/hubspot/jinjava/objects/collections/PyList.java index 136ab8661..428a3cad5 100644 --- a/src/main/java/com/hubspot/jinjava/objects/collections/PyList.java +++ b/src/main/java/com/hubspot/jinjava/objects/collections/PyList.java @@ -7,8 +7,12 @@ import java.util.Collections; import java.util.List; import java.util.Objects; +import java.util.concurrent.Semaphore; public class PyList extends ForwardingList implements PyWrapper { + private final ThreadLocal semaphore = ThreadLocal.withInitial( + () -> new Semaphore(1) + ); private final List list; public PyList(List list) { @@ -99,4 +103,17 @@ IndexOutOfRangeException createOutOfRangeException(int index) { String.format("Index %d is out of range for list of size %d", index, list.size()) ); } + + @Override + public int hashCode() { + if (semaphore.get().tryAcquire()) { + try { + return super.hashCode(); + } finally { + semaphore.get().release(); + } + } else { + return Objects.hashCode(null); + } + } } diff --git a/src/main/java/com/hubspot/jinjava/objects/collections/PyMap.java b/src/main/java/com/hubspot/jinjava/objects/collections/PyMap.java index cf867bfed..9afca5037 100644 --- a/src/main/java/com/hubspot/jinjava/objects/collections/PyMap.java +++ b/src/main/java/com/hubspot/jinjava/objects/collections/PyMap.java @@ -3,9 +3,15 @@ import com.google.common.collect.ForwardingMap; import com.hubspot.jinjava.objects.PyWrapper; import java.util.Map; +import java.util.Objects; import java.util.Set; +import java.util.concurrent.Semaphore; public class PyMap extends ForwardingMap implements PyWrapper { + private final ThreadLocal semaphore = ThreadLocal.withInitial( + () -> new Semaphore(1) + ); + private final Map map; public PyMap(Map map) { @@ -61,12 +67,14 @@ public void putAll(Map m) { @Override public int hashCode() { - int h = 0; - for (Entry entry : map.entrySet()) { - if (entry.getValue() != map && entry.getValue() != this) { - h += entry.hashCode(); + if (semaphore.get().tryAcquire()) { + try { + return super.hashCode(); + } finally { + semaphore.get().release(); } + } else { + return Objects.hashCode(null); } - return h; } } diff --git a/src/test/java/com/hubspot/jinjava/objects/collections/PyMapTest.java b/src/test/java/com/hubspot/jinjava/objects/collections/PyMapTest.java index 3ec10fbe8..ea4cb8c15 100644 --- a/src/test/java/com/hubspot/jinjava/objects/collections/PyMapTest.java +++ b/src/test/java/com/hubspot/jinjava/objects/collections/PyMapTest.java @@ -3,6 +3,7 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; +import com.google.common.collect.ImmutableList; import com.hubspot.jinjava.BaseJinjavaTest; import com.hubspot.jinjava.Jinjava; import com.hubspot.jinjava.JinjavaConfig; @@ -10,6 +11,7 @@ import com.hubspot.jinjava.interpret.IndexOutOfRangeException; import com.hubspot.jinjava.interpret.RenderResult; import com.hubspot.jinjava.interpret.TemplateError; +import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; import org.junit.Test; @@ -367,4 +369,61 @@ public void itUpdatesKeysWithVariableName() { ) .isEqualTo("value2"); } + + @Test + public void itComputesHashCodeWhenContainedWithinItself() { + PyMap map = new PyMap(new HashMap<>()); + map.put("map1key1", "value1"); + + PyMap map2 = new PyMap(new HashMap<>()); + map2.put("map2key1", map); + + map.put("map1key2", map2); + + assertThat(map.hashCode()).isEqualTo(-413943561); + } + + @Test + public void itComputesHashCodeWhenContainedWithinItselfWithFurtherEntries() { + PyMap map = new PyMap(new HashMap<>()); + map.put("map1key1", "value1"); + + PyMap map2 = new PyMap(new HashMap<>()); + map2.put("map2key1", map); + + map.put("map1key2", map2); + + int originalHashCode = map.hashCode(); + map2.put("newKey", "newValue"); + int newHashCode = map.hashCode(); + assertThat(originalHashCode).isNotEqualTo(newHashCode); + } + + @Test + public void itComputesHashCodeWhenContainedWithinItselfInsideList() { + PyMap map = new PyMap(new HashMap<>()); + map.put("map1key1", "value1"); + + PyMap map2 = new PyMap(new HashMap<>()); + map2.put("map2key1", map); + + map.put("map1key2", new PyList(ImmutableList.of((map2)))); + + assertThat(map.hashCode()).isEqualTo(-413943561); + } + + @Test + public void itComputesHashCodeWithNullKeysAndValues() { + PyMap map = new PyMap(new HashMap<>()); + map.put(null, "value1"); + + PyMap map2 = new PyMap(new HashMap<>()); + map2.put("map2key1", map); + + PyList list = new PyList(new ArrayList<>()); + list.add(null); + map.put("map1key2", new PyList(list)); + + assertThat(map.hashCode()).isEqualTo(-687497624); + } } From 74a88a767f635fc06455cced65d6f043ad089857 Mon Sep 17 00:00:00 2001 From: Jack Smith Date: Wed, 23 Aug 2023 16:35:47 -0400 Subject: [PATCH 2/3] Add additional testing for PyList --- .../objects/collections/PyListTest.java | 35 +++++++++++++++++++ .../objects/collections/PyMapTest.java | 6 ++-- 2 files changed, 38 insertions(+), 3 deletions(-) diff --git a/src/test/java/com/hubspot/jinjava/objects/collections/PyListTest.java b/src/test/java/com/hubspot/jinjava/objects/collections/PyListTest.java index 5cb9cf67a..4352a0431 100644 --- a/src/test/java/com/hubspot/jinjava/objects/collections/PyListTest.java +++ b/src/test/java/com/hubspot/jinjava/objects/collections/PyListTest.java @@ -6,6 +6,7 @@ import com.hubspot.jinjava.interpret.IndexOutOfRangeException; import com.hubspot.jinjava.interpret.RenderResult; import com.hubspot.jinjava.interpret.TemplateError; +import java.util.ArrayList; import java.util.Collections; import org.junit.Test; @@ -261,4 +262,38 @@ public void itDisallowsAppendingSelf() { ) .isEqualTo("[1, 2]"); } + + @Test + public void itComputesHashCodeWhenListContainsItself() { + PyList list1 = new PyList(new ArrayList<>()); + PyList list2 = new PyList(new ArrayList<>()); + list1.add(list2); + int initialHashCode = list1.hashCode(); + list2.add(list1); + int hashCodeWithInfiniteRecursion = list1.hashCode(); + assertThat(initialHashCode).isNotEqualTo(hashCodeWithInfiniteRecursion); + assertThat(list1.hashCode()) + .isEqualTo(hashCodeWithInfiniteRecursion) + .describedAs("Hash code should be consistent on multiple calls"); + assertThat(list2.hashCode()) + .isEqualTo(list1.hashCode()) + .describedAs( + "The two lists are currently the same as they are both a list1 of a single infinitely recurring list" + ); + list1.add(123456); + assertThat(list2.hashCode()) + .isNotEqualTo(list1.hashCode()) + .describedAs( + "The two lists are no longer the same as list1 has 2 elements while list2 has one" + ); + PyList copy = list1.copy(); + assertThat(copy.hashCode()) + .isNotEqualTo(list1.hashCode()) + .describedAs( + "copy is not the same as list1 because it is a list of a list of recursion, whereas list1 is a list of recursion" + ); + assertThat(list1.copy().hashCode()) + .isEqualTo(copy.hashCode()) + .describedAs("All copies should have the same hash code"); + } } diff --git a/src/test/java/com/hubspot/jinjava/objects/collections/PyMapTest.java b/src/test/java/com/hubspot/jinjava/objects/collections/PyMapTest.java index ea4cb8c15..051208aeb 100644 --- a/src/test/java/com/hubspot/jinjava/objects/collections/PyMapTest.java +++ b/src/test/java/com/hubspot/jinjava/objects/collections/PyMapTest.java @@ -380,7 +380,7 @@ public void itComputesHashCodeWhenContainedWithinItself() { map.put("map1key2", map2); - assertThat(map.hashCode()).isEqualTo(-413943561); + assertThat(map.hashCode()).isNotEqualTo(0); } @Test @@ -409,7 +409,7 @@ public void itComputesHashCodeWhenContainedWithinItselfInsideList() { map.put("map1key2", new PyList(ImmutableList.of((map2)))); - assertThat(map.hashCode()).isEqualTo(-413943561); + assertThat(map.hashCode()).isNotEqualTo(0); } @Test @@ -424,6 +424,6 @@ public void itComputesHashCodeWithNullKeysAndValues() { list.add(null); map.put("map1key2", new PyList(list)); - assertThat(map.hashCode()).isEqualTo(-687497624); + assertThat(map.hashCode()).isNotEqualTo(0); } } From 96072092713486dfc06625bc9b216265b37f8b46 Mon Sep 17 00:00:00 2001 From: Jack Smith Date: Wed, 23 Aug 2023 17:18:36 -0400 Subject: [PATCH 3/3] Prefer performance over thread-safety for hashCode computation. Use a boolean rather than a ThreadLocal --- .../jinjava/objects/collections/PyList.java | 23 ++++++++++--------- .../jinjava/objects/collections/PyMap.java | 23 ++++++++++--------- 2 files changed, 24 insertions(+), 22 deletions(-) diff --git a/src/main/java/com/hubspot/jinjava/objects/collections/PyList.java b/src/main/java/com/hubspot/jinjava/objects/collections/PyList.java index 428a3cad5..e710bf316 100644 --- a/src/main/java/com/hubspot/jinjava/objects/collections/PyList.java +++ b/src/main/java/com/hubspot/jinjava/objects/collections/PyList.java @@ -7,12 +7,9 @@ import java.util.Collections; import java.util.List; import java.util.Objects; -import java.util.concurrent.Semaphore; public class PyList extends ForwardingList implements PyWrapper { - private final ThreadLocal semaphore = ThreadLocal.withInitial( - () -> new Semaphore(1) - ); + private boolean computingHashCode = false; private final List list; public PyList(List list) { @@ -104,16 +101,20 @@ IndexOutOfRangeException createOutOfRangeException(int index) { ); } + /** + * This is not thread-safe + * @return hashCode, preventing recursion + */ @Override public int hashCode() { - if (semaphore.get().tryAcquire()) { - try { - return super.hashCode(); - } finally { - semaphore.get().release(); - } - } else { + if (computingHashCode) { return Objects.hashCode(null); } + try { + computingHashCode = true; + return super.hashCode(); + } finally { + computingHashCode = false; + } } } diff --git a/src/main/java/com/hubspot/jinjava/objects/collections/PyMap.java b/src/main/java/com/hubspot/jinjava/objects/collections/PyMap.java index 9afca5037..96954f06e 100644 --- a/src/main/java/com/hubspot/jinjava/objects/collections/PyMap.java +++ b/src/main/java/com/hubspot/jinjava/objects/collections/PyMap.java @@ -5,12 +5,9 @@ import java.util.Map; import java.util.Objects; import java.util.Set; -import java.util.concurrent.Semaphore; public class PyMap extends ForwardingMap implements PyWrapper { - private final ThreadLocal semaphore = ThreadLocal.withInitial( - () -> new Semaphore(1) - ); + private boolean computingHashCode = false; private final Map map; @@ -65,16 +62,20 @@ public void putAll(Map m) { super.putAll(m); } + /** + * This is not thread-safe + * @return hashCode, preventing recursion + */ @Override public int hashCode() { - if (semaphore.get().tryAcquire()) { - try { - return super.hashCode(); - } finally { - semaphore.get().release(); - } - } else { + if (computingHashCode) { return Objects.hashCode(null); } + try { + computingHashCode = true; + return super.hashCode(); + } finally { + computingHashCode = false; + } } }