diff --git a/src/com/esotericsoftware/kryo/Kryo.java b/src/com/esotericsoftware/kryo/Kryo.java index 4fc8aa85f..66e6777ad 100644 --- a/src/com/esotericsoftware/kryo/Kryo.java +++ b/src/com/esotericsoftware/kryo/Kryo.java @@ -953,6 +953,7 @@ public void reset () { depth = 0; if (graphContext != null) graphContext.clear(2048); classResolver.reset(); + generics.reset(); if (references) { referenceResolver.reset(); readObject = null; diff --git a/src/com/esotericsoftware/kryo/serializers/ReflectField.java b/src/com/esotericsoftware/kryo/serializers/ReflectField.java index b98f5e750..13d574d52 100644 --- a/src/com/esotericsoftware/kryo/serializers/ReflectField.java +++ b/src/com/esotericsoftware/kryo/serializers/ReflectField.java @@ -86,7 +86,6 @@ public void write (Output output, Object object) { kryo.writeObject(output, value, serializer); } } - kryo.getGenerics().popGenericType(); } catch (IllegalAccessException ex) { throw new KryoException("Error accessing field: " + name + " (" + object.getClass().getName() + ")", ex); } catch (KryoException ex) { @@ -102,6 +101,9 @@ public void write (Output output, Object object) { KryoException ex = new KryoException(t); ex.addTrace(name + " (" + object.getClass().getName() + ")"); throw ex; + } finally { + // Pop in a finally so an exception thrown by the nested write does not leave the generics stack unbalanced. + kryo.getGenerics().popGenericType(); } } @@ -134,8 +136,6 @@ public void read (Input input, Object object) { else value = kryo.readObject(input, concreteType, serializer); } - kryo.getGenerics().popGenericType(); - set(object, value); } catch (IllegalAccessException ex) { throw new KryoException("Error accessing field: " + name + " (" + fieldSerializer.type.getName() + ")", ex); @@ -146,6 +146,9 @@ public void read (Input input, Object object) { KryoException ex = new KryoException(t); ex.addTrace(name + " (" + fieldSerializer.type.getName() + ")"); throw ex; + } finally { + // Pop in a finally so an exception thrown by the nested read does not leave the generics stack unbalanced. + kryo.getGenerics().popGenericType(); } } diff --git a/src/com/esotericsoftware/kryo/util/DefaultGenerics.java b/src/com/esotericsoftware/kryo/util/DefaultGenerics.java index 105432100..cd1c19e9c 100644 --- a/src/com/esotericsoftware/kryo/util/DefaultGenerics.java +++ b/src/com/esotericsoftware/kryo/util/DefaultGenerics.java @@ -156,6 +156,19 @@ public int getGenericTypesSize () { return genericTypesSize; } + @Override + public void reset () { + // Fast path: after a balanced (successful) serialization both stacks are already empty. + if (genericTypesSize == 0 && argumentsSize == 0) return; + // Slow path: a serializer threw before popping. Discard the leaked entries so the next serialization starts clean. + for (int i = 0; i < genericTypesSize; i++) + genericTypes[i] = null; + genericTypesSize = 0; + for (int i = 0; i < argumentsSize; i++) + arguments[i] = null; + argumentsSize = 0; + } + public String toString () { StringBuilder buffer = new StringBuilder(); for (int i = 0; i < argumentsSize; i += 2) { diff --git a/src/com/esotericsoftware/kryo/util/Generics.java b/src/com/esotericsoftware/kryo/util/Generics.java index fa04c4ee3..31d396bf5 100644 --- a/src/com/esotericsoftware/kryo/util/Generics.java +++ b/src/com/esotericsoftware/kryo/util/Generics.java @@ -21,6 +21,8 @@ import static com.esotericsoftware.kryo.util.Util.*; +import com.esotericsoftware.kryo.Kryo; + import java.lang.reflect.Array; import java.lang.reflect.GenericArrayType; import java.lang.reflect.GenericDeclaration; @@ -78,6 +80,14 @@ public interface Generics { /** Returns the number of generic types currently tracked */ int getGenericTypesSize (); + /** Discards all tracked generic type information, returning to an empty state. This is called by {@link Kryo#reset()} so a + * reused Kryo instance is not left with stale generics if a serializer throws an exception before a balancing + * {@link #popGenericType()} or {@link #popTypeVariables(int)}. Implementations should be cheap when there is nothing to + * discard, since this is called after every serialization and deserialization (when auto-reset is enabled). The default + * implementation does nothing. */ + default void reset () { + } + /** Stores the type parameters for a class and, for parameters passed to super classes, the corresponding super class type * parameters. */ class GenericsHierarchy { diff --git a/src/com/esotericsoftware/kryo/util/NoGenerics.java b/src/com/esotericsoftware/kryo/util/NoGenerics.java index 689875b92..d40b1df57 100644 --- a/src/com/esotericsoftware/kryo/util/NoGenerics.java +++ b/src/com/esotericsoftware/kryo/util/NoGenerics.java @@ -71,4 +71,8 @@ public Class resolveTypeVariable (TypeVariable typeVariable) { public int getGenericTypesSize () { return 0; } + + @Override + public void reset () { + } } diff --git a/test/com/esotericsoftware/kryo/serializers/GenericsResetTest.java b/test/com/esotericsoftware/kryo/serializers/GenericsResetTest.java new file mode 100644 index 000000000..9db2ad2c8 --- /dev/null +++ b/test/com/esotericsoftware/kryo/serializers/GenericsResetTest.java @@ -0,0 +1,156 @@ +/* Copyright (c) 2008-2025, Nathan Sweet + * All rights reserved. + * + * Redistribution and use in source and binary forms, with or without modification, are permitted provided that the following + * conditions are met: + * + * - Redistributions of source code must retain the above copyright notice, this list of conditions and the following disclaimer. + * - Redistributions in binary form must reproduce the above copyright notice, this list of conditions and the following + * disclaimer in the documentation and/or other materials provided with the distribution. + * - Neither the name of Esoteric Software nor the names of its contributors may be used to endorse or promote products derived + * from this software without specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, + * BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT + * SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL + * DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS + * INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING + * NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. */ + +package com.esotericsoftware.kryo.serializers; + +import static org.junit.jupiter.api.Assertions.*; + +import com.esotericsoftware.kryo.Kryo; +import com.esotericsoftware.kryo.KryoException; +import com.esotericsoftware.kryo.KryoTestCase; +import com.esotericsoftware.kryo.Serializer; +import com.esotericsoftware.kryo.io.Input; +import com.esotericsoftware.kryo.io.Output; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import org.junit.jupiter.api.Test; + +/** Verifies that an exception thrown while (de)serializing a generic field does not leak the generics stack and poison a reused + * Kryo instance. + *

+ * See #1281: when a serializer threw between + * {@link com.esotericsoftware.kryo.util.Generics#pushGenericType pushGenericType} and the balancing + * {@link com.esotericsoftware.kryo.util.Generics#popGenericType popGenericType}, the leaked entry survived {@link Kryo#reset()} + * and caused a later, unrelated deserialization to fail with an {@link ArrayIndexOutOfBoundsException}. */ +class GenericsResetTest extends KryoTestCase { + + @Test + void testGenericsStackIsResetAfterDeserializationException () { + kryo.setReferences(false); + kryo.setRegistrationRequired(false); + kryo.register(Holder.class); + kryo.register(Item.class); + kryo.register(Payload.class, new ThrowOnReadSerializer()); + + // A Holder whose List element holds a Payload. Reading the Payload throws, after the generic type for the + // List field was pushed, leaking it onto the generics stack. + Holder holder = new Holder(); + holder.items = new ArrayList(); + Item item = new Item(); + item.payload = new Payload(); + holder.items.add(item); + + Output output = new Output(512); + kryo.writeClassAndObject(output, holder); + byte[] failingBytes = output.toBytes(); + + // An unrelated, well-formed HashMap containing two non-empty nested maps. On a poisoned instance, reading the second + // nested map mistakes the leaked List generic for a Map generic and indexes [1], throwing AIOOBE. + byte[] validBytes = writeNestedMaps(); + + // The first deserialization fails inside the generic field. + assertThrows(KryoException.class, () -> kryo.readClassAndObject(new Input(failingBytes))); + + // Kryo.reset() must have discarded the leaked generic type. + assertEquals(0, kryo.getGenerics().getGenericTypesSize()); + + // A subsequent unrelated deserialization on the same instance must succeed. + Object result = kryo.readClassAndObject(new Input(validBytes)); + assertTrue(result instanceof Map); + assertEquals(2, ((Map)result).size()); + } + + @Test + void testGenericsStackIsResetAfterSerializationException () { + kryo.setReferences(false); + kryo.setRegistrationRequired(false); + kryo.register(Holder.class); + kryo.register(Item.class); + kryo.register(Payload.class, new ThrowOnWriteSerializer()); + + Holder holder = new Holder(); + holder.items = new ArrayList(); + Item item = new Item(); + item.payload = new Payload(); + holder.items.add(item); + + // Writing the Payload throws, after the generic type for the List field was pushed. + assertThrows(KryoException.class, () -> kryo.writeClassAndObject(new Output(512), holder)); + + // Kryo.reset() must have discarded the leaked generic type. + assertEquals(0, kryo.getGenerics().getGenericTypesSize()); + + // A subsequent unrelated serialization/deserialization on the same instance must succeed. + byte[] validBytes = writeNestedMaps(); + Object result = kryo.readClassAndObject(new Input(validBytes)); + assertTrue(result instanceof Map); + assertEquals(2, ((Map)result).size()); + } + + /** A bare HashMap with two non-empty nested maps, mirroring the payload that triggers the AIOOBE on a poisoned instance. */ + private byte[] writeNestedMaps () { + Map map = new HashMap(); + Map nested1 = new HashMap(); + nested1.put("a", "1"); + Map nested2 = new HashMap(); + nested2.put("b", "2"); + map.put("k1", nested1); + map.put("k2", nested2); + + Output output = new Output(512); + kryo.writeClassAndObject(output, map); + return output.toBytes(); + } + + public static class Holder { + public List items; + } + + public static class Item { + public Object payload; + } + + public static class Payload { + } + + /** Writes nothing and always throws on read, simulating any read-time failure (eg a missing class). */ + static class ThrowOnReadSerializer extends Serializer { + public void write (Kryo kryo, Output output, Payload object) { + } + + public Payload read (Kryo kryo, Input input, Class type) { + throw new RuntimeException("simulated read failure"); + } + } + + /** Always throws on write, simulating any write-time failure. */ + static class ThrowOnWriteSerializer extends Serializer { + public void write (Kryo kryo, Output output, Payload object) { + throw new RuntimeException("simulated write failure"); + } + + public Payload read (Kryo kryo, Input input, Class type) { + return new Payload(); + } + } +}