Skip to content

Commit 4603a36

Browse files
joschijoschi
authored
Prevent deep recursion in ConfigurationMetadata (dropwizard#3536)
Processing a configuration class with self-referencing fields could lead to an `OutOfMemoryError`. To prevent this, multiple safe guards have been added: - Reduce recursion depth from 100 to 10 levels - Check for loops in the parents of a property - Take `@JsonIgnore` annotation into account Fixes dropwizard#3528
1 parent af29dd4 commit 4603a36

File tree

3 files changed

+135
-40
lines changed

3 files changed

+135
-40
lines changed

‎dropwizard-configuration/pom.xml

+5
Original file line numberDiff line numberDiff line change
@@ -77,6 +77,11 @@
7777
<artifactId>assertj-core</artifactId>
7878
<scope>test</scope>
7979
</dependency>
80+
<dependency>
81+
<groupId>ch.qos.logback</groupId>
82+
<artifactId>logback-classic</artifactId>
83+
<scope>test</scope>
84+
</dependency>
8085
</dependencies>
8186

8287
<build>

‎dropwizard-configuration/src/main/java/io/dropwizard/configuration/ConfigurationMetadata.java

+27-9
Original file line numberDiff line numberDiff line change
@@ -1,15 +1,19 @@
11
package io.dropwizard.configuration;
22

3+
import com.fasterxml.jackson.annotation.JsonIgnore;
34
import com.fasterxml.jackson.databind.BeanProperty;
45
import com.fasterxml.jackson.databind.JavaType;
56
import com.fasterxml.jackson.databind.JsonMappingException;
67
import com.fasterxml.jackson.databind.ObjectMapper;
78
import com.fasterxml.jackson.databind.jsonFormatVisitors.JsonFormatVisitorWrapper;
89
import com.fasterxml.jackson.databind.jsonFormatVisitors.JsonObjectFormatVisitor;
910
import com.fasterxml.jackson.databind.jsontype.TypeDeserializer;
11+
1012
import java.util.HashMap;
13+
import java.util.HashSet;
1114
import java.util.Map;
1215
import java.util.Optional;
16+
import java.util.Set;
1317

1418
/**
1519
* A class to get metadata about the properties that are available in a configuration class. It can
@@ -35,34 +39,35 @@
3539
* }
3640
* }
3741
* </pre>
38-
*
42+
* <p>
3943
* This leads to the following entries:
4044
* <ul>
4145
* <li><pre>{@code name -> {SimpleType} "[simple type, class java.lang.String]"}</pre></li>
4246
* <li><pre>{@code names -> {CollectionType} "[collection type; class java.util.List, contains [simple type, class java.lang.String]]"}</pre></li>
4347
* </ul>
44-
*
48+
* <p>
4549
* Restrictions: The field-tree is only discovered correctly when no inheritance is present. It is
4650
* hard to discover the correct class, so this sticks to the defaultImpl that is provided.
4751
*/
4852
public class ConfigurationMetadata extends JsonFormatVisitorWrapper.Base {
4953

5054
// Just a safety option if someone uses recursive configuration classes
51-
private static final int MAX_DEPTH = 100;
55+
private static final int MAX_DEPTH = 10;
5256

5357
private final ObjectMapper mapper;
5458

5559
// Field is package-private to be visible for unit tests
5660
final Map<String, JavaType> fields = new HashMap<>();
5761

62+
private final Set<BeanProperty> parentProps = new HashSet<>();
5863
private String currentPrefix = "";
5964
private int currentDepth = 0;
6065

6166
/**
6267
* Create a metadata instance and
6368
*
6469
* @param mapper the {@link ObjectMapper} that is used to parse the configuration file
65-
* @param klass the target class of the configuration
70+
* @param klass the target class of the configuration
6671
*/
6772
public ConfigurationMetadata(ObjectMapper mapper, Class<?> klass) {
6873
this.mapper = mapper;
@@ -109,13 +114,22 @@ public JsonObjectFormatVisitor expectObjectFormat(JavaType type) throws JsonMapp
109114
@Override
110115
public void optionalProperty(BeanProperty prop) throws JsonMappingException {
111116
// don't run into an infinite loop with circular dependencies
112-
if (currentDepth > MAX_DEPTH) {
117+
if (currentDepth >= MAX_DEPTH) {
118+
return;
119+
}
120+
121+
// check if we already visited the same property
122+
if (parentProps.contains(prop)) {
123+
return;
124+
}
125+
126+
if (prop.getAnnotation(JsonIgnore.class) != null) {
113127
return;
114128
}
115129

116130
// build the complete field path
117131
String name = !currentPrefix.isEmpty() ? currentPrefix + "." + prop.getName()
118-
: prop.getName();
132+
: prop.getName();
119133

120134
// set state for the recursive traversal
121135
int oldFieldSize = fields.size();
@@ -135,17 +149,21 @@ public void optionalProperty(BeanProperty prop) throws JsonMappingException {
135149

136150
// get the type deserializer
137151
TypeDeserializer typeDeserializer =
138-
mapper.getDeserializationConfig().findTypeDeserializer(fieldType);
152+
mapper.getDeserializationConfig().findTypeDeserializer(fieldType);
139153

140154
// get the default impl if available
141155
Class<?> defaultImpl =
142-
typeDeserializer != null ? typeDeserializer.getDefaultImpl() : null;
156+
typeDeserializer != null ? typeDeserializer.getDefaultImpl() : null;
157+
158+
// remember current property
159+
parentProps.add(prop);
143160

144161
// visit the type of the property (or its defaultImpl).
145162
mapper.acceptJsonFormatVisitor(
146-
defaultImpl == null ? fieldType.getRawClass() : defaultImpl, thiss);
163+
defaultImpl == null ? fieldType.getRawClass() : defaultImpl, thiss);
147164

148165
// reset state after the recursive traversal
166+
parentProps.remove(prop);
149167
currentDepth--;
150168
currentPrefix = oldPrefix;
151169

‎dropwizard-configuration/src/test/java/io/dropwizard/configuration/ConfigurationMetadataTest.java

+103-31
Original file line numberDiff line numberDiff line change
@@ -1,18 +1,23 @@
11
package io.dropwizard.configuration;
22

3-
import static org.assertj.core.api.Assertions.assertThat;
4-
3+
import com.fasterxml.jackson.annotation.JsonIgnore;
54
import com.fasterxml.jackson.annotation.JsonProperty;
65
import com.fasterxml.jackson.annotation.JsonTypeInfo;
6+
import com.fasterxml.jackson.databind.ObjectMapper;
77
import io.dropwizard.jackson.Jackson;
8+
import org.junit.jupiter.api.Test;
9+
import org.junit.jupiter.params.ParameterizedTest;
10+
import org.junit.jupiter.params.provider.Arguments;
11+
import org.junit.jupiter.params.provider.MethodSource;
12+
813
import java.util.ArrayList;
914
import java.util.Collections;
1015
import java.util.List;
1116
import java.util.Set;
1217
import java.util.stream.Stream;
13-
import org.junit.jupiter.params.ParameterizedTest;
14-
import org.junit.jupiter.params.provider.Arguments;
15-
import org.junit.jupiter.params.provider.MethodSource;
18+
19+
import static org.assertj.core.api.Assertions.assertThat;
20+
import static org.assertj.core.api.Assertions.assertThatNoException;
1621

1722
class ConfigurationMetadataTest {
1823

@@ -60,7 +65,7 @@ public interface ExampleInterfaceWithDefaultImpl {
6065

6166
@SuppressWarnings("UnusedDeclaration")
6267
public static class DefaultExampleInterface implements ExampleInterface,
63-
ExampleInterfaceWithDefaultImpl {
68+
ExampleInterfaceWithDefaultImpl {
6469

6570
@JsonProperty
6671
private String[] array = new String[]{};
@@ -84,18 +89,64 @@ public Set<String> getSet() {
8489
}
8590
}
8691

92+
public static class Issue3528Configuration {
93+
@JsonProperty
94+
public ObjectMapper getMapper() {
95+
return new ObjectMapper();
96+
}
97+
}
98+
99+
public static class SelfReferencingConfiguration {
100+
private String str = "test";
101+
102+
@JsonProperty
103+
public SelfReferencingConfiguration getSelfReferencingConfiguration() {
104+
return new SelfReferencingConfiguration();
105+
}
106+
107+
@JsonProperty
108+
public String getStr() {
109+
return str;
110+
}
111+
}
112+
113+
public static class SelfReferencingIgnoredConfiguration {
114+
private String str = "test";
115+
private Long number = 42L;
116+
@JsonIgnore
117+
private SelfReferencingConfiguration ignored = new SelfReferencingConfiguration();
118+
119+
@JsonIgnore
120+
public SelfReferencingConfiguration getSelfReferencingConfiguration() {
121+
return new SelfReferencingConfiguration();
122+
}
123+
124+
@JsonProperty
125+
public String getStr() {
126+
return str;
127+
}
128+
129+
public Long getNumber() {
130+
return number;
131+
}
132+
133+
public SelfReferencingConfiguration getIgnored() {
134+
return ignored;
135+
}
136+
}
137+
87138
@ParameterizedTest
88139
@MethodSource("provideArgsForShouldDiscoverAllFields")
89140
public void shouldDiscoverAllFields(String name, boolean isPrimitive,
90-
boolean isCollectionOrArrayType,
91-
Class<?> klass) {
141+
boolean isCollectionOrArrayType,
142+
Class<?> klass) {
92143
final ConfigurationMetadata metadata = new ConfigurationMetadata(
93-
Jackson.newObjectMapper(), ExampleConfiguration.class);
144+
Jackson.newObjectMapper(), ExampleConfiguration.class);
94145

95146
assertThat(metadata.fields.get(name)).isNotNull().satisfies((f) -> {
96147
assertThat(f.isPrimitive()).isEqualTo(isPrimitive);
97148
assertThat(f.isCollectionLikeType() || f.isArrayType())
98-
.isEqualTo(isCollectionOrArrayType);
149+
.isEqualTo(isCollectionOrArrayType);
99150

100151
if (isCollectionOrArrayType) {
101152
assertThat(f.getContentType().isTypeOrSubTypeOf(klass)).isTrue();
@@ -107,40 +158,61 @@ public void shouldDiscoverAllFields(String name, boolean isPrimitive,
107158

108159
private static Stream<Arguments> provideArgsForShouldDiscoverAllFields() {
109160
return Stream.of(
110-
Arguments.of("port", true, false, Integer.TYPE),
111-
Arguments.of("example", false, false, ExampleInterface.class),
112-
Arguments.of("exampleWithDefault.array", false, true, String.class),
113-
Arguments.of("exampleWithDefault.list", false, true, String.class),
114-
Arguments.of("exampleWithDefault.set", false, true, String.class),
115-
Arguments.of("exampleWithDefaults[*].array", false, true, String.class),
116-
Arguments.of("exampleWithDefaults[*].list", false, true, String.class),
117-
Arguments.of("exampleWithDefaults[*].set", false, true, String.class)
161+
Arguments.of("port", true, false, Integer.TYPE),
162+
Arguments.of("example", false, false, ExampleInterface.class),
163+
Arguments.of("exampleWithDefault.array", false, true, String.class),
164+
Arguments.of("exampleWithDefault.list", false, true, String.class),
165+
Arguments.of("exampleWithDefault.set", false, true, String.class),
166+
Arguments.of("exampleWithDefaults[*].array", false, true, String.class),
167+
Arguments.of("exampleWithDefaults[*].list", false, true, String.class),
168+
Arguments.of("exampleWithDefaults[*].set", false, true, String.class)
118169
);
119170
}
120171

121172
@ParameterizedTest
122173
@MethodSource("provideArgsForIsCollectionOfStringsShouldWork")
123174
public void isCollectionOfStringsShouldWork(String name, boolean isCollectionOfStrings) {
124175
final ConfigurationMetadata metadata = new ConfigurationMetadata(
125-
Jackson.newObjectMapper(), ExampleConfiguration.class);
176+
Jackson.newObjectMapper(), ExampleConfiguration.class);
126177

127178
assertThat(metadata.isCollectionOfStrings(name)).isEqualTo(isCollectionOfStrings);
128179
}
129180

130-
131181
private static Stream<Arguments> provideArgsForIsCollectionOfStringsShouldWork() {
132182
return Stream.of(
133-
Arguments.of("doesnotexist", false),
134-
Arguments.of("port", false),
135-
Arguments.of("example.array", false),
136-
Arguments.of("example.list", false),
137-
Arguments.of("example.set", false),
138-
Arguments.of("exampleWithDefault.array", true),
139-
Arguments.of("exampleWithDefault.list", true),
140-
Arguments.of("exampleWithDefault.set", true),
141-
Arguments.of("exampleWithDefaults[0].array", true),
142-
Arguments.of("exampleWithDefaults[0].list", true),
143-
Arguments.of("exampleWithDefaults[0].set", true)
183+
Arguments.of("doesnotexist", false),
184+
Arguments.of("port", false),
185+
Arguments.of("example.array", false),
186+
Arguments.of("example.list", false),
187+
Arguments.of("example.set", false),
188+
Arguments.of("exampleWithDefault.array", true),
189+
Arguments.of("exampleWithDefault.list", true),
190+
Arguments.of("exampleWithDefault.set", true),
191+
Arguments.of("exampleWithDefaults[0].array", true),
192+
Arguments.of("exampleWithDefaults[0].list", true),
193+
Arguments.of("exampleWithDefaults[0].set", true)
144194
);
145195
}
196+
197+
@Test
198+
void issue3528ShouldNotProduceOutOfMemoryError() {
199+
assertThatNoException().isThrownBy(
200+
() -> new ConfigurationMetadata(Jackson.newObjectMapper(), Issue3528Configuration.class));
201+
}
202+
203+
@Test
204+
void fieldsAnnotatedWithJsonIgnoreShouldBeIgnored() {
205+
final ConfigurationMetadata metadata =
206+
new ConfigurationMetadata(Jackson.newObjectMapper(), SelfReferencingIgnoredConfiguration.class);
207+
208+
assertThat(metadata.fields).containsOnlyKeys("str", "number");
209+
}
210+
211+
@Test
212+
void selfReferencingConfigurationShouldNotLoop() {
213+
final ConfigurationMetadata metadata =
214+
new ConfigurationMetadata(Jackson.newObjectMapper(), SelfReferencingConfiguration.class);
215+
216+
assertThat(metadata.fields).containsOnlyKeys("selfReferencingConfiguration.str", "str");
217+
}
146218
}

0 commit comments

Comments
 (0)