Skip to content

Commit 6e6a080

Browse files
adriancoleadriancole
authored
Hardens json parser (openzipkin#1691)
[go-swagger](https://github.com/go-swagger/go-swagger) initializes some fields to null. This ensures null fields don't break parsing.
1 parent 3bd6743 commit 6e6a080

File tree

2 files changed

+92
-77
lines changed

2 files changed

+92
-77
lines changed

‎zipkin/src/main/java/zipkin/internal/Span2JsonCodec.java

Lines changed: 31 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -66,21 +66,29 @@ static final class SimpleSpanReader implements JsonReaderAdapter<Span2> {
6666
String nextName = reader.nextName();
6767
if (nextName.equals("traceId")) {
6868
builder.traceId(reader.nextString());
69-
} else if (nextName.equals("parentId") && reader.peek() != JsonToken.NULL) {
70-
builder.parentId(reader.nextString());
69+
continue;
7170
} else if (nextName.equals("id")) {
7271
builder.id(reader.nextString());
72+
continue;
73+
} else if (reader.peek() == JsonToken.NULL) {
74+
reader.skipValue();
75+
continue;
76+
}
77+
78+
// read any optional fields
79+
if (nextName.equals("parentId")) {
80+
builder.parentId(reader.nextString());
7381
} else if (nextName.equals("kind")) {
7482
builder.kind(Span2.Kind.valueOf(reader.nextString()));
75-
} else if (nextName.equals("name") && reader.peek() != JsonToken.NULL) {
83+
} else if (nextName.equals("name")) {
7684
builder.name(reader.nextString());
77-
} else if (nextName.equals("timestamp") && reader.peek() != JsonToken.NULL) {
85+
} else if (nextName.equals("timestamp")) {
7886
builder.timestamp(reader.nextLong());
79-
} else if (nextName.equals("duration") && reader.peek() != JsonToken.NULL) {
87+
} else if (nextName.equals("duration")) {
8088
builder.duration(reader.nextLong());
81-
} else if (nextName.equals("localEndpoint") && reader.peek() != JsonToken.NULL) {
89+
} else if (nextName.equals("localEndpoint")) {
8290
builder.localEndpoint(ENDPOINT_READER.fromJson(reader));
83-
} else if (nextName.equals("remoteEndpoint") && reader.peek() != JsonToken.NULL) {
91+
} else if (nextName.equals("remoteEndpoint")) {
8492
builder.remoteEndpoint(ENDPOINT_READER.fromJson(reader));
8593
} else if (nextName.equals("annotations")) {
8694
reader.beginArray();
@@ -98,8 +106,11 @@ static final class SimpleSpanReader implements JsonReaderAdapter<Span2> {
98106
reader.skipValue();
99107
}
100108
}
109+
if (timestamp == null || value == null) {
110+
throw new MalformedJsonException("Incomplete annotation at " + reader.getPath());
111+
}
101112
reader.endObject();
102-
if (timestamp != null && value != null) builder.addAnnotation(timestamp, value);
113+
builder.addAnnotation(timestamp, value);
103114
}
104115
reader.endArray();
105116
} else if (nextName.equals("tags")) {
@@ -112,9 +123,9 @@ static final class SimpleSpanReader implements JsonReaderAdapter<Span2> {
112123
builder.putTag(key, reader.nextString());
113124
}
114125
reader.endObject();
115-
} else if (nextName.equals("debug") && reader.peek() != JsonToken.NULL) {
126+
} else if (nextName.equals("debug")) {
116127
if (reader.nextBoolean()) builder.debug(true);
117-
} else if (nextName.equals("shared") && reader.peek() != JsonToken.NULL) {
128+
} else if (nextName.equals("shared")) {
118129
if (reader.nextBoolean()) builder.shared(true);
119130
} else {
120131
reader.skipValue();
@@ -132,19 +143,28 @@ static final class SimpleSpanReader implements JsonReaderAdapter<Span2> {
132143
static final JsonReaderAdapter<Endpoint> ENDPOINT_READER = reader -> {
133144
Endpoint.Builder result = Endpoint.builder().serviceName("");
134145
reader.beginObject();
146+
boolean readField = false;
135147
while (reader.hasNext()) {
136148
String nextName = reader.nextName();
137-
if (nextName.equals("serviceName") && reader.peek() != JsonToken.NULL) {
149+
if (reader.peek() == JsonToken.NULL) {
150+
reader.skipValue();
151+
continue;
152+
}
153+
if (nextName.equals("serviceName")) {
138154
result.serviceName(reader.nextString());
155+
readField = true;
139156
} else if (nextName.equals("ipv4") || nextName.equals("ipv6")) {
140157
result.parseIp(reader.nextString());
158+
readField = true;
141159
} else if (nextName.equals("port")) {
142160
result.port(reader.nextInt());
161+
readField = true;
143162
} else {
144163
reader.skipValue();
145164
}
146165
}
147166
reader.endObject();
167+
if (!readField) throw new MalformedJsonException("Empty endpoint at " + reader.getPath());
148168
return result.build();
149169
};
150170

‎zipkin/src/test/java/zipkin/internal/Span2JsonCodecTest.java

Lines changed: 61 additions & 66 deletions
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,7 @@ public class Span2JsonCodecTest {
106106
.isEqualTo(worstSpanInTheWorld);
107107
}
108108

109-
@Test public void niceErrorOnUppercaseTraceId() {
109+
@Test public void niceErrorOnUppercase_traceId() {
110110
thrown.expect(IllegalArgumentException.class);
111111
thrown.expectMessage(
112112
"48485A3953BB6124 should be a 1 to 32 character lower-hex string with no prefix");
@@ -120,21 +120,21 @@ public class Span2JsonCodecTest {
120120
codec.readSpan(json.getBytes(UTF_8));
121121
}
122122

123-
@Test public void decentErrorMessageOnEmptyInput_span() throws IOException {
123+
@Test public void niceErrorOnEmpty_inputSpan() throws IOException {
124124
thrown.expect(IllegalArgumentException.class);
125125
thrown.expectMessage("Empty input reading Span2");
126126

127127
codec.readSpan(new byte[0]);
128128
}
129129

130-
@Test public void decentErrorMessageOnEmptyInput_spans() throws IOException {
130+
@Test public void niceErrorOnEmpty_inputSpans() throws IOException {
131131
thrown.expect(IllegalArgumentException.class);
132132
thrown.expectMessage("Empty input reading List<Span2>");
133133

134134
codec.readSpans(new byte[0]);
135135
}
136136

137-
@Test public void decentErrorMessageOnMalformedInput_span() throws IOException {
137+
@Test public void niceErrorOnMalformed_inputSpan() throws IOException {
138138
thrown.expect(IllegalArgumentException.class);
139139
thrown.expectMessage("Malformed reading Span2 from ");
140140

@@ -144,7 +144,7 @@ public class Span2JsonCodecTest {
144144
/**
145145
* Particulary, thrift can mistake malformed content as a huge list. Let's not blow up.
146146
*/
147-
@Test public void decentErrorMessageOnMalformedInput_spans() throws IOException {
147+
@Test public void niceErrorOnMalformed_inputSpans() throws IOException {
148148
thrown.expect(IllegalArgumentException.class);
149149
thrown.expectMessage("Malformed reading List<Span2> from ");
150150

@@ -187,78 +187,71 @@ public class Span2JsonCodecTest {
187187
.traceIdHigh(Util.lowerHexToUnsignedLong("48485a3953bb6124")).build());
188188
}
189189

190-
@Test public void ignoreNull_parentId() {
190+
@Test public void ignoresNull_topLevelFields() {
191191
String json = "{\n"
192192
+ " \"traceId\": \"6b221d5bc9e6496c\",\n"
193-
+ " \"name\": \"get-traces\",\n"
193+
+ " \"parentId\": null,\n"
194194
+ " \"id\": \"6b221d5bc9e6496c\",\n"
195-
+ " \"parentId\": null\n"
196-
+ "}";
197-
198-
codec.readSpan(json.getBytes(UTF_8));
199-
}
200-
201-
@Test public void ignoreNull_timestamp() {
202-
String json = "{\n"
203-
+ " \"traceId\": \"6b221d5bc9e6496c\",\n"
204-
+ " \"name\": \"get-traces\",\n"
205-
+ " \"id\": \"6b221d5bc9e6496c\",\n"
206-
+ " \"timestamp\": null\n"
195+
+ " \"name\": null,\n"
196+
+ " \"timestamp\": null,\n"
197+
+ " \"duration\": null,\n"
198+
+ " \"localEndpoint\": null,\n"
199+
+ " \"remoteEndpoint\": null,\n"
200+
+ " \"annotations\": null,\n"
201+
+ " \"tags\": null,\n"
202+
+ " \"debug\": null,\n"
203+
+ " \"shared\": null\n"
207204
+ "}";
208205

209206
codec.readSpan(json.getBytes(UTF_8));
210207
}
211208

212-
@Test public void ignoreNull_duration() {
209+
@Test public void ignoresNull_endpoint_topLevelFields() {
213210
String json = "{\n"
214211
+ " \"traceId\": \"6b221d5bc9e6496c\",\n"
215212
+ " \"name\": \"get-traces\",\n"
216213
+ " \"id\": \"6b221d5bc9e6496c\",\n"
217-
+ " \"duration\": null\n"
214+
+ " \"localEndpoint\": {\n"
215+
+ " \"serviceName\": null,\n"
216+
+ " \"ipv4\": \"127.0.0.1\",\n"
217+
+ " \"ipv6\": null,\n"
218+
+ " \"port\": null\n"
219+
+ " }\n"
218220
+ "}";
219221

220-
codec.readSpan(json.getBytes(UTF_8));
222+
assertThat(codec.readSpan(json.getBytes(UTF_8)).localEndpoint())
223+
.isEqualTo(Endpoint.create("", 127 << 24 | 1));
221224
}
222225

223-
@Test public void ignoreNull_debug() {
224-
String json = "{\n"
225-
+ " \"traceId\": \"6b221d5bc9e6496c\",\n"
226-
+ " \"name\": \"get-traces\",\n"
227-
+ " \"id\": \"6b221d5bc9e6496c\",\n"
228-
+ " \"debug\": null\n"
229-
+ "}";
230-
231-
codec.readSpan(json.getBytes(UTF_8));
232-
}
226+
@Test public void niceErrorOnIncomplete_endpoint() {
227+
thrown.expect(IllegalArgumentException.class);
228+
thrown.expectMessage("Empty endpoint at $.localEndpoint reading Span2 from json");
233229

234-
@Test public void ignoreNull_shared() {
235230
String json = "{\n"
236231
+ " \"traceId\": \"6b221d5bc9e6496c\",\n"
237232
+ " \"name\": \"get-traces\",\n"
238233
+ " \"id\": \"6b221d5bc9e6496c\",\n"
239-
+ " \"shared\": null\n"
234+
+ " \"localEndpoint\": {\n"
235+
+ " \"serviceName\": null,\n"
236+
+ " \"ipv4\": null,\n"
237+
+ " \"ipv6\": null,\n"
238+
+ " \"port\": null\n"
239+
+ " }\n"
240240
+ "}";
241-
242241
codec.readSpan(json.getBytes(UTF_8));
243242
}
244243

245-
@Test public void ignoreNull_localEndpoint() {
246-
String json = "{\n"
247-
+ " \"traceId\": \"6b221d5bc9e6496c\",\n"
248-
+ " \"name\": \"get-traces\",\n"
249-
+ " \"id\": \"6b221d5bc9e6496c\",\n"
250-
+ " \"localEndpoint\": null\n"
251-
+ "}";
252-
253-
codec.readSpan(json.getBytes(UTF_8));
254-
}
244+
@Test public void niceErrorOnIncomplete_annotation() {
245+
thrown.expect(IllegalArgumentException.class);
246+
thrown.expectMessage("Incomplete annotation at $.annotations[0]");
255247

256-
@Test public void ignoreNull_remoteEndpoint() {
257248
String json = "{\n"
258249
+ " \"traceId\": \"6b221d5bc9e6496c\",\n"
259250
+ " \"name\": \"get-traces\",\n"
260251
+ " \"id\": \"6b221d5bc9e6496c\",\n"
261-
+ " \"remoteEndpoint\": null\n"
252+
+ " \"annotations\": [\n"
253+
+ " { \"timestamp\": 1472470996199000}\n"
254+
+ " ]\n"
262255
+ "}";
263256

264257
codec.readSpan(json.getBytes(UTF_8));
@@ -290,7 +283,7 @@ public class Span2JsonCodecTest {
290283
codec.readSpan(json.getBytes(UTF_8));
291284
}
292285

293-
@Test public void missingValue() {
286+
@Test public void niceErrorOnNull_tagValue() {
294287
thrown.expect(IllegalArgumentException.class);
295288
thrown.expectMessage("No value at $.tags.foo");
296289

@@ -306,56 +299,58 @@ public class Span2JsonCodecTest {
306299
codec.readSpan(json.getBytes(UTF_8));
307300
}
308301

309-
@Test public void readSpan_localEndpoint_noServiceName() {
302+
@Test public void niceErrorOnNull_annotationValue() {
303+
thrown.expect(IllegalArgumentException.class);
304+
thrown.expectMessage("$.annotations[0].value");
305+
310306
String json = "{\n"
311307
+ " \"traceId\": \"6b221d5bc9e6496c\",\n"
312308
+ " \"name\": \"get-traces\",\n"
313309
+ " \"id\": \"6b221d5bc9e6496c\",\n"
314-
+ " \"localEndpoint\": {\n"
315-
+ " \"ipv4\": \"127.0.0.1\"\n"
316-
+ " }\n"
310+
+ " \"annotations\": [\n"
311+
+ " { \"timestamp\": 1472470996199000, \"value\": NULL}\n"
312+
+ " ]\n"
317313
+ "}";
318314

319-
assertThat(codec.readSpan(json.getBytes(UTF_8)).localEndpoint())
320-
.isEqualTo(Endpoint.create("", 127 << 24 | 1));
315+
codec.readSpan(json.getBytes(UTF_8));
321316
}
322317

323-
@Test public void readSpan_localEndpoint_nullServiceName() {
318+
@Test public void niceErrorOnNull_annotationTimestamp() {
319+
thrown.expect(IllegalArgumentException.class);
320+
thrown.expectMessage("$.annotations[0].timestamp");
321+
324322
String json = "{\n"
325323
+ " \"traceId\": \"6b221d5bc9e6496c\",\n"
326324
+ " \"name\": \"get-traces\",\n"
327325
+ " \"id\": \"6b221d5bc9e6496c\",\n"
328-
+ " \"localEndpoint\": {\n"
329-
+ " \"serviceName\": null,\n"
330-
+ " \"ipv4\": \"127.0.0.1\"\n"
331-
+ " }\n"
326+
+ " \"annotations\": [\n"
327+
+ " { \"timestamp\": NULL, \"value\": \"foo\"}\n"
328+
+ " ]\n"
332329
+ "}";
333330

334-
assertThat(codec.readSpan(json.getBytes(UTF_8)).localEndpoint())
335-
.isEqualTo(Endpoint.create("", 127 << 24 | 1));
331+
codec.readSpan(json.getBytes(UTF_8));
336332
}
337333

338-
@Test public void readSpan_remoteEndpoint_noServiceName() {
334+
@Test public void readSpan_localEndpoint_noServiceName() {
339335
String json = "{\n"
340336
+ " \"traceId\": \"6b221d5bc9e6496c\",\n"
341337
+ " \"name\": \"get-traces\",\n"
342338
+ " \"id\": \"6b221d5bc9e6496c\",\n"
343-
+ " \"remoteEndpoint\": {\n"
339+
+ " \"localEndpoint\": {\n"
344340
+ " \"ipv4\": \"127.0.0.1\"\n"
345341
+ " }\n"
346342
+ "}";
347343

348-
assertThat(codec.readSpan(json.getBytes(UTF_8)).remoteEndpoint())
344+
assertThat(codec.readSpan(json.getBytes(UTF_8)).localEndpoint())
349345
.isEqualTo(Endpoint.create("", 127 << 24 | 1));
350346
}
351347

352-
@Test public void readSpan_remoteEndpoint_nullServiceName() {
348+
@Test public void readSpan_remoteEndpoint_noServiceName() {
353349
String json = "{\n"
354350
+ " \"traceId\": \"6b221d5bc9e6496c\",\n"
355351
+ " \"name\": \"get-traces\",\n"
356352
+ " \"id\": \"6b221d5bc9e6496c\",\n"
357353
+ " \"remoteEndpoint\": {\n"
358-
+ " \"serviceName\": null,\n"
359354
+ " \"ipv4\": \"127.0.0.1\"\n"
360355
+ " }\n"
361356
+ "}";

0 commit comments

Comments
 (0)