Skip to content

Commit 90a1c0b

Browse files
IcesourceIcesource
andauthored
Fixed generics stack leak that poisoned reused Kryo instances. (#1283)
An exception thrown while (de)serializing a field left entries on the generics stacks: ReflectField skipped its popGenericType, and FieldSerializer skipped popTypeVariables (both are outside a finally). Kryo.reset() did not clear these stacks, so the leaked state survived autoReset and poisoned a reused (thread-local or pooled) Kryo instance: a later, unrelated (de)serialization could fail with an ArrayIndexOutOfBoundsException. - Add Generics.reset(), called from Kryo.reset(), clearing both the genericTypes and the type-variable (arguments) stacks. It has a fast path that does nothing when the stacks are already empty, since it runs on every serialization. - Move popGenericType to a finally block in ReflectField read/write, the most likely source of serialization exceptions. Issue: #1281 Co-authored-by: Icesource <icesource-japan@outlook.com>
1 parent 54cfe4a commit 90a1c0b

6 files changed

Lines changed: 190 additions & 3 deletions

File tree

src/com/esotericsoftware/kryo/Kryo.java

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -953,6 +953,7 @@ public void reset () {
953953
depth = 0;
954954
if (graphContext != null) graphContext.clear(2048);
955955
classResolver.reset();
956+
generics.reset();
956957
if (references) {
957958
referenceResolver.reset();
958959
readObject = null;

src/com/esotericsoftware/kryo/serializers/ReflectField.java

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -86,7 +86,6 @@ public void write (Output output, Object object) {
8686
kryo.writeObject(output, value, serializer);
8787
}
8888
}
89-
kryo.getGenerics().popGenericType();
9089
} catch (IllegalAccessException ex) {
9190
throw new KryoException("Error accessing field: " + name + " (" + object.getClass().getName() + ")", ex);
9291
} catch (KryoException ex) {
@@ -102,6 +101,9 @@ public void write (Output output, Object object) {
102101
KryoException ex = new KryoException(t);
103102
ex.addTrace(name + " (" + object.getClass().getName() + ")");
104103
throw ex;
104+
} finally {
105+
// Pop in a finally so an exception thrown by the nested write does not leave the generics stack unbalanced.
106+
kryo.getGenerics().popGenericType();
105107
}
106108
}
107109

@@ -134,8 +136,6 @@ public void read (Input input, Object object) {
134136
else
135137
value = kryo.readObject(input, concreteType, serializer);
136138
}
137-
kryo.getGenerics().popGenericType();
138-
139139
set(object, value);
140140
} catch (IllegalAccessException ex) {
141141
throw new KryoException("Error accessing field: " + name + " (" + fieldSerializer.type.getName() + ")", ex);
@@ -146,6 +146,9 @@ public void read (Input input, Object object) {
146146
KryoException ex = new KryoException(t);
147147
ex.addTrace(name + " (" + fieldSerializer.type.getName() + ")");
148148
throw ex;
149+
} finally {
150+
// Pop in a finally so an exception thrown by the nested read does not leave the generics stack unbalanced.
151+
kryo.getGenerics().popGenericType();
149152
}
150153
}
151154

src/com/esotericsoftware/kryo/util/DefaultGenerics.java

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -156,6 +156,19 @@ public int getGenericTypesSize () {
156156
return genericTypesSize;
157157
}
158158

