Skip to content

Commit

Permalink
Limit to 31 levels of nested structure.
Browse files Browse the repository at this point in the history
With the implicit enclosing document this is 32 levels total. The limit
is arbitrary but sufficient - deeper limits yield code that fails with
StackOverflowExceptions during the depth-first traversal.

We were previously broken in JsonWriter on this - the code we added to
support path building was accessing the top of the stack before the
stack had been resized, causing a crash. Because this crash has existed
forever without much outcry we know the limit is likely sufficient for
our existing users.

Closes: #189
  • Loading branch information
swankjesse committed Oct 15, 2016
1 parent fce8fc6 commit d8743ee
Show file tree
Hide file tree
Showing 8 changed files with 157 additions and 51 deletions.
Expand Up @@ -57,14 +57,17 @@ final class BufferedSinkJsonWriter extends JsonWriter {
/** The output data, containing at most one top-level array or object. */
private final BufferedSink sink;

private int[] stack = new int[32];
// The nesting stack. Using a manual array rather than an ArrayList saves 20%. This stack permits
// up to 32 levels of nesting including the top-level document. Deeper nesting is prone to trigger
// StackOverflowErrors.
private final int[] stack = new int[32];
private int stackSize = 0;
{
push(EMPTY_DOCUMENT);
}

private String[] pathNames = new String[32];
private int[] pathIndices = new int[32];
private final String[] pathNames = new String[32];
private final int[] pathIndices = new int[32];

/**
* A string containing a full set of spaces for a single level of
Expand Down Expand Up @@ -143,8 +146,8 @@ final class BufferedSinkJsonWriter extends JsonWriter {
*/
private JsonWriter open(int empty, String openBracket) throws IOException {
beforeValue();
pathIndices[stackSize] = 0;
push(empty);
pathIndices[stackSize - 1] = 0;
sink.writeUtf8(openBracket);
return this;
}
Expand Down Expand Up @@ -175,9 +178,7 @@ private JsonWriter close(int empty, int nonempty, String closeBracket)

private void push(int newTop) {
if (stackSize == stack.length) {
int[] newStack = new int[stackSize * 2];
System.arraycopy(stack, 0, newStack, 0, stackSize);
stack = newStack;
throw new JsonDataException("Nesting too deep at " + getPath() + ": circular reference?");
}
stack[stackSize++] = newTop;
}
Expand Down
Expand Up @@ -92,17 +92,17 @@ final class BufferedSourceJsonReader extends JsonReader {
*/
private String peekedString;

/*
* The nesting stack. Using a manual array rather than an ArrayList saves 20%.
*/
private int[] stack = new int[32];
// The nesting stack. Using a manual array rather than an ArrayList saves 20%. This stack permits
// up to 32 levels of nesting including the top-level document. Deeper nesting is prone to trigger
// StackOverflowErrors.
private final int[] stack = new int[32];
private int stackSize = 0;
{
stack[stackSize++] = JsonScope.EMPTY_DOCUMENT;
}

private String[] pathNames = new String[32];
private int[] pathIndices = new int[32];
private final String[] pathNames = new String[32];
private final int[] pathIndices = new int[32];

BufferedSourceJsonReader(BufferedSource source) {
if (source == null) {
Expand Down Expand Up @@ -901,15 +901,7 @@ private void skipUnquotedValue() throws IOException {

private void push(int newTop) {
if (stackSize == stack.length) {
int[] newStack = new int[stackSize * 2];
int[] newPathIndices = new int[stackSize * 2];
String[] newPathNames = new String[stackSize * 2];
System.arraycopy(stack, 0, newStack, 0, stackSize);
System.arraycopy(pathIndices, 0, newPathIndices, 0, stackSize);
System.arraycopy(pathNames, 0, newPathNames, 0, stackSize);
stack = newStack;
pathIndices = newPathIndices;
pathNames = newPathNames;
throw new JsonDataException("Nesting too deep at " + getPath());
}
stack[stackSize++] = newTop;
}
Expand Down
4 changes: 4 additions & 0 deletions moshi/src/main/java/com/squareup/moshi/JsonDataException.java
Expand Up @@ -22,6 +22,10 @@
*
* <p>Exceptions of this type should be fixed by either changing the application code to accept
* the unexpected JSON, or by changing the JSON to conform to the application's expectations.
*
* <p>This exception may also be triggered if a document's nesting exceeds 31 levels. This depth is
* sufficient for all practical applications, but shallow enough to avoid uglier failures like
* {@link StackOverflowError}.
*/
public final class JsonDataException extends RuntimeException {
public JsonDataException() {
Expand Down
2 changes: 1 addition & 1 deletion moshi/src/main/java/com/squareup/moshi/MapJsonAdapter.java
Expand Up @@ -50,7 +50,7 @@ public MapJsonAdapter(Moshi moshi, Type keyType, Type valueType) {
writer.beginObject();
for (Map.Entry<K, V> entry : map.entrySet()) {
if (entry.getKey() == null) {
throw new JsonDataException("Map key is null at path " + writer.getPath());
throw new JsonDataException("Map key is null at " + writer.getPath());
}
writer.promoteNameToValue();
keyAdapter.toJson(writer, entry.getKey());
Expand Down
Expand Up @@ -419,31 +419,62 @@ public final class BufferedSinkJsonWriterTest {
@Test public void deepNestingArrays() throws IOException {
Buffer buffer = new Buffer();
JsonWriter jsonWriter = JsonWriter.of(buffer);
for (int i = 0; i < 20; i++) {
for (int i = 0; i < 31; i++) {
jsonWriter.beginArray();
}
for (int i = 0; i < 20; i++) {
for (int i = 0; i < 31; i++) {
jsonWriter.endArray();
}
assertThat(buffer.readUtf8()).isEqualTo("[[[[[[[[[[[[[[[[[[[[]]]]]]]]]]]]]]]]]]]]");
assertThat(buffer.readUtf8())
.isEqualTo("[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]");
}

@Test public void tooDeepNestingArrays() throws IOException {
Buffer buffer = new Buffer();
JsonWriter jsonWriter = JsonWriter.of(buffer);
for (int i = 0; i < 31; i++) {
jsonWriter.beginArray();
}
try {
jsonWriter.beginArray();
fail();
} catch (JsonDataException expected) {
assertThat(expected).hasMessage("Nesting too deep at $[0][0][0][0][0][0][0][0][0][0][0][0][0]"
+ "[0][0][0][0][0][0][0][0][0][0][0][0][0][0][0][0][0][0]: circular reference?");
}
}

@Test public void deepNestingObjects() throws IOException {
Buffer buffer = new Buffer();
JsonWriter jsonWriter = JsonWriter.of(buffer);
jsonWriter.beginObject();
for (int i = 0; i < 20; i++) {
jsonWriter.name("a");
for (int i = 0; i < 31; i++) {
jsonWriter.beginObject();
jsonWriter.name("a");
}
for (int i = 0; i < 20; i++) {
jsonWriter.value(true);
for (int i = 0; i < 31; i++) {
jsonWriter.endObject();
}
jsonWriter.endObject();
assertThat(buffer.readUtf8()).isEqualTo(
"{\"a\":{\"a\":{\"a\":{\"a\":{\"a\":{\"a\":{\"a\":{\"a\":{\"a\":{\"a\":"
+ "{\"a\":{\"a\":{\"a\":{\"a\":{\"a\":{\"a\":{\"a\":{\"a\":{\"a\":{\"a\":{"
+ "}}}}}}}}}}}}}}}}}}}}}");
assertThat(buffer.readUtf8()).isEqualTo(""
+ "{\"a\":{\"a\":{\"a\":{\"a\":{\"a\":{\"a\":{\"a\":{\"a\":{\"a\":{\"a\":{\"a\":{\"a\":"
+ "{\"a\":{\"a\":{\"a\":{\"a\":{\"a\":{\"a\":{\"a\":{\"a\":{\"a\":{\"a\":{\"a\":{\"a\":"
+ "{\"a\":{\"a\":{\"a\":{\"a\":{\"a\":{\"a\":{\"a\":true}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}}");
}

@Test public void tooDeepNestingObjects() throws IOException {
Buffer buffer = new Buffer();
JsonWriter jsonWriter = JsonWriter.of(buffer);
for (int i = 0; i < 31; i++) {
jsonWriter.beginObject();
jsonWriter.name("a");
}
try {
jsonWriter.beginObject();
fail();
} catch (JsonDataException expected) {
assertThat(expected).hasMessage("Nesting too deep at $.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a."
+ "a.a.a.a.a.a.a.a.a.a.a.a: circular reference?");
}
}

@Test public void repeatedName() throws IOException {
Expand Down
Expand Up @@ -1498,43 +1498,77 @@ private void testFailWithPosition(String message, String json) throws IOExceptio
}

@Test public void deeplyNestedArrays() throws IOException {
// this is nested 40 levels deep; Gson is tuned for nesting is 30 levels deep or fewer
JsonReader reader = newReader(
"[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]");
for (int i = 0; i < 40; i++) {
JsonReader reader = newReader("[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]");
for (int i = 0; i < 31; i++) {
reader.beginArray();
}
assertThat(reader.getPath()).isEqualTo(
"$[0][0][0][0][0][0][0][0][0][0][0][0][0][0][0][0][0][0][0][0][0][0][0][0][0][0]"
+ "[0][0][0][0][0][0][0][0][0][0][0][0][0][0]");
for (int i = 0; i < 40; i++) {
assertThat(reader.getPath()).isEqualTo("$[0][0][0][0][0][0][0][0][0][0][0][0][0][0][0][0][0][0]"
+ "[0][0][0][0][0][0][0][0][0][0][0][0][0]");
for (int i = 0; i < 31; i++) {
reader.endArray();
}
assertThat(reader.peek()).isEqualTo(JsonReader.Token.END_DOCUMENT);
}

@Test public void tooDeeplyNestedArrays() throws IOException {
JsonReader reader = newReader(
"[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[[]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]]");
for (int i = 0; i < 31; i++) {
reader.beginArray();
}
try {
reader.beginArray();
fail();
} catch (JsonDataException expected) {
assertThat(expected).hasMessage("Nesting too deep at $[0][0][0][0][0][0][0][0][0][0][0][0][0]"
+ "[0][0][0][0][0][0][0][0][0][0][0][0][0][0][0][0][0][0]");
}
}

@Test public void deeplyNestedObjects() throws IOException {
// Build a JSON document structured like {"a":{"a":{"a":{"a":true}}}}, but 40 levels deep
// Build a JSON document structured like {"a":{"a":{"a":{"a":true}}}}, but 31 levels deep.
String array = "{\"a\":%s}";
String json = "true";
for (int i = 0; i < 40; i++) {
for (int i = 0; i < 31; i++) {
json = String.format(array, json);
}

JsonReader reader = newReader(json);
for (int i = 0; i < 40; i++) {
for (int i = 0; i < 31; i++) {
reader.beginObject();
assertThat(reader.nextName()).isEqualTo("a");
}
assertThat(reader.getPath()).isEqualTo("$.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a"
+ ".a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a");
assertThat(reader.getPath())
.isEqualTo("$.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a");
assertThat(reader.nextBoolean()).isTrue();
for (int i = 0; i < 40; i++) {
for (int i = 0; i < 31; i++) {
reader.endObject();
}
assertThat(reader.peek()).isEqualTo(JsonReader.Token.END_DOCUMENT);
}

@Test public void tooDeeplyNestedObjects() throws IOException {
// Build a JSON document structured like {"a":{"a":{"a":{"a":true}}}}, but 31 levels deep.
String array = "{\"a\":%s}";
String json = "true";
for (int i = 0; i < 32; i++) {
json = String.format(array, json);
}

JsonReader reader = newReader(json);
for (int i = 0; i < 31; i++) {
reader.beginObject();
assertThat(reader.nextName()).isEqualTo("a");
}
try {
reader.beginObject();
fail();
} catch (JsonDataException expected) {
assertThat(expected).hasMessage(
"Nesting too deep at $.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a");
}
}

// http://code.google.com/p/google-gson/issues/detail?id=409
@Test public void stringEndingInSlash() throws IOException {
JsonReader reader = newReader("/");
Expand Down
Expand Up @@ -23,7 +23,6 @@
import java.util.Map;
import okio.Buffer;
import org.assertj.core.data.MapEntry;
import org.junit.Ignore;
import org.junit.Test;

import static com.squareup.moshi.TestUtil.newReader;
Expand Down Expand Up @@ -57,7 +56,7 @@ public final class MapJsonAdapterTest {
toJson(String.class, Boolean.class, map);
fail();
} catch (JsonDataException expected) {
assertThat(expected).hasMessage("Map key is null at path $.");
assertThat(expected).hasMessage("Map key is null at $.");
}
}

Expand Down
45 changes: 45 additions & 0 deletions moshi/src/test/java/com/squareup/moshi/MoshiTest.java
Expand Up @@ -23,13 +23,17 @@
import java.lang.reflect.Field;
import java.lang.reflect.Type;
import java.util.ArrayDeque;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collection;
import java.util.LinkedHashMap;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.Set;
import javax.crypto.KeyGenerator;
import org.junit.Assert;
import org.junit.Test;

import static com.squareup.moshi.TestUtil.newReader;
Expand Down Expand Up @@ -816,6 +820,47 @@ static class Message {
}
}

@Test public void referenceCyclesOnArrays() throws Exception {
Moshi moshi = new Moshi.Builder().build();
Map<String, Object> map = new LinkedHashMap<>();
map.put("a", map);
try {
moshi.adapter(Object.class).toJson(map);
fail();
} catch (JsonDataException expected) {
assertThat(expected).hasMessage("Nesting too deep at $.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a.a."
+ "a.a.a.a.a.a.a.a.a.a.a.a: circular reference?");
}
}

@Test public void referenceCyclesOnObjects() throws Exception {
Moshi moshi = new Moshi.Builder().build();
List<Object> list = new ArrayList<>();
list.add(list);
try {
moshi.adapter(Object.class).toJson(list);
fail();
} catch (JsonDataException expected) {
assertThat(expected).hasMessage("Nesting too deep at $[0][0][0][0][0][0][0][0][0][0][0][0][0]"
+ "[0][0][0][0][0][0][0][0][0][0][0][0][0][0][0][0][0][0]: circular reference?");
}
}

@Test public void referenceCyclesOnMixedTypes() throws Exception {
Moshi moshi = new Moshi.Builder().build();
List<Object> list = new ArrayList<>();
Map<String, Object> map = new LinkedHashMap<>();
list.add(map);
map.put("a", list);
try {
moshi.adapter(Object.class).toJson(list);
fail();
} catch (JsonDataException expected) {
assertThat(expected).hasMessage("Nesting too deep at $[0].a[0].a[0].a[0].a[0].a[0].a[0].a[0]."
+ "a[0].a[0].a[0].a[0].a[0].a[0].a[0].a[0]: circular reference?");
}
}

static class Pizza {
final int diameter;
final boolean extraCheese;
Expand Down

0 comments on commit d8743ee

Please sign in to comment.