From f56e58e0155fd66f45c2a967525d9e8d1f1342bd Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 28 Sep 2026 23:52:37 +0000 Subject: [PATCH 1/3] fix(decode): stop infinite loop on non-object repeated message elements A message reader that met a non-object token returned an empty message without consuming the token. Inside a repeated field the enclosing array loop then saw the same token forever and appended messages until OutOfMemoryError: {"repeatedMsg":[1]} or [true] on all three decode paths, {"repeatedMsg":[null]} on the codegen path, repeated Struct and Empty elements, and non-array values for repeated message fields. Every reader now consumes the container it expects or throws a JSONException naming the proto type: '{' for messages, maps, Struct, Any and Empty, '[' for repeated fields and ListValue. The checks live in FieldReader.requireObjectStart/requireArrayStart, which generated decoders call too, so all paths report the same error. Valid proto3 JSON always has these shapes, so no valid input changes behavior. Codegen also no longer accepts a bare number or array as google.protobuf.Empty, matching the runtime paths. BuffJsonMalformedContainerTest runs 209 malformed-container cases on all three paths under a timeout. Before the fix the test JVM died with "Java heap space". Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_013AbdzHqfNXSdynywckLnWY --- buff-json-protoc-plugin/CLAUDE.md | 1 + .../buffjson/protoc/DecoderGenerator.java | 24 +- buff-json-tests/CLAUDE.md | 1 + .../BuffJsonMalformedContainerTest.java | 245 ++++++++++++++++++ buff-json/CLAUDE.md | 1 + .../buffjson/internal/FieldReader.java | 28 +- .../internal/ProtobufMessageReader.java | 4 +- .../internal/TypedMessageReaderSchema.java | 6 +- .../buffjson/internal/WellKnownTypes.java | 6 +- 9 files changed, 301 insertions(+), 15 deletions(-) create mode 100644 buff-json-tests/src/test/java/io/suboptimal/buffjson/BuffJsonMalformedContainerTest.java diff --git a/buff-json-protoc-plugin/CLAUDE.md b/buff-json-protoc-plugin/CLAUDE.md index 34c1ce6..b0f1746 100644 --- a/buff-json-protoc-plugin/CLAUDE.md +++ b/buff-json-protoc-plugin/CLAUDE.md @@ -76,6 +76,7 @@ For each non-WKT, non-map-entry message type: - **Deprecated fields/types** — included in generated codecs. Both codec classes suppress Java deprecation warnings so generated calls compile with `-Werror`; protobuf deprecation does not change JSON semantics. - **Unsigned map keys** — uint32/fixed32 use `Integer.toUnsignedLong`; uint64/fixed64 use `WellKnownTypes.writeUnsignedLongString`. Keys always remain quoted JSON strings. Long-key writes share `FieldWriter.writeLongMapKey`, which preserves key spelling under BrowserCompatible and WriteClassName; boolean keys use constant strings. +- **Container checks** — generated decoders call `FieldReader.requireObjectStart` (messages, maps, inline `Empty`), and `requireArrayStart` (repeated fields) instead of ignoring the result of `nextIfObjectStart()`/`nextIfArrayStart()`. A reader that returned without consuming a non-object token made the enclosing array loop spin until `OutOfMemoryError`. Rebuild consumers with `mvn clean install`: the protobuf plugin only regenerates when `.proto` inputs change. - **`google.protobuf.Empty`** is NOT in the WKT set — it serializes as a regular empty message `{}` - **`DynamicMessage`** cannot use generated encoders (would fail cast) — guarded in `ProtobufMessageWriter` - **Map entry types** (`options.map_entry = true`) are skipped — they're synthetic diff --git a/buff-json-protoc-plugin/src/main/java/io/suboptimal/buffjson/protoc/DecoderGenerator.java b/buff-json-protoc-plugin/src/main/java/io/suboptimal/buffjson/protoc/DecoderGenerator.java index f800579..dcccbcf 100644 --- a/buff-json-protoc-plugin/src/main/java/io/suboptimal/buffjson/protoc/DecoderGenerator.java +++ b/buff-json-protoc-plugin/src/main/java/io/suboptimal/buffjson/protoc/DecoderGenerator.java @@ -15,6 +15,7 @@ final class DecoderGenerator { private static final Set WELL_KNOWN_TYPES = BuffJsonProtocPlugin.WELL_KNOWN_TYPES; + private static final String FIELD_READER = "io.suboptimal.buffjson.internal.FieldReader"; private DecoderGenerator() { } @@ -38,9 +39,14 @@ static String generate(Descriptor msgDesc, String javaPackage, String decoderSim sb.append(" @Override\n"); sb.append(" public ").append(messageClassName).append( " readMessage(JSONReader reader, io.suboptimal.buffjson.internal.ProtobufMessageReader msgReader) {\n"); + // Every read must consume its value or throw: a message reader that returned on + // a non-object token (e.g. `1` in `"repeated": [1]`) without consuming it would + // make the enclosing array loop spin forever. + String fullNameLiteral = SourceLiterals.javaString(msgDesc.getFullName()); + sb.append(" ").append(FIELD_READER).append(".requireObjectStart(reader, \"message\", ") + .append(fullNameLiteral).append(");\n"); sb.append(" ").append(messageClassName).append(".Builder builder = ").append(messageClassName) .append(".newBuilder();\n"); - sb.append(" reader.nextIfObjectStart();\n"); sb.append(" while (!reader.nextIfObjectEnd()) {\n"); sb.append(" String fieldName = reader.readFieldName();\n"); sb.append(" if (fieldName == null) break;\n"); @@ -130,7 +136,8 @@ private static void generateRepeatedFieldRead(StringBuilder sb, FieldDescriptor sb.append(" if (!reader.nextIfNull()) {\n"); } - sb.append(indent).append(" reader.nextIfArrayStart();\n"); + sb.append(indent).append(" ").append(FIELD_READER).append(".requireArrayStart(reader, \"repeated field\", ") + .append(SourceLiterals.javaString(fd.getFullName())).append(");\n"); sb.append(indent).append(" while (!reader.nextIfArrayEnd()) {\n"); emitValueRead(sb, fd, adder, protoToJavaClass, protoToDecoderClass, indent + " "); sb.append(indent).append(" }\n"); @@ -158,7 +165,9 @@ private static void generateMapFieldRead(StringBuilder sb, FieldDescriptor fd, M sb.append(" if (!reader.nextIfNull()) {\n"); } - sb.append(indent).append(" reader.nextIfObjectStart();\n"); + String mapNameLiteral = SourceLiterals.javaString(fd.getFullName()); + sb.append(indent).append(" ").append(FIELD_READER).append(".requireObjectStart(reader, \"map field\", ") + .append(mapNameLiteral).append(");\n"); sb.append(indent).append(" while (!reader.nextIfObjectEnd()) {\n"); sb.append(indent).append(" String keyStr = reader.readFieldName();\n"); sb.append(indent).append(" if (keyStr == null) break;\n"); @@ -289,9 +298,12 @@ private static void emitMessageRead(StringBuilder sb, FieldDescriptor fd, String .append("(io.suboptimal.buffjson.internal.WellKnownTypes.readListValue(reader)").append(closeSuffix) .append(");\n"); } else if ("google.protobuf.Empty".equals(fullName)) { - sb.append(indent).append("reader.nextIfObjectStart();\n"); - sb.append(indent) - .append("while (!reader.nextIfObjectEnd()) { reader.readFieldName(); reader.skipValue(); }\n"); + sb.append(indent).append(FIELD_READER) + .append(".requireObjectStart(reader, \"message\", \"google.protobuf.Empty\");\n"); + sb.append(indent).append("while (!reader.nextIfObjectEnd()) {\n"); + sb.append(indent).append(" if (reader.readFieldName() == null) break;\n"); + sb.append(indent).append(" reader.skipValue();\n"); + sb.append(indent).append("}\n"); sb.append(indent).append(prefix).append("(com.google.protobuf.Empty.getDefaultInstance()") .append(closeSuffix).append(");\n"); } else if (WELL_KNOWN_TYPES.contains(fullName)) { diff --git a/buff-json-tests/CLAUDE.md b/buff-json-tests/CLAUDE.md index 4109f4e..5ed4fd5 100644 --- a/buff-json-tests/CLAUDE.md +++ b/buff-json-tests/CLAUDE.md @@ -12,6 +12,7 @@ pure reflection). - `BuffJsonReferenceTest.java` — 5 smoke tests (scalar, default, complex, plus two `DynamicMessage` tests on UTF-16 and UTF-8 paths — `DynamicMessage` is the only thing that exclusively exercises pure reflection in production) - `BuffJsonEncodingRegressionTest.java` — escaped custom names compile in both generated codecs, round-trip through all encoder paths (UTF-16/UTF-8) and both decoders, and accept proto-name aliases. Concrete and actual DynamicMessage WKTs share output/range validation. Deprecated-field fixtures and unsigned key/digit boundaries live in the main conformance tests. - `BuffJsonMemoryTest.java` — 8 reachability tests using `WeakReference` + `System.gc()` to confirm the encoder doesn't retain `Message` references after `encode`/`encodeToBytes`/`encode(stream)` on any of the three paths, including `DynamicMessage`. Steady-state allocation regressions are caught separately by `./allocation-check.sh` in CI (JMH `-prof gc`). +- `BuffJsonMalformedContainerTest.java` — untrusted-input regression tests for mismatched containers, each case on **all three decode paths** inside `assertTimeoutPreemptively` (a regression fails fast instead of exhausting the heap): non-object repeated message/Struct/ListValue/Any/Empty elements, non-array repeated values, non-object map and singular message values (including top-level), malformed objects inside containers, and truncated input and null repeated message elements (termination only). Uses `TestNesting` and the official `TestAllTypesProto3`, which has repeated fields of every WKT. - `BuffJsonCrossPathFuzzTest.java` — seeded-random (reproducible) fuzzer over `TestAllTypesProto3`. `encodePathsAgreeAndAreParseable` asserts **codegen == typed == reflection** byte-for-byte (UTF-16 and UTF-8) over 500 messages — the direct "the three paths agree" guarantee — plus a buff-json self round-trip. `decodePathsRoundTrip` asserts both decode paths reconstruct messages from `JsonFormat`-printed JSON. (It does not byte-compare encode output against `JsonFormat` because fastjson2 and protobuf may format the same float/double differently — both round-trip to the same value; curated byte-equality lives in `BuffJsonProto3ConformanceTest`.) - `BuffJsonProto3ConformanceTest.java` — proto3 JSON **encode** coverage in nested classes, each `assertMatchesReference` validating all three paths (codegen, typed-accessor, reflection) byte-for-byte against `JsonFormat`; well-known-type groups also carry out-of-range **edge cases** asserting all three paths reject identically: - ScalarTypes (13): all types, boundaries, NaN, Infinity, -0.0, unicode, escapes, bytes diff --git a/buff-json-tests/src/test/java/io/suboptimal/buffjson/BuffJsonMalformedContainerTest.java b/buff-json-tests/src/test/java/io/suboptimal/buffjson/BuffJsonMalformedContainerTest.java new file mode 100644 index 0000000..f7381c0 --- /dev/null +++ b/buff-json-tests/src/test/java/io/suboptimal/buffjson/BuffJsonMalformedContainerTest.java @@ -0,0 +1,245 @@ +package io.suboptimal.buffjson; + +import static org.junit.jupiter.api.Assertions.*; + +import java.time.Duration; +import java.util.ArrayList; +import java.util.LinkedHashMap; +import java.util.List; +import java.util.Map; +import java.util.stream.Stream; + +import com.alibaba.fastjson2.JSONException; +import com.google.protobuf.Message; +import com.google.protobuf.TypeRegistry; +import com.google.protobuf_test_messages.proto3.TestMessagesProto3.TestAllTypesProto3; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; + +import io.suboptimal.buffjson.proto.NestedMessage; +import io.suboptimal.buffjson.proto.TestNesting; + +/** + * Regression tests for untrusted JSON whose containers don't match the schema: + * a non-object where a message, map, Struct, Any or Empty is expected, or a + * non-array where a repeated field or ListValue is expected. + * + *

+ * Before the fix, a message reader that met such a token returned without + * consuming it. Inside a repeated field the enclosing array loop then saw the + * same token forever, appending empty messages until {@code OutOfMemoryError} + * (for example {@code {"repeatedNested":[1]}} on every decode path, or + * {@code {"repeatedNested":[null]}} on the codegen path). Every case runs on + * all three decode paths under a timeout, so a regression fails fast instead of + * exhausting the heap. + */ +class BuffJsonMalformedContainerTest { + + private static final Duration TIMEOUT = Duration.ofSeconds(10); + + private static final TypeRegistry REGISTRY = TypeRegistry.newBuilder().add(TestAllTypesProto3.getDescriptor()) + .add(NestedMessage.getDescriptor()).build(); + + private static Map paths() { + Map paths = new LinkedHashMap<>(); + paths.put("codegen", BuffJson.decoder().setTypeRegistry(REGISTRY)); + paths.put("typed", BuffJson.decoder().setGeneratedDecoders(false).setTypeRegistry(REGISTRY)); + paths.put("reflection", + BuffJson.decoder().setGeneratedDecoders(false).setTypedAccessors(false).setTypeRegistry(REGISTRY)); + return paths; + } + + private static Stream onAllPaths(Class type, String... inputs) { + List args = new ArrayList<>(); + for (var path : paths().entrySet()) { + for (String json : inputs) { + args.add(Arguments.of(path.getKey(), path.getValue(), type, json)); + } + } + return args.stream(); + } + + private static void assertRejected(BuffJsonDecoder decoder, Class type, String json) { + assertTimeoutPreemptively(TIMEOUT, () -> { + assertThrows(JSONException.class, () -> decoder.decode(json, type), json); + }); + } + + /** Must finish within the timeout, either decoding or with a JSONException. */ + private static void assertTerminates(BuffJsonDecoder decoder, Class type, String json) { + assertTimeoutPreemptively(TIMEOUT, () -> { + try { + decoder.decode(json, type); + } catch (JSONException rejected) { + // either outcome is fine; only termination is asserted + } + }, json); + } + + // ========================================================================= + // Repeated message elements that are not objects (the infinite loop) + // ========================================================================= + + static Stream nonObjectRepeatedMessageElements() { + return Stream.concat( + onAllPaths(TestNesting.class, "{\"repeatedNested\":[1]}", "{\"repeatedNested\":[true]}", + "{\"repeatedNested\":[false]}", "{\"repeatedNested\":[\"x\"]}", "{\"repeatedNested\":[[]]}", + "{\"repeatedNested\":[{\"value\":1},2]}", "{\"repeatedNested\":[{\"value\":1},[]]}"), + onAllPaths(TestAllTypesProto3.class, "{\"repeatedNestedMessage\":[1]}", + "{\"repeatedForeignMessage\":[true]}", "{\"repeatedStruct\":[1]}", "{\"repeatedStruct\":[[1]]}", + "{\"repeatedListValue\":[1]}", "{\"repeatedListValue\":[{}]}", "{\"repeatedAny\":[1]}", + "{\"repeatedAny\":[[]]}", "{\"repeatedEmpty\":[1]}", "{\"repeatedEmpty\":[[]]}")); + } + + @ParameterizedTest(name = "{0}: {3}") + @MethodSource + void nonObjectRepeatedMessageElements(String path, BuffJsonDecoder decoder, Class type, + String json) { + assertRejected(decoder, type, json); + } + + // ========================================================================= + // Repeated / map fields whose value is the wrong container + // ========================================================================= + + static Stream nonArrayRepeatedValues() { + return Stream.concat( + onAllPaths(TestNesting.class, "{\"repeatedNested\":5}", "{\"repeatedNested\":{}}", + "{\"repeatedNested\":\"x\"}", "{\"repeatedNested\":true}", "{\"repeatedEnum\":\"FOO\"}"), + onAllPaths(TestAllTypesProto3.class, "{\"repeatedInt32\":5}", "{\"repeatedString\":\"a\"}", + "{\"repeatedStruct\":{}}", "{\"repeatedValue\":1}", "{\"repeatedTimestamp\":\"x\"}")); + } + + @ParameterizedTest(name = "{0}: {3}") + @MethodSource + void nonArrayRepeatedValues(String path, BuffJsonDecoder decoder, Class type, String json) { + assertRejected(decoder, type, json); + } + + static Stream nonObjectMapValues() { + return onAllPaths(TestAllTypesProto3.class, "{\"mapStringString\":[]}", "{\"mapStringString\":5}", + "{\"mapStringNestedMessage\":5}", "{\"mapStringNestedMessage\":{\"k\":1}}", + "{\"mapStringNestedMessage\":{\"k\":[]}}"); + } + + @ParameterizedTest(name = "{0}: {3}") + @MethodSource + void nonObjectMapValues(String path, BuffJsonDecoder decoder, Class type, String json) { + assertRejected(decoder, type, json); + } + + // ========================================================================= + // Singular message values and top-level input that are not objects + // ========================================================================= + + static Stream nonObjectSingularMessages() { + return Stream.concat(onAllPaths(TestNesting.class, "{\"nested\":1}", "{\"nested\":[]}", "1", "[]", "true"), + onAllPaths(TestAllTypesProto3.class, "{\"optionalNestedMessage\":1}", "{\"optionalStruct\":1}", + "{\"optionalStruct\":[1]}", "{\"optionalAny\":1}", "{\"optionalEmpty\":1}", + "{\"optionalEmpty\":[]}", "{\"recursiveMessage\":{\"recursiveMessage\":1}}")); + } + + @ParameterizedTest(name = "{0}: {3}") + @MethodSource + void nonObjectSingularMessages(String path, BuffJsonDecoder decoder, Class type, String json) { + assertRejected(decoder, type, json); + } + + // ========================================================================= + // Malformed objects inside containers + // ========================================================================= + + static Stream malformedObjectsInsideContainers() { + // A member without a name ends the object early (unchanged behavior); the + // leftover token is then rejected by the enclosing reader or by the top-level + // trailing-input check instead of being re-read forever. + return Stream.concat( + onAllPaths(TestNesting.class, "{\"nested\":{\"value\":1, 2}}", + "{\"repeatedNested\":[{\"value\":1, 2}]}", "{\"repeatedNested\":[{\"value\":1, true}]}"), + onAllPaths(TestAllTypesProto3.class, "{\"mapStringString\":{\"a\":\"b\", 1}}", + "{\"repeatedStruct\":[{\"a\":1, 2}]}", "{\"optionalStruct\":{\"a\":1, 2}}", + "{\"optionalEmpty\":{1}}", "{\"repeatedEmpty\":[{1}]}", "{\"optionalAny\":{\"a\":1, 2}}", + "{\"optionalAny\":{\"@type\":\"type.googleapis.com/google.protobuf.Timestamp\", 5}}")); + } + + @ParameterizedTest(name = "{0}: {3}") + @MethodSource + void malformedObjectsInsideContainers(String path, BuffJsonDecoder decoder, Class type, + String json) { + assertRejected(decoder, type, json); + } + + static Stream truncatedInput() { + return Stream.concat( + onAllPaths(TestNesting.class, "{\"repeatedNested\":[", "{\"repeatedNested\":[{", + "{\"repeatedNested\":[{\"value\":1", "{\"repeatedNested\":[{\"value\":1},"), + onAllPaths(TestAllTypesProto3.class, "{\"repeatedStruct\":[{\"a\":1", "{\"repeatedListValue\":[[1", + "{\"repeatedEmpty\":[{", "{\"optionalEmpty\":{", "{\"mapStringNestedMessage\":{\"k\":{")); + } + + /** + * Input cut off mid-container must terminate. Truncated objects are accepted + * leniently at end of input (see {@code BuffJsonErrorTest}), so either outcome + * is fine here; only termination is pinned. + */ + @ParameterizedTest(name = "{0}: {3}") + @MethodSource + void truncatedInput(String path, BuffJsonDecoder decoder, Class type, String json) { + assertTerminates(decoder, type, json); + } + + // ========================================================================= + // Null elements in repeated message fields + // ========================================================================= + + static Stream nullRepeatedMessageElements() { + return Stream.concat(onAllPaths(TestNesting.class, "{\"repeatedNested\":[null]}"), + onAllPaths(TestAllTypesProto3.class, "{\"repeatedNestedMessage\":[{\"a\":1},null,{\"a\":2}]}", + "{\"repeatedStruct\":[null]}", "{\"repeatedListValue\":[null]}", "{\"repeatedAny\":[null]}", + "{\"repeatedEmpty\":[null]}")); + } + + /** + * A null element used to loop forever on the codegen path. It now terminates on + * every path. Codegen rejects it (as {@code JsonFormat} does), while the + * runtime paths keep their existing behavior of skipping null elements; either + * outcome is accepted here so this test only pins termination. + */ + @ParameterizedTest(name = "{0}: {3}") + @MethodSource + void nullRepeatedMessageElements(String path, BuffJsonDecoder decoder, Class type, String json) { + assertTerminates(decoder, type, json); + } + + // ========================================================================= + // Error message and valid input + // ========================================================================= + + @Test + void errorNamesTheExpectedTypeAndOffsetOnEveryPath() { + for (var path : paths().entrySet()) { + JSONException ex = assertTimeoutPreemptively(TIMEOUT, () -> assertThrows(JSONException.class, + () -> path.getValue().decode("{\"repeatedNested\":[{\"value\":1},7]}", TestNesting.class))); + assertTrue( + ex.getMessage() + .contains("Expected a JSON object for message io.suboptimal.buffjson.proto.NestedMessage"), + path.getKey() + ": " + ex.getMessage()); + assertTrue(ex.getMessage().contains("offset"), path.getKey() + ": " + ex.getMessage()); + } + } + + @Test + void wellFormedContainersStillDecodeIdenticallyOnEveryPath() { + var expected = TestNesting.newBuilder().setNested(NestedMessage.newBuilder().setValue(1).setName("a")) + .addRepeatedNested(NestedMessage.newBuilder().setValue(2)) + .addRepeatedNested(NestedMessage.getDefaultInstance()).addRepeatedEnumValue(1).build(); + String json = "{ \"nested\" : { \"value\" : 1 , \"name\" : \"a\" } , \"repeatedNested\" : [ { \"value\" : 2 } , { } ] ," + + " \"repeated_enum\" : [ \"TEST_ENUM_FOO\" ] , \"unknown\" : [ 1 , { \"x\" : [ ] } ] }"; + for (var path : paths().entrySet()) { + assertEquals(expected, path.getValue().decode(json, TestNesting.class), path.getKey()); + } + } +} diff --git a/buff-json/CLAUDE.md b/buff-json/CLAUDE.md index 8801af9..c0cd42c 100644 --- a/buff-json/CLAUDE.md +++ b/buff-json/CLAUDE.md @@ -141,6 +141,7 @@ Nested concrete messages use their own builders rather than parsing into Dynamic The decoder consumes untrusted JSON, so a few defenses are built into the read path. All are zero-cost on the success path. - **Strict int32/uint32 + string parsing (`FieldReader.readStrictInt32`/`readStrictUint32`/`readStrictString`)**: rather than letting fastjson2 coerce, these enforce the proto3 JSON spec so malformed input is *rejected* (a `JSONException`) instead of silently corrupting data. int32/uint32 accept an integer JSON number or a quoted integer string and reject non-integral numbers (`1.5`), out-of-range values (uint32 > 2³²−1, int32 overflow), empty/non-numeric strings, and wrong JSON types (bool/object/array); integral floats (`2.0`, `1e2`) are accepted per the spec. String fields reject any non-string token. All three decode paths use these (reflection and typed setters via `FieldReader`; codegen via `DecoderGenerator`), and because repeated/map readers call the same helpers per element, wrong-element-type arrays are rejected too. **The common path is zero-allocation**: `isNumber()` rejects bool/object/array with no read, and the bare-number value is read via the *primitive* `readDoubleValue()` — exact for the 32-bit range (`|max| < 2⁵³`), so a fractional part (`1.5`) and out-of-range are detected via `rint`/comparison with no boxing or `BigDecimal` (measured 0 B/op, same as the old lenient `readInt64Value`). Only the non-canonical *quoted* form (`"42"`) allocates (String + `BigDecimal`), which `JsonFormat` never emits for 32-bit fields. **Caveat — don't gate on `reader.isInt()`/`readInt64Value()`**: `isInt()` means "the token starts like a number," not "is integral" (it's true for `1.5`), and `readInt64Value()` silently truncates `1.5`→`1` and coerces `true`→`1`. (int64/uint64 parsing stays as-is — those tests aren't gaps; the canonical 64-bit form is a quoted string.) +- **Every reader consumes its container or throws (`FieldReader.requireObjectStart`/`requireArrayStart`)**: a message, map, `Struct`, `Any` or `Empty` value must open with `{`, and a repeated field or `ListValue` with `[`; otherwise a `JSONException` names the expected container and the proto type. This is what keeps array loops finite: previously a message reader that met a non-object token returned an empty message *without consuming it*, so `{"repeatedMsg":[1]}` (all three paths), `{"repeatedMsg":[null]}` (codegen) or a non-array repeated value spun forever, appending messages until `OutOfMemoryError`. All three paths share the helpers (`DecoderGenerator` emits calls to them), so messages and offsets match. Valid proto3 JSON always satisfies these shapes and `JsonFormat` rejects the rest, so no valid input changes behavior; the checks test a `boolean` the reader already returned (zero cost). Null *elements* in repeated fields still differ by path: codegen rejects them, the typed and reflection paths skip them. Because every element read now consumes at least its opening token, each array-loop iteration makes progress, so termination no longer depends on the element reader. A member without a name (`{"a":1, 2}`) still ends the object early, as before: the leftover token is rejected by the enclosing reader or the top-level trailing-input check, except `[{"a":1, {}}]`, which is still accepted as two elements. Tests: `BuffJsonMalformedContainerTest` (every case on all three paths under a timeout). - **Top-level `null` rejected (`BuffJsonDecoder.readProto`)**: a bare top-level JSON `null` is not a valid message (proto3 JSON only allows `null` as a field value meaning "absent", or as a wrapped `NullValue`), so the top-level decode entry throws a `JSONException` instead of returning a null `Message` (which would NPE downstream). This is distinct from *empty input* — a `null`/empty Java `String` or `byte[]` is short-circuited by the public `decode(...)` methods to `null` as a lenient convenience, and from *field-level* `null` (handled in `readFieldsInto`, still means "absent"). Only the literal `null` payload reaches `readProto`. The fastjson2 module path (`readObject`) is unchanged. - **Recursion depth cap (`WellKnownTypes.MAX_RECURSION_DEPTH = 100`)**: The `Struct`/`Value`/`ListValue` reader (`readStruct`/`readListValue`/`readJsonValueImpl`) threads an `int depth` and throws a clean `JSONException` past 100 levels instead of `StackOverflowError`. 100 matches protobuf's own limit (`CodedInputStream.DEFAULT_RECURSION_LIMIT` and `JsonFormat.Parser`'s default). Public single-arg entry points (`readStruct(reader)`, etc.) delegate to private `(reader, depth)` overloads, so generated decoders keep calling the unchanged signatures — no codegen ABI change. Note: this caps the universal Struct/Value/ListValue vector; arbitrary message nesting (self-referential message types) is not capped because that would require threading depth through the `BuffJsonGeneratedDecoder` ABI. - **Any `@type`-first fast path** (`WellKnownTypes.readAny`): the canonical proto3 form lists `@type` first, so the descriptor is resolved before any content and the remaining fields are decoded straight off the live reader via `ProtobufMessageReader.readRemainingMessageFields` (regular messages → `DynamicMessage`) or direct WKT read — no `LinkedHashMap` buffering, no `JSON.toJSONString` + re-parse. The buffer-and-reparse slow path is retained only for the rare case where `@type` arrives after content. diff --git a/buff-json/src/main/java/io/suboptimal/buffjson/internal/FieldReader.java b/buff-json/src/main/java/io/suboptimal/buffjson/internal/FieldReader.java index d9c7fba..2afd448 100644 --- a/buff-json/src/main/java/io/suboptimal/buffjson/internal/FieldReader.java +++ b/buff-json/src/main/java/io/suboptimal/buffjson/internal/FieldReader.java @@ -316,12 +316,36 @@ private static Message readMessageValue(JSONReader reader, FieldDescriptor fd, P return msgReader.readMessage(reader, msgDesc); } + /** + * Consumes the opening {@code '{'} of a message, map or Struct value, or throws + * {@link JSONException}. A reader must never return without consuming its + * value: an element reader that leaves a non-object token (such as {@code 1} in + * {@code "repeatedMessage": [1]}) in place makes the enclosing array loop see + * the same token forever. Public so generated decoders (in other packages) + * share the same check and error message. + */ + public static void requireObjectStart(JSONReader reader, String kind, String name) { + if (!reader.nextIfObjectStart()) { + throw new JSONException(reader.info("Expected a JSON object for " + kind + " " + name)); + } + } + + /** + * Consumes the opening {@code '['} of a repeated field or ListValue, or throws + * {@link JSONException} (see {@link #requireObjectStart}). + */ + public static void requireArrayStart(JSONReader reader, String kind, String name) { + if (!reader.nextIfArrayStart()) { + throw new JSONException(reader.info("Expected a JSON array for " + kind + " " + name)); + } + } + /** * Reads a repeated field as a JSON array, adding each element to the builder. */ public static void readRepeated(JSONReader reader, Message.Builder builder, FieldDescriptor fd, ProtobufMessageReader msgReader) { - reader.nextIfArrayStart(); + requireArrayStart(reader, "repeated field", fd.getFullName()); while (!reader.nextIfArrayEnd()) { if (reader.nextIfNull()) { continue; @@ -340,7 +364,7 @@ public static void readMap(JSONReader reader, Message.Builder builder, FieldDesc FieldDescriptor keyFd = entryDesc.findFieldByName("key"); FieldDescriptor valueFd = entryDesc.findFieldByName("value"); - reader.nextIfObjectStart(); + requireObjectStart(reader, "map field", fd.getFullName()); while (!reader.nextIfObjectEnd()) { String keyStr = reader.readFieldName(); if (keyStr == null) { diff --git a/buff-json/src/main/java/io/suboptimal/buffjson/internal/ProtobufMessageReader.java b/buff-json/src/main/java/io/suboptimal/buffjson/internal/ProtobufMessageReader.java index 7db20a7..f9e5b0a 100644 --- a/buff-json/src/main/java/io/suboptimal/buffjson/internal/ProtobufMessageReader.java +++ b/buff-json/src/main/java/io/suboptimal/buffjson/internal/ProtobufMessageReader.java @@ -123,8 +123,8 @@ public Message readMessage(JSONReader reader, Descriptor descriptor, Message def * descriptor/builder fallback. */ Message readMessageRuntime(JSONReader reader, Descriptor descriptor, Message defaultInstance) { + FieldReader.requireObjectStart(reader, "message", descriptor.getFullName()); Message.Builder builder = defaultInstance.newBuilderForType(); - reader.nextIfObjectStart(); readRuntimeFields(reader, builder, descriptor); return builder.build(); } @@ -145,7 +145,7 @@ Message readMessage(JSONReader reader, Message.Builder builder) { return decoder.readMessage(reader, this); } } - reader.nextIfObjectStart(); + FieldReader.requireObjectStart(reader, "message", builder.getDescriptorForType().getFullName()); readRuntimeFields(reader, builder, builder.getDescriptorForType()); return builder.build(); } diff --git a/buff-json/src/main/java/io/suboptimal/buffjson/internal/TypedMessageReaderSchema.java b/buff-json/src/main/java/io/suboptimal/buffjson/internal/TypedMessageReaderSchema.java index 6b13869..356baa6 100644 --- a/buff-json/src/main/java/io/suboptimal/buffjson/internal/TypedMessageReaderSchema.java +++ b/buff-json/src/main/java/io/suboptimal/buffjson/internal/TypedMessageReaderSchema.java @@ -149,8 +149,9 @@ private static Parser create(FieldDescriptor fd, Class messageClass, Class if (!fd.isRepeated()) { return scalar; } + String fieldName = fd.getFullName(); return (r, b, mr) -> { - r.nextIfArrayStart(); + FieldReader.requireArrayStart(r, "repeated field", fieldName); while (!r.nextIfArrayEnd()) { if (!r.nextIfNull()) { scalar.read(r, b, mr); @@ -175,8 +176,9 @@ private static Parser createMap(FieldDescriptor fd, String suffix, Class mess : valueFd.getJavaType() == FieldDescriptor.JavaType.MESSAGE ? ProtobufMessageReader.getDefaultInstance(valueClass) : FieldReader.getDefaultMapValue(valueFd); + String fieldName = fd.getFullName(); return (r, b, mr) -> { - r.nextIfObjectStart(); + FieldReader.requireObjectStart(r, "map field", fieldName); while (!r.nextIfObjectEnd()) { String keyText = r.readFieldName(); if (keyText == null) { diff --git a/buff-json/src/main/java/io/suboptimal/buffjson/internal/WellKnownTypes.java b/buff-json/src/main/java/io/suboptimal/buffjson/internal/WellKnownTypes.java index 29f64fd..ae0909d 100644 --- a/buff-json/src/main/java/io/suboptimal/buffjson/internal/WellKnownTypes.java +++ b/buff-json/src/main/java/io/suboptimal/buffjson/internal/WellKnownTypes.java @@ -802,8 +802,8 @@ public static Struct readStruct(JSONReader reader) { private static Struct readStruct(JSONReader reader, int depth) { checkDepth(reader, depth); + FieldReader.requireObjectStart(reader, "message", "google.protobuf.Struct"); Struct.Builder builder = Struct.newBuilder(); - reader.nextIfObjectStart(); while (!reader.nextIfObjectEnd()) { String key = reader.readFieldName(); if (key == null) { @@ -848,8 +848,8 @@ public static ListValue readListValue(JSONReader reader) { private static ListValue readListValue(JSONReader reader, int depth) { checkDepth(reader, depth); + FieldReader.requireArrayStart(reader, "message", "google.protobuf.ListValue"); ListValue.Builder builder = ListValue.newBuilder(); - reader.nextIfArrayStart(); while (!reader.nextIfArrayEnd()) { builder.addValues(readJsonValueImpl(reader, depth)); } @@ -857,7 +857,7 @@ private static ListValue readListValue(JSONReader reader, int depth) { } private static Any readAny(JSONReader reader, ProtobufMessageReader msgReader) { - reader.nextIfObjectStart(); + FieldReader.requireObjectStart(reader, "message", "google.protobuf.Any"); if (reader.nextIfObjectEnd()) { return Any.getDefaultInstance(); From 867e35bc47cc5702998a763a91372aa331be9e2f Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 29 Sep 2026 09:08:02 +0000 Subject: [PATCH 2/3] fix(decode): consistent null elements and empty input after container checks Follow-ups from review of the infinite-loop fix: - Codegen rejects null repeated elements with a JSONException (FieldReader.requireNonNullElement), as JsonFormat does. They used to throw NullPointerException (Timestamp, Duration, FieldMask, String/BytesValue) or add a phantom default element (int64, bool, enum, wrappers). Value elements keep null-as-NullValue, and NullValue elements now map null to NULL_VALUE. - The typed and reflection repeated readers keep null elements of repeated Value/NullValue as values (FieldReader.nullValueFor) instead of dropping them, matching codegen, JsonFormat and the encoder's output. Other null elements are still skipped on those paths. - Empty or whitespace-only input decodes to null from every overload (byte[], slice, InputStream), like an empty String. The new object-start check would otherwise have turned these into exceptions. - Hoist the descriptor in readMessage(JSONReader, Builder), use the FIELD_READER constant for every generated FieldReader reference, and correct the docs. Adds TestRepeatedNullValue to the test protos so the generated NullValue branch is compiled and tested on all three paths. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_013AbdzHqfNXSdynywckLnWY --- .../buffjson/protoc/DecoderGenerator.java | 67 +++++++------ buff-json-tests/CLAUDE.md | 2 +- .../src/main/protobuf/conformance_test.proto | 6 ++ .../BuffJsonMalformedContainerTest.java | 98 ++++++++++++++++++- buff-json/CLAUDE.md | 4 +- .../suboptimal/buffjson/BuffJsonDecoder.java | 18 +++- .../buffjson/internal/FieldReader.java | 41 +++++++- .../internal/ProtobufMessageReader.java | 19 ++-- .../internal/TypedMessageReaderSchema.java | 4 + 9 files changed, 207 insertions(+), 52 deletions(-) diff --git a/buff-json-protoc-plugin/src/main/java/io/suboptimal/buffjson/protoc/DecoderGenerator.java b/buff-json-protoc-plugin/src/main/java/io/suboptimal/buffjson/protoc/DecoderGenerator.java index dcccbcf..e575bab 100644 --- a/buff-json-protoc-plugin/src/main/java/io/suboptimal/buffjson/protoc/DecoderGenerator.java +++ b/buff-json-protoc-plugin/src/main/java/io/suboptimal/buffjson/protoc/DecoderGenerator.java @@ -136,9 +136,21 @@ private static void generateRepeatedFieldRead(StringBuilder sb, FieldDescriptor sb.append(" if (!reader.nextIfNull()) {\n"); } + String fieldNameLiteral = SourceLiterals.javaString(fd.getFullName()); sb.append(indent).append(" ").append(FIELD_READER).append(".requireArrayStart(reader, \"repeated field\", ") - .append(SourceLiterals.javaString(fd.getFullName())).append(");\n"); + .append(fieldNameLiteral).append(");\n"); sb.append(indent).append(" while (!reader.nextIfArrayEnd()) {\n"); + // proto3 JSON: a null element is a value only for Value (NullValue, handled by + // readJsonValue) and NullValue (NULL_VALUE); anything else is rejected here + // instead of reaching element readers that NPE (Timestamp, StringValue, ...) + // or coerce it to a phantom default element (int64, bool, enum, ...). + if (isNullValueField(fd)) { + sb.append(indent).append(" if (reader.nextIfNull()) { ").append(adder) + .append("Value(0); continue; }\n"); + } else if (!isValueField(fd)) { + sb.append(indent).append(" ").append(FIELD_READER).append(".requireNonNullElement(reader, ") + .append(fieldNameLiteral).append(");\n"); + } emitValueRead(sb, fd, adder, protoToJavaClass, protoToDecoderClass, indent + " "); sb.append(indent).append(" }\n"); @@ -181,9 +193,8 @@ private static void generateMapFieldRead(StringBuilder sb, FieldDescriptor fd, M String valuePutter = putter + "Value(" + keyExpr + ", "; String enumClass = protoToJavaClass.get(valueFd.getEnumType().getFullName()); sb.append(indent).append(" if (reader.isString()) {\n"); - sb.append(indent).append(" ").append(valuePutter) - .append("io.suboptimal.buffjson.internal.FieldReader.enumNumber(reader, ").append(enumClass) - .append(".getDescriptor(), reader.readString()));\n"); + sb.append(indent).append(" ").append(valuePutter).append(FIELD_READER + ".enumNumber(reader, ") + .append(enumClass).append(".getDescriptor(), reader.readString()));\n"); sb.append(indent).append(" } else {\n"); sb.append(indent).append(" ").append(valuePutter).append("reader.readInt32Value());\n"); sb.append(indent).append(" }\n"); @@ -219,50 +230,42 @@ private static void emitValueRead(StringBuilder sb, FieldDescriptor fd, String p case INT -> { var type = fd.getType(); if (type == FieldDescriptor.Type.UINT32 || type == FieldDescriptor.Type.FIXED32) { - sb.append(indent).append(prefix) - .append("(io.suboptimal.buffjson.internal.FieldReader.readStrictUint32(reader)") + sb.append(indent).append(prefix).append("(" + FIELD_READER + ".readStrictUint32(reader)") .append(closeSuffix).append(");\n"); } else { - sb.append(indent).append(prefix) - .append("(io.suboptimal.buffjson.internal.FieldReader.readStrictInt32(reader)") + sb.append(indent).append(prefix).append("(" + FIELD_READER + ".readStrictInt32(reader)") .append(closeSuffix).append(");\n"); } } case LONG -> { var type = fd.getType(); if (type == FieldDescriptor.Type.UINT64 || type == FieldDescriptor.Type.FIXED64) { - sb.append(indent).append(prefix) - .append("(io.suboptimal.buffjson.internal.FieldReader.readUnsignedLong(reader)") + sb.append(indent).append(prefix).append("(" + FIELD_READER + ".readUnsignedLong(reader)") .append(closeSuffix).append(");\n"); } else { - sb.append(indent).append(prefix) - .append("(io.suboptimal.buffjson.internal.FieldReader.readSignedLong(reader)") + sb.append(indent).append(prefix).append("(" + FIELD_READER + ".readSignedLong(reader)") .append(closeSuffix).append(");\n"); } } - case FLOAT -> sb.append(indent).append(prefix) - .append("(io.suboptimal.buffjson.internal.FieldReader.readFloatValue(reader)").append(closeSuffix) - .append(");\n"); - case DOUBLE -> sb.append(indent).append(prefix) - .append("(io.suboptimal.buffjson.internal.FieldReader.readDoubleValue(reader)").append(closeSuffix) - .append(");\n"); + case FLOAT -> sb.append(indent).append(prefix).append("(" + FIELD_READER + ".readFloatValue(reader)") + .append(closeSuffix).append(");\n"); + case DOUBLE -> sb.append(indent).append(prefix).append("(" + FIELD_READER + ".readDoubleValue(reader)") + .append(closeSuffix).append(");\n"); case BOOLEAN -> sb.append(indent).append(prefix).append("(reader.readBoolValue()").append(closeSuffix).append(");\n"); - case STRING -> sb.append(indent).append(prefix) - .append("(io.suboptimal.buffjson.internal.FieldReader.readStrictString(reader)").append(closeSuffix) - .append(");\n"); - case BYTE_STRING -> sb.append(indent).append(prefix) - .append("(io.suboptimal.buffjson.internal.FieldReader.readBytes(reader)").append(closeSuffix) - .append(");\n"); + case STRING -> sb.append(indent).append(prefix).append("(" + FIELD_READER + ".readStrictString(reader)") + .append(closeSuffix).append(");\n"); + case BYTE_STRING -> sb.append(indent).append(prefix).append("(" + FIELD_READER + ".readBytes(reader)") + .append(closeSuffix).append(");\n"); case ENUM -> { // Enum fields use the Value variant: setFoo -> setFooValue, addFoo -> // addFooValue String valueName = prefix + "Value"; String enumClass = protoToJavaClass.get(fd.getEnumType().getFullName()); sb.append(indent).append("if (reader.isString()) {\n"); - sb.append(indent).append(" ").append(valueName) - .append("(io.suboptimal.buffjson.internal.FieldReader.enumNumber(reader, ").append(enumClass) - .append(".getDescriptor(), reader.readString())").append(closeSuffix).append(");\n"); + sb.append(indent).append(" ").append(valueName).append("(" + FIELD_READER + ".enumNumber(reader, ") + .append(enumClass).append(".getDescriptor(), reader.readString())").append(closeSuffix) + .append(");\n"); sb.append(indent).append("} else {\n"); sb.append(indent).append(" ").append(valueName).append("(reader.readInt32Value()") .append(closeSuffix).append(");\n"); @@ -333,16 +336,16 @@ private static String mapKeyExpr(FieldDescriptor keyFd) { case INT -> { var type = keyFd.getType(); if (type == FieldDescriptor.Type.UINT32 || type == FieldDescriptor.Type.FIXED32) - yield "io.suboptimal.buffjson.internal.FieldReader.parseUnsignedIntKey(reader, keyStr)"; - yield "io.suboptimal.buffjson.internal.FieldReader.parseIntKey(reader, keyStr)"; + yield FIELD_READER + ".parseUnsignedIntKey(reader, keyStr)"; + yield FIELD_READER + ".parseIntKey(reader, keyStr)"; } case LONG -> { var type = keyFd.getType(); if (type == FieldDescriptor.Type.UINT64 || type == FieldDescriptor.Type.FIXED64) - yield "io.suboptimal.buffjson.internal.FieldReader.parseUnsignedLongKey(reader, keyStr)"; - yield "io.suboptimal.buffjson.internal.FieldReader.parseLongKey(reader, keyStr)"; + yield FIELD_READER + ".parseUnsignedLongKey(reader, keyStr)"; + yield FIELD_READER + ".parseLongKey(reader, keyStr)"; } - case BOOLEAN -> "io.suboptimal.buffjson.internal.FieldReader.parseBoolKey(reader, keyStr)"; + case BOOLEAN -> FIELD_READER + ".parseBoolKey(reader, keyStr)"; default -> throw new IllegalArgumentException("Unsupported map key type: " + keyFd.getJavaType()); }; } diff --git a/buff-json-tests/CLAUDE.md b/buff-json-tests/CLAUDE.md index 5ed4fd5..c2ccbcc 100644 --- a/buff-json-tests/CLAUDE.md +++ b/buff-json-tests/CLAUDE.md @@ -12,7 +12,7 @@ pure reflection). - `BuffJsonReferenceTest.java` — 5 smoke tests (scalar, default, complex, plus two `DynamicMessage` tests on UTF-16 and UTF-8 paths — `DynamicMessage` is the only thing that exclusively exercises pure reflection in production) - `BuffJsonEncodingRegressionTest.java` — escaped custom names compile in both generated codecs, round-trip through all encoder paths (UTF-16/UTF-8) and both decoders, and accept proto-name aliases. Concrete and actual DynamicMessage WKTs share output/range validation. Deprecated-field fixtures and unsigned key/digit boundaries live in the main conformance tests. - `BuffJsonMemoryTest.java` — 8 reachability tests using `WeakReference` + `System.gc()` to confirm the encoder doesn't retain `Message` references after `encode`/`encodeToBytes`/`encode(stream)` on any of the three paths, including `DynamicMessage`. Steady-state allocation regressions are caught separately by `./allocation-check.sh` in CI (JMH `-prof gc`). -- `BuffJsonMalformedContainerTest.java` — untrusted-input regression tests for mismatched containers, each case on **all three decode paths** inside `assertTimeoutPreemptively` (a regression fails fast instead of exhausting the heap): non-object repeated message/Struct/ListValue/Any/Empty elements, non-array repeated values, non-object map and singular message values (including top-level), malformed objects inside containers, and truncated input and null repeated message elements (termination only). Uses `TestNesting` and the official `TestAllTypesProto3`, which has repeated fields of every WKT. +- `BuffJsonMalformedContainerTest.java` — untrusted-input regression tests for mismatched containers, each case on **all three decode paths** inside `assertTimeoutPreemptively` (a regression fails at the timeout instead of hanging the build; the stuck thread can't be stopped, so the fork may still OOM afterwards): non-object repeated message/Struct/ListValue/Any/Empty elements, non-array repeated values, non-object map and singular message values (including top-level), malformed objects inside containers, and truncated input and null repeated message elements (termination only). Null scalar/WKT repeated elements are rejected with a `JSONException` on codegen (they used to NPE or add a phantom default) and skipped on the runtime paths; null `repeated Value` elements are kept as `NullValue` on every path (matches `JsonFormat` and round-trips); empty input (empty/whitespace-only `String`, `byte[]`, slice or `InputStream`) decodes to `null` from every overload. Uses `TestNesting` and the official `TestAllTypesProto3`, which has repeated fields of every WKT. - `BuffJsonCrossPathFuzzTest.java` — seeded-random (reproducible) fuzzer over `TestAllTypesProto3`. `encodePathsAgreeAndAreParseable` asserts **codegen == typed == reflection** byte-for-byte (UTF-16 and UTF-8) over 500 messages — the direct "the three paths agree" guarantee — plus a buff-json self round-trip. `decodePathsRoundTrip` asserts both decode paths reconstruct messages from `JsonFormat`-printed JSON. (It does not byte-compare encode output against `JsonFormat` because fastjson2 and protobuf may format the same float/double differently — both round-trip to the same value; curated byte-equality lives in `BuffJsonProto3ConformanceTest`.) - `BuffJsonProto3ConformanceTest.java` — proto3 JSON **encode** coverage in nested classes, each `assertMatchesReference` validating all three paths (codegen, typed-accessor, reflection) byte-for-byte against `JsonFormat`; well-known-type groups also carry out-of-range **edge cases** asserting all three paths reject identically: - ScalarTypes (13): all types, boundaries, NaN, Infinity, -0.0, unicode, escapes, bytes diff --git a/buff-json-tests/src/main/protobuf/conformance_test.proto b/buff-json-tests/src/main/protobuf/conformance_test.proto index 8071681..cacb4ef 100644 --- a/buff-json-tests/src/main/protobuf/conformance_test.proto +++ b/buff-json-tests/src/main/protobuf/conformance_test.proto @@ -162,6 +162,12 @@ message TestStruct { google.protobuf.ListValue list_value = 3; } +// Repeated google.protobuf.NullValue: a JSON null element is a value (NULL_VALUE), +// not an absent element. Exercises the generated decoder's null-element branch. +message TestRepeatedNullValue { + repeated google.protobuf.NullValue values = 1; +} + // Proto3 explicit presence (optional keyword) message TestOptionalFields { optional int32 optional_int32 = 1; diff --git a/buff-json-tests/src/test/java/io/suboptimal/buffjson/BuffJsonMalformedContainerTest.java b/buff-json-tests/src/test/java/io/suboptimal/buffjson/BuffJsonMalformedContainerTest.java index f7381c0..8246678 100644 --- a/buff-json-tests/src/test/java/io/suboptimal/buffjson/BuffJsonMalformedContainerTest.java +++ b/buff-json-tests/src/test/java/io/suboptimal/buffjson/BuffJsonMalformedContainerTest.java @@ -33,8 +33,10 @@ * same token forever, appending empty messages until {@code OutOfMemoryError} * (for example {@code {"repeatedNested":[1]}} on every decode path, or * {@code {"repeatedNested":[null]}} on the codegen path). Every case runs on - * all three decode paths under a timeout, so a regression fails fast instead of - * exhausting the heap. + * all three decode paths under a timeout, so a regression is reported as a + * failure at the timeout rather than a hung build. The timed-out decode thread + * cannot be stopped (the loop never checks for interruption), so after such a + * failure the forked test JVM may still run out of memory. */ class BuffJsonMalformedContainerTest { @@ -214,6 +216,98 @@ void nullRepeatedMessageElements(String path, BuffJsonDecoder decoder, Class nullRepeatedScalarAndWktElements() { + return Stream.of("{\"repeatedTimestamp\":[null]}", "{\"repeatedDuration\":[null]}", + "{\"repeatedFieldmask\":[null]}", "{\"repeatedStringWrapper\":[null]}", + "{\"repeatedBytesWrapper\":[null]}", "{\"repeatedInt32Wrapper\":[null]}", "{\"repeatedInt64\":[null]}", + "{\"repeatedUint64\":[null]}", "{\"repeatedBool\":[null]}", "{\"repeatedDouble\":[null]}", + "{\"repeatedNestedEnum\":[null]}", "{\"repeatedInt32\":[1,null]}").map(Arguments::of); + } + + /** + * Codegen rejects a null element with a {@link JSONException} for every type + * but Value/NullValue: it used to NPE for Timestamp/Duration/FieldMask/ + * String/BytesValue and add a phantom default element for int64/bool/enum/... + * The runtime paths skip it (either way no default element is added). + */ + @ParameterizedTest + @MethodSource + void nullRepeatedScalarAndWktElements(String json) { + var paths = paths(); + assertRejected(paths.get("codegen"), TestAllTypesProto3.class, json); + TestAllTypesProto3 expected = json.startsWith("{\"repeatedInt32\":") + ? TestAllTypesProto3.newBuilder().addRepeatedInt32(1).build() + : TestAllTypesProto3.getDefaultInstance(); + for (String runtime : List.of("typed", "reflection")) { + BuffJsonDecoder decoder = paths.get(runtime); + assertEquals(expected, + assertTimeoutPreemptively(TIMEOUT, () -> decoder.decode(json, TestAllTypesProto3.class)), + runtime + ": " + json); + } + } + + @Test + void nullRepeatedValueElementsArePreservedOnEveryPath() throws Exception { + var expected = TestAllTypesProto3.newBuilder() + .addRepeatedValue( + com.google.protobuf.Value.newBuilder().setNullValue(com.google.protobuf.NullValue.NULL_VALUE)) + .addRepeatedValue(com.google.protobuf.Value.newBuilder().setNumberValue(1)).build(); + String json = "{\"repeatedValue\":[null,1]}"; + var reference = TestAllTypesProto3.newBuilder(); + com.google.protobuf.util.JsonFormat.parser().merge(json, reference); + assertEquals(expected, reference.build()); + for (var path : paths().entrySet()) { + assertEquals(expected, path.getValue().decode(json, TestAllTypesProto3.class), path.getKey()); + assertEquals(expected, + path.getValue().decode(BuffJson.encoder().encode(expected), TestAllTypesProto3.class), + path.getKey() + " round trip"); + } + } + + @Test + void nullRepeatedNullValueElementsArePreservedOnEveryPath() throws Exception { + var expected = io.suboptimal.buffjson.proto.TestRepeatedNullValue.newBuilder() + .addValues(com.google.protobuf.NullValue.NULL_VALUE).addValues(com.google.protobuf.NullValue.NULL_VALUE) + .build(); + String json = "{\"values\":[null,null]}"; + var reference = io.suboptimal.buffjson.proto.TestRepeatedNullValue.newBuilder(); + com.google.protobuf.util.JsonFormat.parser().merge(json, reference); + assertEquals(expected, reference.build()); + for (var path : paths().entrySet()) { + assertEquals(expected, + path.getValue().decode(json, io.suboptimal.buffjson.proto.TestRepeatedNullValue.class), + path.getKey()); + assertEquals(expected, path.getValue().decode(BuffJson.encoder().encode(expected), + io.suboptimal.buffjson.proto.TestRepeatedNullValue.class), path.getKey() + " round trip"); + } + } + + /** + * Empty input decodes to {@code null} from every overload, instead of failing + * the new object-start check (an empty {@code InputStream} or whitespace-only + * body used to decode to an empty message). + */ + @Test + void emptyInputDecodesToNullFromEveryOverload() { + byte[] blank = " \n ".getBytes(java.nio.charset.StandardCharsets.UTF_8); + for (var path : paths().entrySet()) { + BuffJsonDecoder decoder = path.getValue(); + assertNull(decoder.decode("", TestNesting.class), path.getKey()); + assertNull(decoder.decode(" \n ", TestNesting.class), path.getKey()); + assertNull(decoder.decode(new byte[0], TestNesting.class), path.getKey()); + assertNull(decoder.decode(blank, TestNesting.class), path.getKey()); + assertNull(decoder.decode("{}".getBytes(java.nio.charset.StandardCharsets.UTF_8), 1, 0, TestNesting.class), + path.getKey()); + assertNull(decoder.decode(new java.io.ByteArrayInputStream(new byte[0]), TestNesting.class), path.getKey()); + assertNull(decoder.decode(new java.io.ByteArrayInputStream(blank), TestNesting.class), path.getKey()); + assertEquals(TestNesting.getDefaultInstance(), + decoder.decode( + new java.io.ByteArrayInputStream("{}".getBytes(java.nio.charset.StandardCharsets.UTF_8)), + TestNesting.class), + path.getKey()); + } + } + // ========================================================================= // Error message and valid input // ========================================================================= diff --git a/buff-json/CLAUDE.md b/buff-json/CLAUDE.md index c0cd42c..da836e4 100644 --- a/buff-json/CLAUDE.md +++ b/buff-json/CLAUDE.md @@ -141,8 +141,8 @@ Nested concrete messages use their own builders rather than parsing into Dynamic The decoder consumes untrusted JSON, so a few defenses are built into the read path. All are zero-cost on the success path. - **Strict int32/uint32 + string parsing (`FieldReader.readStrictInt32`/`readStrictUint32`/`readStrictString`)**: rather than letting fastjson2 coerce, these enforce the proto3 JSON spec so malformed input is *rejected* (a `JSONException`) instead of silently corrupting data. int32/uint32 accept an integer JSON number or a quoted integer string and reject non-integral numbers (`1.5`), out-of-range values (uint32 > 2³²−1, int32 overflow), empty/non-numeric strings, and wrong JSON types (bool/object/array); integral floats (`2.0`, `1e2`) are accepted per the spec. String fields reject any non-string token. All three decode paths use these (reflection and typed setters via `FieldReader`; codegen via `DecoderGenerator`), and because repeated/map readers call the same helpers per element, wrong-element-type arrays are rejected too. **The common path is zero-allocation**: `isNumber()` rejects bool/object/array with no read, and the bare-number value is read via the *primitive* `readDoubleValue()` — exact for the 32-bit range (`|max| < 2⁵³`), so a fractional part (`1.5`) and out-of-range are detected via `rint`/comparison with no boxing or `BigDecimal` (measured 0 B/op, same as the old lenient `readInt64Value`). Only the non-canonical *quoted* form (`"42"`) allocates (String + `BigDecimal`), which `JsonFormat` never emits for 32-bit fields. **Caveat — don't gate on `reader.isInt()`/`readInt64Value()`**: `isInt()` means "the token starts like a number," not "is integral" (it's true for `1.5`), and `readInt64Value()` silently truncates `1.5`→`1` and coerces `true`→`1`. (int64/uint64 parsing stays as-is — those tests aren't gaps; the canonical 64-bit form is a quoted string.) -- **Every reader consumes its container or throws (`FieldReader.requireObjectStart`/`requireArrayStart`)**: a message, map, `Struct`, `Any` or `Empty` value must open with `{`, and a repeated field or `ListValue` with `[`; otherwise a `JSONException` names the expected container and the proto type. This is what keeps array loops finite: previously a message reader that met a non-object token returned an empty message *without consuming it*, so `{"repeatedMsg":[1]}` (all three paths), `{"repeatedMsg":[null]}` (codegen) or a non-array repeated value spun forever, appending messages until `OutOfMemoryError`. All three paths share the helpers (`DecoderGenerator` emits calls to them), so messages and offsets match. Valid proto3 JSON always satisfies these shapes and `JsonFormat` rejects the rest, so no valid input changes behavior; the checks test a `boolean` the reader already returned (zero cost). Null *elements* in repeated fields still differ by path: codegen rejects them, the typed and reflection paths skip them. Because every element read now consumes at least its opening token, each array-loop iteration makes progress, so termination no longer depends on the element reader. A member without a name (`{"a":1, 2}`) still ends the object early, as before: the leftover token is rejected by the enclosing reader or the top-level trailing-input check, except `[{"a":1, {}}]`, which is still accepted as two elements. Tests: `BuffJsonMalformedContainerTest` (every case on all three paths under a timeout). -- **Top-level `null` rejected (`BuffJsonDecoder.readProto`)**: a bare top-level JSON `null` is not a valid message (proto3 JSON only allows `null` as a field value meaning "absent", or as a wrapped `NullValue`), so the top-level decode entry throws a `JSONException` instead of returning a null `Message` (which would NPE downstream). This is distinct from *empty input* — a `null`/empty Java `String` or `byte[]` is short-circuited by the public `decode(...)` methods to `null` as a lenient convenience, and from *field-level* `null` (handled in `readFieldsInto`, still means "absent"). Only the literal `null` payload reaches `readProto`. The fastjson2 module path (`readObject`) is unchanged. +- **Every reader consumes its container or throws (`FieldReader.requireObjectStart`/`requireArrayStart`)**: a message, map, `Struct`, `Any` or `Empty` value must open with `{`, and a repeated field or `ListValue` with `[`; otherwise a `JSONException` names the expected container and the proto type. This is what keeps array loops finite: previously a message reader that met a non-object token returned an empty message *without consuming it*, so `{"repeatedMsg":[1]}` (all three paths), `{"repeatedMsg":[null]}` (codegen) or a non-array repeated value spun forever, appending messages until `OutOfMemoryError`. All three paths share the helpers (`DecoderGenerator` emits calls to them), so messages and offsets match. Valid proto3 JSON always satisfies these shapes and `JsonFormat` rejects the rest, so no valid input changes behavior; the checks test a `boolean` the reader already returned (zero cost). Null *elements* in repeated fields still differ by path: codegen rejects them (`FieldReader.requireNonNullElement`, as `JsonFormat` does), the typed and reflection paths skip them; on every path a null element of `repeated google.protobuf.Value` / `NullValue` is kept as a value (wrapped `NullValue` / `NULL_VALUE`, via `FieldReader.nullValueFor`), since that is how the encoder writes it. Because every element read now consumes at least its opening token, each array-loop iteration makes progress, so termination no longer depends on the element reader. A member without a name (`{"a":1, 2}`) still ends the object early, as before: the leftover token is rejected by the enclosing reader or the top-level trailing-input check, except when the leftover token is itself a valid element: an unclosed object such as `[{"a":1, {}]` or `[{"a":1, {"a":2}]` is still accepted as two elements. Tests: `BuffJsonMalformedContainerTest` (every case on all three paths under a timeout). +- **Top-level `null` rejected (`BuffJsonDecoder.readProto`)**: a bare top-level JSON `null` is not a valid message (proto3 JSON only allows `null` as a field value meaning "absent", or as a wrapped `NullValue`), so the top-level decode entry throws a `JSONException` instead of returning a null `Message` (which would NPE downstream). This is distinct from *empty input* — a `null`/empty Java `String` or `byte[]`, a zero-length slice, an empty `InputStream` and whitespace-only input all decode to `null` as a lenient convenience (the public `decode(...)` methods short-circuit the obvious cases; `readProto` returns `null` when the reader starts at end of input), and from *field-level* `null` (handled in `readFieldsInto`, still means "absent"). Only the literal `null` payload reaches `readProto`. The fastjson2 module path (`readObject`) is unchanged. - **Recursion depth cap (`WellKnownTypes.MAX_RECURSION_DEPTH = 100`)**: The `Struct`/`Value`/`ListValue` reader (`readStruct`/`readListValue`/`readJsonValueImpl`) threads an `int depth` and throws a clean `JSONException` past 100 levels instead of `StackOverflowError`. 100 matches protobuf's own limit (`CodedInputStream.DEFAULT_RECURSION_LIMIT` and `JsonFormat.Parser`'s default). Public single-arg entry points (`readStruct(reader)`, etc.) delegate to private `(reader, depth)` overloads, so generated decoders keep calling the unchanged signatures — no codegen ABI change. Note: this caps the universal Struct/Value/ListValue vector; arbitrary message nesting (self-referential message types) is not capped because that would require threading depth through the `BuffJsonGeneratedDecoder` ABI. - **Any `@type`-first fast path** (`WellKnownTypes.readAny`): the canonical proto3 form lists `@type` first, so the descriptor is resolved before any content and the remaining fields are decoded straight off the live reader via `ProtobufMessageReader.readRemainingMessageFields` (regular messages → `DynamicMessage`) or direct WKT read — no `LinkedHashMap` buffering, no `JSON.toJSONString` + re-parse. The buffer-and-reparse slow path is retained only for the rare case where `@type` arrives after content. - **Any empty/missing `@type` rejected** (`WellKnownTypes.readAny`): a non-empty `Any` object whose `@type` is empty (`{"@type": "", "value": ""}`) or absent (slow path) is unresolvable, so it is rejected with a `JSONException` rather than silently yielding a default `Any` (mirrors protobuf's reference parser). Only a bare `{}` is a valid typeless empty `Any` — that case is handled before any field is read and is unaffected. Conformance: `Required.Proto3.JsonInput.AnyWktRepresentationWithEmptyTypeAndValue`. diff --git a/buff-json/src/main/java/io/suboptimal/buffjson/BuffJsonDecoder.java b/buff-json/src/main/java/io/suboptimal/buffjson/BuffJsonDecoder.java index 88da6af..d505dd8 100644 --- a/buff-json/src/main/java/io/suboptimal/buffjson/BuffJsonDecoder.java +++ b/buff-json/src/main/java/io/suboptimal/buffjson/BuffJsonDecoder.java @@ -113,6 +113,9 @@ public T decode(String json, int offset, int length, Class T decode(byte[] json, Class messageClass) { + if (json == null || json.length == 0) { + return null; + } try (JSONReader reader = JSONReader.of(json)) { return readProto(reader, messageClass); } @@ -123,6 +126,9 @@ public T decode(byte[] json, Class messageClass) { * — FastJson2 reads directly from the provided array. */ public T decode(byte[] json, int offset, int length, Class messageClass) { + if (json == null || length == 0) { + return null; + } try (JSONReader reader = JSONReader.of(json, offset, length)) { return readProto(reader, messageClass); } @@ -152,12 +158,18 @@ public ObjectReaderModule readerModule() { @SuppressWarnings("unchecked") private T readProto(JSONReader reader, Class messageClass) { + if (reader.isEnd()) { + // Empty or whitespace-only input from any overload (String, byte[], slice, + // InputStream) decodes to null, like the empty-String short-circuit, instead of + // failing the message's object-start check. + return null; + } if (reader.nextIfNull()) { // proto3 JSON: a message is never representable as a bare top-level `null` // (null is only a field value meaning "absent", or a wrapped NullValue), so - // reject it rather than returning a null Message. Empty input (a null/empty - // Java string/byte[]) is short-circuited by the public decode methods and is a - // separate, lenient convenience — only the literal `null` reaches here. + // reject it rather than returning a null Message. Empty input is handled above + // and is a separate, lenient convenience — only the literal `null` reaches + // here. throw new JSONException(reader.info("Top-level null is not a valid proto3 JSON message")); } Message defaultInstance = ProtobufMessageReader.getDefaultInstance(messageClass); diff --git a/buff-json/src/main/java/io/suboptimal/buffjson/internal/FieldReader.java b/buff-json/src/main/java/io/suboptimal/buffjson/internal/FieldReader.java index 2afd448..9eb022e 100644 --- a/buff-json/src/main/java/io/suboptimal/buffjson/internal/FieldReader.java +++ b/buff-json/src/main/java/io/suboptimal/buffjson/internal/FieldReader.java @@ -36,6 +36,9 @@ public final class FieldReader { public static final Base64.Decoder BASE64 = Base64.getDecoder(); + private static final com.google.protobuf.Value NULL_JSON_VALUE = com.google.protobuf.Value.newBuilder() + .setNullValue(com.google.protobuf.NullValue.NULL_VALUE).build(); + private FieldReader() { } @@ -341,13 +344,49 @@ public static void requireArrayStart(JSONReader reader, String kind, String name } /** - * Reads a repeated field as a JSON array, adding each element to the builder. + * Rejects a JSON {@code null} repeated-field element with a + * {@link JSONException} (proto3 JSON only allows {@code null} elements for + * {@code google.protobuf.Value} and {@code google.protobuf.NullValue}). Public + * so generated decoders (in other packages) share the same check and message. + */ + public static void requireNonNullElement(JSONReader reader, String name) { + if (reader.nextIfNull()) { + throw new JSONException(reader.info("Repeated field elements cannot be null: " + name)); + } + } + + /** + * The value a JSON {@code null} denotes for {@code fd}: a wrapped + * {@code NullValue} for {@code google.protobuf.Value}, {@code NULL_VALUE} for + * {@code google.protobuf.NullValue}, or {@code null} for every other type + * (where JSON {@code null} means "absent"). + */ + static Object nullValueFor(FieldDescriptor fd) { + if (fd.getJavaType() == FieldDescriptor.JavaType.MESSAGE + && "google.protobuf.Value".equals(fd.getMessageType().getFullName())) { + return NULL_JSON_VALUE; + } + if (fd.getJavaType() == FieldDescriptor.JavaType.ENUM + && "google.protobuf.NullValue".equals(fd.getEnumType().getFullName())) { + return fd.getEnumType().findValueByNumber(0); + } + return null; + } + + /** + * Reads a repeated field as a JSON array, adding each element to the builder. A + * {@code null} element is skipped, except for {@code Value}/{@code NullValue} + * elements, where it is a value ({@link #nullValueFor}). */ public static void readRepeated(JSONReader reader, Message.Builder builder, FieldDescriptor fd, ProtobufMessageReader msgReader) { requireArrayStart(reader, "repeated field", fd.getFullName()); + Object nullElement = nullValueFor(fd); while (!reader.nextIfArrayEnd()) { if (reader.nextIfNull()) { + if (nullElement != null) { + builder.addRepeatedField(fd, nullElement); + } continue; } Object value = readValue(reader, builder, fd, msgReader); diff --git a/buff-json/src/main/java/io/suboptimal/buffjson/internal/ProtobufMessageReader.java b/buff-json/src/main/java/io/suboptimal/buffjson/internal/ProtobufMessageReader.java index f9e5b0a..68ff7fe 100644 --- a/buff-json/src/main/java/io/suboptimal/buffjson/internal/ProtobufMessageReader.java +++ b/buff-json/src/main/java/io/suboptimal/buffjson/internal/ProtobufMessageReader.java @@ -135,18 +135,19 @@ Message readMessageRuntime(JSONReader reader, Descriptor descriptor, Message def */ Message readMessage(JSONReader reader, Message.Builder builder) { Message defaultInstance = builder.getDefaultInstanceForType(); + Descriptor descriptor = builder.getDescriptorForType(); if (useGenerated) { // Preserve discovery and descriptor-cache dispatch for mixed codec graphs. if (defaultInstance instanceof BuffJsonCodecHolder) { - return readMessage(reader, builder.getDescriptorForType(), defaultInstance); + return readMessage(reader, descriptor, defaultInstance); } - BuffJsonGeneratedDecoder decoder = GeneratedDecoderRegistry.get(builder.getDescriptorForType()); + BuffJsonGeneratedDecoder decoder = GeneratedDecoderRegistry.get(descriptor); if (decoder != null) { return decoder.readMessage(reader, this); } } - FieldReader.requireObjectStart(reader, "message", builder.getDescriptorForType().getFullName()); - readRuntimeFields(reader, builder, builder.getDescriptorForType()); + FieldReader.requireObjectStart(reader, "message", descriptor.getFullName()); + readRuntimeFields(reader, builder, descriptor); return builder.build(); } @@ -215,13 +216,9 @@ static void readNullField(Message.Builder builder, FieldDescriptor fd) { if (fd.isRepeated()) { return; } - if (fd.getJavaType() == FieldDescriptor.JavaType.MESSAGE - && "google.protobuf.Value".equals(fd.getMessageType().getFullName())) { - builder.setField(fd, com.google.protobuf.Value.newBuilder() - .setNullValue(com.google.protobuf.NullValue.NULL_VALUE).build()); - } else if (fd.getJavaType() == FieldDescriptor.JavaType.ENUM - && "google.protobuf.NullValue".equals(fd.getEnumType().getFullName())) { - builder.setField(fd, fd.getEnumType().findValueByNumber(0)); + Object value = FieldReader.nullValueFor(fd); + if (value != null) { + builder.setField(fd, value); } } diff --git a/buff-json/src/main/java/io/suboptimal/buffjson/internal/TypedMessageReaderSchema.java b/buff-json/src/main/java/io/suboptimal/buffjson/internal/TypedMessageReaderSchema.java index 356baa6..673d2ca 100644 --- a/buff-json/src/main/java/io/suboptimal/buffjson/internal/TypedMessageReaderSchema.java +++ b/buff-json/src/main/java/io/suboptimal/buffjson/internal/TypedMessageReaderSchema.java @@ -150,11 +150,15 @@ private static Parser create(FieldDescriptor fd, Class messageClass, Class return scalar; } String fieldName = fd.getFullName(); + // Null elements are skipped, except for Value/NullValue, where null is a value. + Object nullElement = FieldReader.nullValueFor(fd); return (r, b, mr) -> { FieldReader.requireArrayStart(r, "repeated field", fieldName); while (!r.nextIfArrayEnd()) { if (!r.nextIfNull()) { scalar.read(r, b, mr); + } else if (nullElement != null) { + b.addRepeatedField(fd, nullElement); } } }; From b0e72dfd4fe925ee8369f8057af6fc83f9eddda0 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 3 Oct 2026 12:30:02 +0000 Subject: [PATCH 3/3] fix(decode): keep null Value/NullValue map values on every path A null map value is a value for map (a wrapped NullValue) and map (NULL_VALUE), as in JsonFormat and as the encoder writes them. Codegen dropped such entries, and the runtime paths stored an empty Value (no kind set), so the paths disagreed and an encode/decode round trip lost data. - Generated map loops no longer skip null for Value maps (readJsonValue returns NullValue itself) and emit putXValue(key, 0) for NullValue maps. Other value types keep the existing null handling. - FieldReader.readMap and the typed map parser use nullValueFor for Value maps. - WellKnownTypes.NULL_JSON_VALUE is now the single shared NullValue Value, used by readJsonValue, packed Any and nullValueFor. - Javadoc: BuffJsonGeneratedDecoder.readMessage must consume or throw (regenerate decoders from older plugins), and BuffJsonDecoder returns null for empty input. Adds TestNullMapValues to the test protos, with a test that checks JsonFormat parity and the encoder round trip on all three paths. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_013AbdzHqfNXSdynywckLnWY --- .../buffjson/protoc/DecoderGenerator.java | 39 +++++++++++-------- buff-json-tests/CLAUDE.md | 2 +- .../src/main/protobuf/conformance_test.proto | 8 ++++ .../BuffJsonMalformedContainerTest.java | 19 +++++++++ buff-json/CLAUDE.md | 2 +- .../suboptimal/buffjson/BuffJsonDecoder.java | 8 ++++ .../buffjson/BuffJsonGeneratedDecoder.java | 14 +++++-- .../buffjson/internal/FieldReader.java | 12 +++--- .../internal/TypedMessageReaderSchema.java | 6 ++- .../buffjson/internal/WellKnownTypes.java | 10 ++++- 10 files changed, 89 insertions(+), 31 deletions(-) diff --git a/buff-json-protoc-plugin/src/main/java/io/suboptimal/buffjson/protoc/DecoderGenerator.java b/buff-json-protoc-plugin/src/main/java/io/suboptimal/buffjson/protoc/DecoderGenerator.java index e575bab..475688b 100644 --- a/buff-json-protoc-plugin/src/main/java/io/suboptimal/buffjson/protoc/DecoderGenerator.java +++ b/buff-json-protoc-plugin/src/main/java/io/suboptimal/buffjson/protoc/DecoderGenerator.java @@ -16,6 +16,7 @@ final class DecoderGenerator { private static final Set WELL_KNOWN_TYPES = BuffJsonProtocPlugin.WELL_KNOWN_TYPES; private static final String FIELD_READER = "io.suboptimal.buffjson.internal.FieldReader"; + private static final String WELL_KNOWN_TYPES_CLASS = "io.suboptimal.buffjson.internal.WellKnownTypes"; private DecoderGenerator() { } @@ -183,11 +184,20 @@ private static void generateMapFieldRead(StringBuilder sb, FieldDescriptor fd, M sb.append(indent).append(" while (!reader.nextIfObjectEnd()) {\n"); sb.append(indent).append(" String keyStr = reader.readFieldName();\n"); sb.append(indent).append(" if (keyStr == null) break;\n"); - sb.append(indent).append(" if (reader.nextIfNull()) continue;\n"); String keyExpr = mapKeyExpr(keyFd); String mapTarget = putter + "(" + keyExpr + ", "; + // proto3 JSON: a null map value is a value for Value (a wrapped NullValue, + // which readJsonValue produces itself) and for NullValue (NULL_VALUE), as in + // JsonFormat and as the encoder writes them. Other null values are skipped. + if (isNullValueField(valueFd)) { + sb.append(indent).append(" if (reader.nextIfNull()) { ").append(putter).append("Value(") + .append(keyExpr).append(", 0); continue; }\n"); + } else if (!isValueField(valueFd)) { + sb.append(indent).append(" if (reader.nextIfNull()) continue;\n"); + } + if (valueFd.getJavaType() == FieldDescriptor.JavaType.ENUM) { // Enum maps use putXxxValue(key, int) for unrecognized enum support String valuePutter = putter + "Value(" + keyExpr + ", "; @@ -281,25 +291,20 @@ private static void emitMessageRead(StringBuilder sb, FieldDescriptor fd, String String fullName = fd.getMessageType().getFullName(); if ("google.protobuf.Timestamp".equals(fullName)) { - sb.append(indent).append(prefix) - .append("(io.suboptimal.buffjson.internal.WellKnownTypes.readTimestamp(reader)").append(closeSuffix) - .append(");\n"); + sb.append(indent).append(prefix).append("(" + WELL_KNOWN_TYPES_CLASS + ".readTimestamp(reader)") + .append(closeSuffix).append(");\n"); } else if ("google.protobuf.Duration".equals(fullName)) { - sb.append(indent).append(prefix) - .append("(io.suboptimal.buffjson.internal.WellKnownTypes.readDuration(reader)").append(closeSuffix) - .append(");\n"); + sb.append(indent).append(prefix).append("(" + WELL_KNOWN_TYPES_CLASS + ".readDuration(reader)") + .append(closeSuffix).append(");\n"); } else if ("google.protobuf.Struct".equals(fullName)) { - sb.append(indent).append(prefix) - .append("(io.suboptimal.buffjson.internal.WellKnownTypes.readStruct(reader)").append(closeSuffix) - .append(");\n"); + sb.append(indent).append(prefix).append("(" + WELL_KNOWN_TYPES_CLASS + ".readStruct(reader)") + .append(closeSuffix).append(");\n"); } else if ("google.protobuf.Value".equals(fullName)) { - sb.append(indent).append(prefix) - .append("(io.suboptimal.buffjson.internal.WellKnownTypes.readJsonValue(reader)").append(closeSuffix) - .append(");\n"); + sb.append(indent).append(prefix).append("(" + WELL_KNOWN_TYPES_CLASS + ".readJsonValue(reader)") + .append(closeSuffix).append(");\n"); } else if ("google.protobuf.ListValue".equals(fullName)) { - sb.append(indent).append(prefix) - .append("(io.suboptimal.buffjson.internal.WellKnownTypes.readListValue(reader)").append(closeSuffix) - .append(");\n"); + sb.append(indent).append(prefix).append("(" + WELL_KNOWN_TYPES_CLASS + ".readListValue(reader)") + .append(closeSuffix).append(");\n"); } else if ("google.protobuf.Empty".equals(fullName)) { sb.append(indent).append(FIELD_READER) .append(".requireObjectStart(reader, \"message\", \"google.protobuf.Empty\");\n"); @@ -312,7 +317,7 @@ private static void emitMessageRead(StringBuilder sb, FieldDescriptor fd, String } else if (WELL_KNOWN_TYPES.contains(fullName)) { String msgJavaClass = protoToJavaClass.get(fullName); sb.append(indent).append(prefix).append("((").append(msgJavaClass) - .append(") io.suboptimal.buffjson.internal.WellKnownTypes.readWkt(reader, ").append(msgJavaClass) + .append(") " + WELL_KNOWN_TYPES_CLASS + ".readWkt(reader, ").append(msgJavaClass) .append(".getDescriptor(), msgReader)").append(closeSuffix).append(");\n"); } else { String decoderClass = protoToDecoderClass.get(fullName); diff --git a/buff-json-tests/CLAUDE.md b/buff-json-tests/CLAUDE.md index c2ccbcc..2f48bdb 100644 --- a/buff-json-tests/CLAUDE.md +++ b/buff-json-tests/CLAUDE.md @@ -12,7 +12,7 @@ pure reflection). - `BuffJsonReferenceTest.java` — 5 smoke tests (scalar, default, complex, plus two `DynamicMessage` tests on UTF-16 and UTF-8 paths — `DynamicMessage` is the only thing that exclusively exercises pure reflection in production) - `BuffJsonEncodingRegressionTest.java` — escaped custom names compile in both generated codecs, round-trip through all encoder paths (UTF-16/UTF-8) and both decoders, and accept proto-name aliases. Concrete and actual DynamicMessage WKTs share output/range validation. Deprecated-field fixtures and unsigned key/digit boundaries live in the main conformance tests. - `BuffJsonMemoryTest.java` — 8 reachability tests using `WeakReference` + `System.gc()` to confirm the encoder doesn't retain `Message` references after `encode`/`encodeToBytes`/`encode(stream)` on any of the three paths, including `DynamicMessage`. Steady-state allocation regressions are caught separately by `./allocation-check.sh` in CI (JMH `-prof gc`). -- `BuffJsonMalformedContainerTest.java` — untrusted-input regression tests for mismatched containers, each case on **all three decode paths** inside `assertTimeoutPreemptively` (a regression fails at the timeout instead of hanging the build; the stuck thread can't be stopped, so the fork may still OOM afterwards): non-object repeated message/Struct/ListValue/Any/Empty elements, non-array repeated values, non-object map and singular message values (including top-level), malformed objects inside containers, and truncated input and null repeated message elements (termination only). Null scalar/WKT repeated elements are rejected with a `JSONException` on codegen (they used to NPE or add a phantom default) and skipped on the runtime paths; null `repeated Value` elements are kept as `NullValue` on every path (matches `JsonFormat` and round-trips); empty input (empty/whitespace-only `String`, `byte[]`, slice or `InputStream`) decodes to `null` from every overload. Uses `TestNesting` and the official `TestAllTypesProto3`, which has repeated fields of every WKT. +- `BuffJsonMalformedContainerTest.java` — untrusted-input regression tests for mismatched containers, each case on **all three decode paths** inside `assertTimeoutPreemptively` (a regression fails at the timeout instead of hanging the build; the stuck thread can't be stopped, so the fork may still OOM afterwards): non-object repeated message/Struct/ListValue/Any/Empty elements, non-array repeated values, non-object map and singular message values (including top-level), malformed objects inside containers, and truncated input and null repeated message elements (termination only). Null scalar/WKT repeated elements are rejected with a `JSONException` on codegen (they used to NPE or add a phantom default) and skipped on the runtime paths; null `repeated Value` elements are kept as `NullValue` on every path (matches `JsonFormat` and round-trips), as are null `map` / `map` values (`TestNullMapValues`); empty input (empty/whitespace-only `String`, `byte[]`, slice or `InputStream`) decodes to `null` from every overload. Uses `TestNesting` and the official `TestAllTypesProto3`, which has repeated fields of every WKT. - `BuffJsonCrossPathFuzzTest.java` — seeded-random (reproducible) fuzzer over `TestAllTypesProto3`. `encodePathsAgreeAndAreParseable` asserts **codegen == typed == reflection** byte-for-byte (UTF-16 and UTF-8) over 500 messages — the direct "the three paths agree" guarantee — plus a buff-json self round-trip. `decodePathsRoundTrip` asserts both decode paths reconstruct messages from `JsonFormat`-printed JSON. (It does not byte-compare encode output against `JsonFormat` because fastjson2 and protobuf may format the same float/double differently — both round-trip to the same value; curated byte-equality lives in `BuffJsonProto3ConformanceTest`.) - `BuffJsonProto3ConformanceTest.java` — proto3 JSON **encode** coverage in nested classes, each `assertMatchesReference` validating all three paths (codegen, typed-accessor, reflection) byte-for-byte against `JsonFormat`; well-known-type groups also carry out-of-range **edge cases** asserting all three paths reject identically: - ScalarTypes (13): all types, boundaries, NaN, Infinity, -0.0, unicode, escapes, bytes diff --git a/buff-json-tests/src/main/protobuf/conformance_test.proto b/buff-json-tests/src/main/protobuf/conformance_test.proto index cacb4ef..49f8707 100644 --- a/buff-json-tests/src/main/protobuf/conformance_test.proto +++ b/buff-json-tests/src/main/protobuf/conformance_test.proto @@ -168,6 +168,14 @@ message TestRepeatedNullValue { repeated google.protobuf.NullValue values = 1; } +// Map values of google.protobuf.Value / NullValue: a JSON null value is a value +// (wrapped NullValue / NULL_VALUE), not an absent entry. Exercises the decoders' +// null map-value branches. +message TestNullMapValues { + map values = 1; + map nulls = 2; +} + // Proto3 explicit presence (optional keyword) message TestOptionalFields { optional int32 optional_int32 = 1; diff --git a/buff-json-tests/src/test/java/io/suboptimal/buffjson/BuffJsonMalformedContainerTest.java b/buff-json-tests/src/test/java/io/suboptimal/buffjson/BuffJsonMalformedContainerTest.java index 8246678..6f08a78 100644 --- a/buff-json-tests/src/test/java/io/suboptimal/buffjson/BuffJsonMalformedContainerTest.java +++ b/buff-json-tests/src/test/java/io/suboptimal/buffjson/BuffJsonMalformedContainerTest.java @@ -282,6 +282,25 @@ void nullRepeatedNullValueElementsArePreservedOnEveryPath() throws Exception { } } + @Test + void nullMapValuesOfValueAndNullValueArePreservedOnEveryPath() throws Exception { + var nullValue = com.google.protobuf.Value.newBuilder().setNullValue(com.google.protobuf.NullValue.NULL_VALUE) + .build(); + var expected = io.suboptimal.buffjson.proto.TestNullMapValues.newBuilder().putValues("a", nullValue) + .putValues("b", com.google.protobuf.Value.newBuilder().setNumberValue(1).build()) + .putNulls("c", com.google.protobuf.NullValue.NULL_VALUE).build(); + String json = "{\"values\":{\"a\":null,\"b\":1},\"nulls\":{\"c\":null}}"; + var reference = io.suboptimal.buffjson.proto.TestNullMapValues.newBuilder(); + com.google.protobuf.util.JsonFormat.parser().merge(json, reference); + assertEquals(expected, reference.build()); + for (var path : paths().entrySet()) { + assertEquals(expected, path.getValue().decode(json, io.suboptimal.buffjson.proto.TestNullMapValues.class), + path.getKey()); + assertEquals(expected, path.getValue().decode(BuffJson.encoder().encode(expected), + io.suboptimal.buffjson.proto.TestNullMapValues.class), path.getKey() + " round trip"); + } + } + /** * Empty input decodes to {@code null} from every overload, instead of failing * the new object-start check (an empty {@code InputStream} or whitespace-only diff --git a/buff-json/CLAUDE.md b/buff-json/CLAUDE.md index da836e4..e910f52 100644 --- a/buff-json/CLAUDE.md +++ b/buff-json/CLAUDE.md @@ -141,7 +141,7 @@ Nested concrete messages use their own builders rather than parsing into Dynamic The decoder consumes untrusted JSON, so a few defenses are built into the read path. All are zero-cost on the success path. - **Strict int32/uint32 + string parsing (`FieldReader.readStrictInt32`/`readStrictUint32`/`readStrictString`)**: rather than letting fastjson2 coerce, these enforce the proto3 JSON spec so malformed input is *rejected* (a `JSONException`) instead of silently corrupting data. int32/uint32 accept an integer JSON number or a quoted integer string and reject non-integral numbers (`1.5`), out-of-range values (uint32 > 2³²−1, int32 overflow), empty/non-numeric strings, and wrong JSON types (bool/object/array); integral floats (`2.0`, `1e2`) are accepted per the spec. String fields reject any non-string token. All three decode paths use these (reflection and typed setters via `FieldReader`; codegen via `DecoderGenerator`), and because repeated/map readers call the same helpers per element, wrong-element-type arrays are rejected too. **The common path is zero-allocation**: `isNumber()` rejects bool/object/array with no read, and the bare-number value is read via the *primitive* `readDoubleValue()` — exact for the 32-bit range (`|max| < 2⁵³`), so a fractional part (`1.5`) and out-of-range are detected via `rint`/comparison with no boxing or `BigDecimal` (measured 0 B/op, same as the old lenient `readInt64Value`). Only the non-canonical *quoted* form (`"42"`) allocates (String + `BigDecimal`), which `JsonFormat` never emits for 32-bit fields. **Caveat — don't gate on `reader.isInt()`/`readInt64Value()`**: `isInt()` means "the token starts like a number," not "is integral" (it's true for `1.5`), and `readInt64Value()` silently truncates `1.5`→`1` and coerces `true`→`1`. (int64/uint64 parsing stays as-is — those tests aren't gaps; the canonical 64-bit form is a quoted string.) -- **Every reader consumes its container or throws (`FieldReader.requireObjectStart`/`requireArrayStart`)**: a message, map, `Struct`, `Any` or `Empty` value must open with `{`, and a repeated field or `ListValue` with `[`; otherwise a `JSONException` names the expected container and the proto type. This is what keeps array loops finite: previously a message reader that met a non-object token returned an empty message *without consuming it*, so `{"repeatedMsg":[1]}` (all three paths), `{"repeatedMsg":[null]}` (codegen) or a non-array repeated value spun forever, appending messages until `OutOfMemoryError`. All three paths share the helpers (`DecoderGenerator` emits calls to them), so messages and offsets match. Valid proto3 JSON always satisfies these shapes and `JsonFormat` rejects the rest, so no valid input changes behavior; the checks test a `boolean` the reader already returned (zero cost). Null *elements* in repeated fields still differ by path: codegen rejects them (`FieldReader.requireNonNullElement`, as `JsonFormat` does), the typed and reflection paths skip them; on every path a null element of `repeated google.protobuf.Value` / `NullValue` is kept as a value (wrapped `NullValue` / `NULL_VALUE`, via `FieldReader.nullValueFor`), since that is how the encoder writes it. Because every element read now consumes at least its opening token, each array-loop iteration makes progress, so termination no longer depends on the element reader. A member without a name (`{"a":1, 2}`) still ends the object early, as before: the leftover token is rejected by the enclosing reader or the top-level trailing-input check, except when the leftover token is itself a valid element: an unclosed object such as `[{"a":1, {}]` or `[{"a":1, {"a":2}]` is still accepted as two elements. Tests: `BuffJsonMalformedContainerTest` (every case on all three paths under a timeout). +- **Every reader consumes its container or throws (`FieldReader.requireObjectStart`/`requireArrayStart`)**: a message, map, `Struct`, `Any` or `Empty` value must open with `{`, and a repeated field or `ListValue` with `[`; otherwise a `JSONException` names the expected container and the proto type. This is what keeps array loops finite: previously a message reader that met a non-object token returned an empty message *without consuming it*, so `{"repeatedMsg":[1]}` (all three paths), `{"repeatedMsg":[null]}` (codegen) or a non-array repeated value spun forever, appending messages until `OutOfMemoryError`. All three paths share the helpers (`DecoderGenerator` emits calls to them), so messages and offsets match. Valid proto3 JSON always satisfies these shapes and `JsonFormat` rejects the rest, so no valid input changes behavior; the checks test a `boolean` the reader already returned (zero cost). Null *elements* in repeated fields still differ by path: codegen rejects them (`FieldReader.requireNonNullElement`, as `JsonFormat` does), the typed and reflection paths skip them; on every path a null element of `repeated google.protobuf.Value` / `NullValue` is kept as a value (wrapped `NullValue` / `NULL_VALUE`, via `FieldReader.nullValueFor`), since that is how the encoder writes it; the same holds for a null *map value* of those types (codegen used to drop the entry and the runtime paths stored an empty `Value`), while other null map values are still skipped (codegen) or stored as the type's default (runtime). Because every element read now consumes at least its opening token, each array-loop iteration makes progress, so termination no longer depends on the element reader. A member without a name (`{"a":1, 2}`) still ends the object early, as before: the leftover token is rejected by the enclosing reader or the top-level trailing-input check, except when the leftover token is itself a valid element: an unclosed object such as `[{"a":1, {}]` or `[{"a":1, {"a":2}]` is still accepted as two elements. Tests: `BuffJsonMalformedContainerTest` (every case on all three paths under a timeout). - **Top-level `null` rejected (`BuffJsonDecoder.readProto`)**: a bare top-level JSON `null` is not a valid message (proto3 JSON only allows `null` as a field value meaning "absent", or as a wrapped `NullValue`), so the top-level decode entry throws a `JSONException` instead of returning a null `Message` (which would NPE downstream). This is distinct from *empty input* — a `null`/empty Java `String` or `byte[]`, a zero-length slice, an empty `InputStream` and whitespace-only input all decode to `null` as a lenient convenience (the public `decode(...)` methods short-circuit the obvious cases; `readProto` returns `null` when the reader starts at end of input), and from *field-level* `null` (handled in `readFieldsInto`, still means "absent"). Only the literal `null` payload reaches `readProto`. The fastjson2 module path (`readObject`) is unchanged. - **Recursion depth cap (`WellKnownTypes.MAX_RECURSION_DEPTH = 100`)**: The `Struct`/`Value`/`ListValue` reader (`readStruct`/`readListValue`/`readJsonValueImpl`) threads an `int depth` and throws a clean `JSONException` past 100 levels instead of `StackOverflowError`. 100 matches protobuf's own limit (`CodedInputStream.DEFAULT_RECURSION_LIMIT` and `JsonFormat.Parser`'s default). Public single-arg entry points (`readStruct(reader)`, etc.) delegate to private `(reader, depth)` overloads, so generated decoders keep calling the unchanged signatures — no codegen ABI change. Note: this caps the universal Struct/Value/ListValue vector; arbitrary message nesting (self-referential message types) is not capped because that would require threading depth through the `BuffJsonGeneratedDecoder` ABI. - **Any `@type`-first fast path** (`WellKnownTypes.readAny`): the canonical proto3 form lists `@type` first, so the descriptor is resolved before any content and the remaining fields are decoded straight off the live reader via `ProtobufMessageReader.readRemainingMessageFields` (regular messages → `DynamicMessage`) or direct WKT read — no `LinkedHashMap` buffering, no `JSON.toJSONString` + re-parse. The buffer-and-reparse slow path is retained only for the rare case where `@type` arrives after content. diff --git a/buff-json/src/main/java/io/suboptimal/buffjson/BuffJsonDecoder.java b/buff-json/src/main/java/io/suboptimal/buffjson/BuffJsonDecoder.java index d505dd8..a953190 100644 --- a/buff-json/src/main/java/io/suboptimal/buffjson/BuffJsonDecoder.java +++ b/buff-json/src/main/java/io/suboptimal/buffjson/BuffJsonDecoder.java @@ -24,6 +24,14 @@ * MyMessage msg = decoder.decode(inputStream, MyMessage.class); * } * + *

Empty input

+ * + * The {@code decode} overloads return {@code null} (not a default message) for + * empty input: a {@code null} or empty {@code String} or {@code byte[]}, a + * zero-length slice, whitespace-only text, or an empty {@link InputStream}. A + * literal JSON {@code null} is rejected with a {@link JSONException}, as is any + * other non-object value. + * *

Thread-safety

* * Once configured, a decoder is safe to share across threads: each diff --git a/buff-json/src/main/java/io/suboptimal/buffjson/BuffJsonGeneratedDecoder.java b/buff-json/src/main/java/io/suboptimal/buffjson/BuffJsonGeneratedDecoder.java index 95877cc..40efb0b 100644 --- a/buff-json/src/main/java/io/suboptimal/buffjson/BuffJsonGeneratedDecoder.java +++ b/buff-json/src/main/java/io/suboptimal/buffjson/BuffJsonGeneratedDecoder.java @@ -25,9 +25,17 @@ public interface BuffJsonGeneratedDecoder { * NOT have consumed the opening '{' — this method reads the full JSON object * including braces. * - * @param msgReader - * the message reader carrying settings (typeRegistry, useGenerated) - * for recursive nested message reads + *

+ * If the current token is not {@code '{'} (including JSON {@code null}), + * implementations must throw a {@code JSONException} (generated decoders call + * {@code FieldReader.requireObjectStart}) and must never return without + * consuming input: the runtime's repeated-field loops rely on every element + * read making progress, and terminate only because of it. Decoders generated by + * an older plugin version ignored a missing {@code '{'}, so regenerate them + * when upgrading. + * + * @param msgReader the message reader carrying settings (typeRegistry, + * useGenerated) for recursive nested message reads */ T readMessage(JSONReader reader, ProtobufMessageReader msgReader); } diff --git a/buff-json/src/main/java/io/suboptimal/buffjson/internal/FieldReader.java b/buff-json/src/main/java/io/suboptimal/buffjson/internal/FieldReader.java index 9eb022e..3d22afd 100644 --- a/buff-json/src/main/java/io/suboptimal/buffjson/internal/FieldReader.java +++ b/buff-json/src/main/java/io/suboptimal/buffjson/internal/FieldReader.java @@ -36,9 +36,6 @@ public final class FieldReader { public static final Base64.Decoder BASE64 = Base64.getDecoder(); - private static final com.google.protobuf.Value NULL_JSON_VALUE = com.google.protobuf.Value.newBuilder() - .setNullValue(com.google.protobuf.NullValue.NULL_VALUE).build(); - private FieldReader() { } @@ -364,7 +361,7 @@ public static void requireNonNullElement(JSONReader reader, String name) { static Object nullValueFor(FieldDescriptor fd) { if (fd.getJavaType() == FieldDescriptor.JavaType.MESSAGE && "google.protobuf.Value".equals(fd.getMessageType().getFullName())) { - return NULL_JSON_VALUE; + return WellKnownTypes.NULL_JSON_VALUE; } if (fd.getJavaType() == FieldDescriptor.JavaType.ENUM && "google.protobuf.NullValue".equals(fd.getEnumType().getFullName())) { @@ -395,7 +392,9 @@ public static void readRepeated(JSONReader reader, Message.Builder builder, Fiel } /** - * Reads a map field as a JSON object, adding entries to the builder. + * Reads a map field as a JSON object, adding entries to the builder. A + * {@code null} value is a value for {@code Value}/{@code NullValue} map values + * ({@link #nullValueFor}), and the value type's default otherwise. */ public static void readMap(JSONReader reader, Message.Builder builder, FieldDescriptor fd, ProtobufMessageReader msgReader) { @@ -415,7 +414,8 @@ public static void readMap(JSONReader reader, Message.Builder builder, FieldDesc Message.Builder entryBuilder = builder.newBuilderForField(fd); Object value; if (reader.nextIfNull()) { - value = getDefaultMapValue(valueFd); + Object nullValue = nullValueFor(valueFd); + value = nullValue != null ? nullValue : getDefaultMapValue(valueFd); } else { value = readValue(reader, entryBuilder, valueFd, msgReader); } diff --git a/buff-json/src/main/java/io/suboptimal/buffjson/internal/TypedMessageReaderSchema.java b/buff-json/src/main/java/io/suboptimal/buffjson/internal/TypedMessageReaderSchema.java index 673d2ca..77cf16f 100644 --- a/buff-json/src/main/java/io/suboptimal/buffjson/internal/TypedMessageReaderSchema.java +++ b/buff-json/src/main/java/io/suboptimal/buffjson/internal/TypedMessageReaderSchema.java @@ -180,6 +180,10 @@ private static Parser createMap(FieldDescriptor fd, String suffix, Class mess : valueFd.getJavaType() == FieldDescriptor.JavaType.MESSAGE ? ProtobufMessageReader.getDefaultInstance(valueClass) : FieldReader.getDefaultMapValue(valueFd); + // A null Value map value is a wrapped NullValue; for NullValue maps the + // default (0) already is NULL_VALUE, and the enum setter takes the number. + Object nullJsonValue = enumValue ? null : FieldReader.nullValueFor(valueFd); + Object nullValue = nullJsonValue != null ? nullJsonValue : defaultValue; String fieldName = fd.getFullName(); return (r, b, mr) -> { FieldReader.requireObjectStart(r, "map field", fieldName); @@ -189,7 +193,7 @@ private static Parser createMap(FieldDescriptor fd, String suffix, Class mess break; } Object key = FieldReader.parseMapKey(r, keyText, keyFd); - Object value = r.nextIfNull() ? defaultValue : parser.read(r, mr); + Object value = r.nextIfNull() ? nullValue : parser.read(r, mr); setter.invokeExact(b, key, value); } }; diff --git a/buff-json/src/main/java/io/suboptimal/buffjson/internal/WellKnownTypes.java b/buff-json/src/main/java/io/suboptimal/buffjson/internal/WellKnownTypes.java index ae0909d..957fe3a 100644 --- a/buff-json/src/main/java/io/suboptimal/buffjson/internal/WellKnownTypes.java +++ b/buff-json/src/main/java/io/suboptimal/buffjson/internal/WellKnownTypes.java @@ -628,6 +628,12 @@ private static String snakeToCamel(String snake) { */ private static final int MAX_RECURSION_DEPTH = 100; + /** + * The {@code google.protobuf.Value} a JSON {@code null} denotes. Immutable, so + * shared by every reader (Value elements, fields, map values, packed Any). + */ + static final Value NULL_JSON_VALUE = Value.newBuilder().setNullValue(NullValue.NULL_VALUE).build(); + private static void checkDepth(JSONReader reader, int depth) { if (depth > MAX_RECURSION_DEPTH) { throw new JSONException(reader.info("JSON nesting depth exceeds " + MAX_RECURSION_DEPTH)); @@ -822,7 +828,7 @@ public static Value readJsonValue(JSONReader reader) { private static Value readJsonValueImpl(JSONReader reader, int depth) { if (reader.nextIfNull()) { - return Value.newBuilder().setNullValue(NullValue.NULL_VALUE).build(); + return NULL_JSON_VALUE; } if (reader.isString()) { return Value.newBuilder().setStringValue(reader.readString()).build(); @@ -998,7 +1004,7 @@ private static Message readPackedWktValue(JSONReader reader, Descriptor type, Pr */ private static Message nullPackedWktValue(Descriptor type) { if ("google.protobuf.Value".equals(type.getFullName())) { - return Value.newBuilder().setNullValue(NullValue.NULL_VALUE).build(); + return NULL_JSON_VALUE; } return DynamicMessage.getDefaultInstance(type); }