159+
@Override
160+
public void reset () {
161+
// Fast path: after a balanced (successful) serialization both stacks are already empty.
162+
if (genericTypesSize == 0 && argumentsSize == 0) return;
163+
// Slow path: a serializer threw before popping. Discard the leaked entries so the next serialization starts clean.
164+
for (int i = 0; i < genericTypesSize; i++)
165+
genericTypes[i] = null;
166+
genericTypesSize = 0;
167+
for (int i = 0; i < argumentsSize; i++)
168+
arguments[i] = null;
169+
argumentsSize = 0;
170+
}
171+
159172
public String toString () {
160173
StringBuilder buffer = new StringBuilder();
161174
for (int i = 0; i < argumentsSize; i += 2) {

src/com/esotericsoftware/kryo/util/Generics.java

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,8 @@
2121

2222
import static com.esotericsoftware.kryo.util.Util.*;
2323

24+
import com.esotericsoftware.kryo.Kryo;
25+
2426
import java.lang.reflect.Array;
2527
import java.lang.reflect.GenericArrayType;
2628
import java.lang.reflect.GenericDeclaration;
@@ -78,6 +80,14 @@ public interface Generics {
7880
/** Returns the number of generic types currently tracked */
7981
int getGenericTypesSize ();
8082

83+
/** Discards all tracked generic type information, returning to an empty state. This is called by {@link Kryo#reset()} so a
84+
* reused Kryo instance is not left with stale generics if a serializer throws an exception before a balancing
85+
* {@link #popGenericType()} or {@link #popTypeVariables(int)}. Implementations should be cheap when there is nothing to
86+
* discard, since this is called after every serialization and deserialization (when auto-reset is enabled). The default
87+
* implementation does nothing. */
88+
default void reset () {
89+
}
90+
8191
/** Stores the type parameters for a class and, for parameters passed to super classes, the corresponding super class type
8292
* parameters. */
8393
class GenericsHierarchy {

src/com/esotericsoftware/kryo/util/NoGenerics.java

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -71,4 +71,8 @@ public Class resolveTypeVariable (TypeVariable typeVariable) {
7171
public int getGenericTypesSize () {
7272
return 0;
7373
}
74+
75+
@Override
76+
public void reset () {
77+
}
7478
}
Lines changed: 156 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,156 @@
1+
/* Copyright (c) 2008-2025, Nathan Sweet
2+
* All rights reserved.
3+
*
4+
* Redistribution and use in source and binary forms, with or without modification, are permitted provided that the following
5+
* conditions are met:
6+
*
7+
* - Redistributions of source code must retain the above copyright notice, this list of conditions and the following disclaimer.
8+
* - Redistributions in binary form must reproduce the above copyright notice, this list of conditions and the following
9+
* disclaimer in the documentation and/or other materials provided with the distribution.
10+
* - Neither the name of Esoteric Software nor the names of its contributors may be used to endorse or promote products derived
11+
* from this software without specific prior written permission.
12+
*
13+
* THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING,
14+
* BUT NOT LIMITED TO, THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT
15+
* SHALL THE COPYRIGHT HOLDER OR CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, EXEMPLARY, OR CONSEQUENTIAL
16+
* DAMAGES (INCLUDING, BUT NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR PROFITS; OR BUSINESS
17+
* INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING
18+
* NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. */
19+
20+
package com.esotericsoftware.kryo.serializers;
21+
22+
import static org.junit.jupiter.api.Assertions.*;
23+
24+
import com.esotericsoftware.kryo.Kryo;
25+
import com.esotericsoftware.kryo.KryoException;
26+
import com.esotericsoftware.kryo.KryoTestCase;
27+
import com.esotericsoftware.kryo.Serializer;
28+
import com.esotericsoftware.kryo.io.Input;
29+
import com.esotericsoftware.kryo.io.Output;
30+
31+
import java.util.ArrayList;
32+
import java.util.HashMap;
33+
import java.util.List;
34+
import java.util.Map;
35+
36+
import org.junit.jupiter.api.Test;
37+
38+
/** Verifies that an exception thrown while (de)serializing a generic field does not leak the generics stack and poison a reused
39+
* Kryo instance.
40+
* <p>
41+
* See <a href="https://github.com/EsotericSoftware/kryo/issues/1281">#1281</a>: when a serializer threw between
42+
* {@link com.esotericsoftware.kryo.util.Generics#pushGenericType pushGenericType} and the balancing
43+
* {@link com.esotericsoftware.kryo.util.Generics#popGenericType popGenericType}, the leaked entry survived {@link Kryo#reset()}
44+
* and caused a later, unrelated deserialization to fail with an {@link ArrayIndexOutOfBoundsException}. */
45+
class GenericsResetTest extends KryoTestCase {
46+
47+
@Test
48+
void testGenericsStackIsResetAfterDeserializationException () {
49+
kryo.setReferences(false);
50+
kryo.setRegistrationRequired(false);
51+
kryo.register(Holder.class);
52+
kryo.register(Item.class);
53+
kryo.register(Payload.class, new ThrowOnReadSerializer());
54+
55+
// A Holder whose List<Item> element holds a Payload. Reading the Payload throws, after the generic type for the
56+
// List<Item> field was pushed, leaking it onto the generics stack.
57+
Holder holder = new Holder();
58+
holder.items = new ArrayList();
59+
Item item = new Item();
60+
item.payload = new Payload();
61+
holder.items.add(item);
62+
63+
Output output = new Output(512);
64+
kryo.writeClassAndObject(output, holder);
65+
byte[] failingBytes = output.toBytes();
66+
67+
// An unrelated, well-formed HashMap containing two non-empty nested maps. On a poisoned instance, reading the second
68+
// nested map mistakes the leaked List generic for a Map generic and indexes [1], throwing AIOOBE.
69+
byte[] validBytes = writeNestedMaps();
70+
71+
// The first deserialization fails inside the generic field.
72+
assertThrows(KryoException.class, () -> kryo.readClassAndObject(new Input(failingBytes)));
73+
74+
// Kryo.reset() must have discarded the leaked generic type.
75+
assertEquals(0, kryo.getGenerics().getGenericTypesSize());
76+
77+
// A subsequent unrelated deserialization on the same instance must succeed.
78+
Object result = kryo.readClassAndObject(new Input(validBytes));
79+
assertTrue(result instanceof Map);
80+
assertEquals(2, ((Map)result).size());
81+
}
82+
83+
@Test
84+
void testGenericsStackIsResetAfterSerializationException () {
85+
kryo.setReferences(false);
86+
kryo.setRegistrationRequired(false);
87+
kryo.register(Holder.class);
88+
kryo.register(Item.class);
89+
kryo.register(Payload.class, new ThrowOnWriteSerializer());
90+
91+
Holder holder = new Holder();
92+
holder.items = new ArrayList();
93+
Item item = new Item();
94+
item.payload = new Payload();
95+
holder.items.add(item);
96+
97+
// Writing the Payload throws, after the generic type for the List<Item> field was pushed.
98+
assertThrows(KryoException.class, () -> kryo.writeClassAndObject(new Output(512), holder));
99+
100+
// Kryo.reset() must have discarded the leaked generic type.
101+
assertEquals(0, kryo.getGenerics().getGenericTypesSize());
102+
103+
// A subsequent unrelated serialization/deserialization on the same instance must succeed.
104+
byte[] validBytes = writeNestedMaps();
105+
Object result = kryo.readClassAndObject(new Input(validBytes));
106+
assertTrue(result instanceof Map);
107+
assertEquals(2, ((Map)result).size());
108+
}
109+
110+
/** A bare HashMap with two non-empty nested maps, mirroring the payload that triggers the AIOOBE on a poisoned instance. */
111+
private byte[] writeNestedMaps () {
112+
Map<String, Object> map = new HashMap();
113+
Map<String, String> nested1 = new HashMap();
114+
nested1.put("a", "1");
115+
Map<String, String> nested2 = new HashMap();
116+
nested2.put("b", "2");
117+
map.put("k1", nested1);
118+
map.put("k2", nested2);
119+
120+
Output output = new Output(512);
121+
kryo.writeClassAndObject(output, map);
122+
return output.toBytes();
123+
}
124+
125+
public static class Holder {
126+
public List<Item> items;
127+
}
128+
129+
public static class Item {
130+
public Object payload;
131+
}
132+
133+
public static class Payload {
134+
}
135+
136+
/** Writes nothing and always throws on read, simulating any read-time failure (eg a missing class). */
137+
static class ThrowOnReadSerializer extends Serializer<Payload> {
138+
public void write (Kryo kryo, Output output, Payload object) {
139+
}
140+
141+
public Payload read (Kryo kryo, Input input, Class<? extends Payload> type) {
142+
throw new RuntimeException("simulated read failure");
143+
}
144+
}
145+
146+
/** Always throws on write, simulating any write-time failure. */
147+
static class ThrowOnWriteSerializer extends Serializer<Payload> {
148+
public void write (Kryo kryo, Output output, Payload object) {
149+
throw new RuntimeException("simulated write failure");
150+
}
151+
152+
public Payload read (Kryo kryo, Input input, Class<? extends Payload> type) {
153+
return new Payload();
154+
}
155+
}
156+
}

0 commit comments

Comments
 (0)