FreeAndNil commented on code in PR #4272:
URL: https://github.com/apache/logging-log4j2/pull/4272#discussion_r3911369298
##########
log4j-api/src/main/java/org/apache/logging/log4j/util/SortedArrayStringMap.java:
##########
@@ -513,18 +521,30 @@ private void readObject(final java.io.ObjectInputStream
s) throws IOException, C
if (mappings < 0) {
throw new InvalidObjectException("Illegal mappings count: " +
mappings);
}
+ if (mappings > capacity) {
+ throw new InvalidObjectException("Illegal mappings count: " +
mappings + " for capacity: " + capacity);
+ }
- // allocate the bucket array;
if (mappings > 0) {
- inflateTable(capacity);
+ // Do not trust the declared capacity: allocate a bounded amount
up front
+ // and resize as entries are actually read.
+ int allocated = Math.min(capacity, MAX_DESERIALIZATION_CAPACITY);
Review Comment:
**Security** — Would `mappings` work as the bound here instead of `capacity`?
`mappings` is validated as `<= capacity` just above and is the tighter of
the two. As it stands, a stream declaring `capacity = Integer.MAX_VALUE,
mappings = 1` still allocates two 131072-element arrays (~1 MiB) for a single
key, and `initFrom0` carries `threshold` into every later copy of the map. The
growth loop would still cover `mappings > 1 << 17`. Unless there is a reason to
prefer `capacity` that I am not seeing.
```suggestion
int allocated = Math.min(mappings, MAX_DESERIALIZATION_CAPACITY);
```
##########
log4j-api/src/main/java/org/apache/logging/log4j/util/SortedArrayStringMap.java:
##########
@@ -513,18 +521,30 @@ private void readObject(final java.io.ObjectInputStream
s) throws IOException, C
if (mappings < 0) {
throw new InvalidObjectException("Illegal mappings count: " +
mappings);
}
+ if (mappings > capacity) {
+ throw new InvalidObjectException("Illegal mappings count: " +
mappings + " for capacity: " + capacity);
+ }
- // allocate the bucket array;
if (mappings > 0) {
- inflateTable(capacity);
+ // Do not trust the declared capacity: allocate a bounded amount
up front
+ // and resize as entries are actually read.
+ int allocated = Math.min(capacity, MAX_DESERIALIZATION_CAPACITY);
+ String[] newKeys = new String[allocated];
+ Object[] newValues = new Object[allocated];
+ for (int i = 0; i < mappings; i++) {
+ if (i == allocated) {
+ allocated = (int) Math.min(capacity, 2L * allocated);
+ newKeys = Arrays.copyOf(newKeys, allocated);
+ newValues = Arrays.copyOf(newValues, allocated);
+ }
+ newKeys[i] = (String) s.readObject();
+ newValues[i] = SerializationUtil.readWrappedObject(s);
+ }
+ keys = newKeys;
+ values = newValues;
+ threshold = allocated;
} else {
- threshold = capacity;
- }
-
- // Read the keys and values, and put the mappings in the arrays
- for (int i = 0; i < mappings; i++) {
- keys[i] = (String) s.readObject();
- values[i] = SerializationUtil.readWrappedObject(s);
+ threshold = Math.min(capacity, MAX_DESERIALIZATION_CAPACITY);
Review Comment:
**Security** — The empty-map branch keeps the declared capacity, so the
first `putValue` allocates ~1 MiB.
`threshold` survives as 131072 and `inflateTable` acts on it at the first
insert, for a map holding one entry. An honestly written empty map declares
`capacity = 4`, so clamping this to `DEFAULT_INITIAL_CAPACITY` looks like it
would cost nothing.
##########
log4j-api-test/src/test/java/org/apache/logging/log4j/util/SortedArrayStringMapTest.java:
##########
@@ -119,6 +123,78 @@ void testSerializationOfNonSerializableValue() {
assertEquals(expected, copy);
}
+ /**
+ * Returns a copy of the serialized form with the declared capacity
replaced.
+ * <p>
+ * The capacity and mappings count follow the default field data as a
block-data record
+ * ({@code 0x77}, length {@code 0x08}), so the pair can be located by
its original values.
+ * </p>
+ */
+ private static byte[] patchCapacity(
+ final byte[] binary, final int capacity, final int mappings, final
int newCapacity) {
+ final ByteBuffer buffer = ByteBuffer.wrap(binary.clone());
+ for (int i = 0; i + 10 <= binary.length; i++) {
+ if (buffer.get(i) == 0x77
+ && buffer.get(i + 1) == 0x08
+ && buffer.getInt(i + 2) == capacity
+ && buffer.getInt(i + 6) == mappings) {
+ buffer.putInt(i + 2, newCapacity);
+ return buffer.array();
+ }
+ }
+ throw new AssertionError("Unable to locate the capacity field in the
serialized form");
+ }
+
+ @Test
+ void testDeserializationDoesNotPreallocateDeclaredCapacity() {
+ final SortedArrayStringMap original = new SortedArrayStringMap();
+ original.putValue("a", "avalue");
+ original.putValue("B", null);
+ original.putValue("3", "3value");
+
+ // A forged stream declaring a huge capacity must not force a huge
allocation.
+ final byte[] forged = patchCapacity(serialize(original), 4, 3,
Integer.MAX_VALUE);
+ final SortedArrayStringMap copy = deserialize(forged);
+ assertEquals(original, copy);
+ }
+
+ @Test
+ void testDeserializationDoesNotKeepDeclaredCapacityOfEmptyMap() {
+ final SortedArrayStringMap original = new SortedArrayStringMap();
+
+ // A forged threshold must not force a huge allocation on the first
`putValue` call.
+ final byte[] forged = patchCapacity(serialize(original), 4, 0,
Integer.MAX_VALUE);
+ final SortedArrayStringMap copy = deserialize(forged);
+ assertEquals(original, copy);
+ copy.putValue("a", "avalue");
+ assertEquals("avalue", copy.getValue("a"));
+ }
+
+ @Test
+ void testDeserializationRejectsMoreMappingsThanCapacity() throws Exception
{
+ final SortedArrayStringMap original = new SortedArrayStringMap();
+ original.putValue("a", "avalue");
+ original.putValue("B", null);
+ original.putValue("3", "3value");
+
+ final byte[] forged = patchCapacity(serialize(original), 4, 3, 2);
+ final ObjectInputStream ois = SerialUtil.getObjectInputStream(forged);
+ assertThrows(InvalidObjectException.class, ois::readObject);
+ }
+
+ @Test
+ void testDeserializationResizesBeyondInitialAllocation() {
+ final SortedArrayStringMap original = new SortedArrayStringMap();
+ // One entry more than the bounded initial allocation of the
deserialized arrays
+ final int count = (1 << 17) + 1;
Review Comment:
**Maintainability** — `count` repeats the value of
`MAX_DESERIALIZATION_CAPACITY` without being tied to it.
If the constant is raised later, `count` drops below it and this test
quietly stops exercising the growth branch while still passing. Deriving it
reflectively, as above, would keep the two in step.
##########
log4j-api/src/main/java/org/apache/logging/log4j/util/SortedArrayStringMap.java:
##########
@@ -57,6 +57,14 @@ public class SortedArrayStringMap implements
IndexedStringMap {
*/
private static final int DEFAULT_INITIAL_CAPACITY = 4;
+ /**
+ * The maximum number of entries pre-allocated during deserialization:
each array of this size occupies
+ * 1 MiB on a typical 64-bit JVM.
Review Comment:
**Maintainability** — Nit: with compressed oops an array of this size is 512
KiB, not 1 MiB.
1 MiB is the pair of arrays, or a single one with compressed oops disabled.
Only worth mentioning because the figure is what justifies the value of the
constant.
##########
log4j-api-test/src/test/java/org/apache/logging/log4j/util/SortedArrayStringMapTest.java:
##########
@@ -119,6 +123,78 @@ void testSerializationOfNonSerializableValue() {
assertEquals(expected, copy);
}
+ /**
+ * Returns a copy of the serialized form with the declared capacity
replaced.
+ * <p>
+ * The capacity and mappings count follow the default field data as a
block-data record
+ * ({@code 0x77}, length {@code 0x08}), so the pair can be located by
its original values.
+ * </p>
+ */
+ private static byte[] patchCapacity(
+ final byte[] binary, final int capacity, final int mappings, final
int newCapacity) {
+ final ByteBuffer buffer = ByteBuffer.wrap(binary.clone());
+ for (int i = 0; i + 10 <= binary.length; i++) {
+ if (buffer.get(i) == 0x77
+ && buffer.get(i + 1) == 0x08
+ && buffer.getInt(i + 2) == capacity
+ && buffer.getInt(i + 6) == mappings) {
+ buffer.putInt(i + 2, newCapacity);
+ return buffer.array();
+ }
+ }
+ throw new AssertionError("Unable to locate the capacity field in the
serialized form");
+ }
+
+ @Test
+ void testDeserializationDoesNotPreallocateDeclaredCapacity() {
+ final SortedArrayStringMap original = new SortedArrayStringMap();
+ original.putValue("a", "avalue");
+ original.putValue("B", null);
+ original.putValue("3", "3value");
+
+ // A forged stream declaring a huge capacity must not force a huge
allocation.
+ final byte[] forged = patchCapacity(serialize(original), 4, 3,
Integer.MAX_VALUE);
+ final SortedArrayStringMap copy = deserialize(forged);
+ assertEquals(original, copy);
Review Comment:
**Testability** — Both forged-capacity tests compare contents only, so they
do not pin the new bound.
They fail on the old code because `new String[Integer.MAX_VALUE]` throws, so
they would also pass with a much larger bound, or with a fix that
over-allocated. Asserting the length of `values` would pin it down - the class
already reads that field reflectively at lines 923 and 1012.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
