Conversation
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013AbdzHqfNXSdynywckLnWY
Fyzu
force-pushed
the
claude/vigilant-archimedes-ygynf3
branch
from
September 28, 2026 23:53
b2e4ee6 to
f56e58e
Compare
Contributor
Performance comparisonWorkflow and raw JMH artifacts Java 21 performanceBase: ad10fe5 → candidate: 867e35b Throughput alerts are advisory. Existing allocation budgets are enforced separately.
Java 25 performanceBase: ad10fe5 → candidate: 867e35b Throughput alerts are advisory. Existing allocation budgets are enforced separately.
|
… 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013AbdzHqfNXSdynywckLnWY
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A tiny untrusted payload can make decoding loop until
OutOfMemoryError:When a message reader met a non-object token, it returned an empty message without consuming the token. It ignored
nextIfObjectStart()'s result, andreadFieldName()returnsnullon a non-name token, which ended the field loop. Inside a repeated field, the enclosingwhile (!reader.nextIfArrayEnd())loop then saw the same token forever, appending messages until the heap was exhausted.Inputs that looped before this change (
-Xmx96m, 8 s timeout):{"repeatedNested":[1]},[true],[[]],[{..},2]{"repeatedNested":[null]}{"repeatedNested":5},{"repeatedNested":{}}(not an array){"repeatedStruct":[1]},[[1]]{"repeatedStruct":[null]}{"repeatedEmpty":[1]},[[]]Singular and map values were rejected only by accident: the leftover token made the top-level "input not end" check fail. The official conformance cases for null repeated elements appeared to pass only because
ConformanceTesteecatchesThrowable, which reported theOutOfMemoryErroras a parse failure.Fix
1. Every reader consumes the container it expects, or throws a
JSONExceptionthat names the proto type:{for messages, maps,Struct,AnyandEmpty;[for repeated fields andListValue.The checks live in two helpers,
FieldReader.requireObjectStartandrequireArrayStart, and the generated decoders call them too. So all three paths produce the same error, for exampleExpected a JSON object for message io.suboptimal.buffjson.proto.NestedMessage, offset 32, …. Changed code:DecoderGenerator— message, repeated, map and inlineEmptyreadsProtobufMessageReader,TypedMessageReaderSchema,FieldReader.readRepeated/readMapWellKnownTypes.readStruct/readListValue/readAnyEvery element read now consumes at least its opening token, so each iteration of an array loop makes progress. Termination no longer depends on what the element reader does. The checks test a
booleanthe reader already returned, so there is no cost on the success path.2. Null elements in repeated fields, found by the same test matrix:
nullelement with aJSONException(FieldReader.requireNonNullElement), asJsonFormatdoes. Before, it threwNullPointerException(Timestamp, Duration, FieldMask, String/BytesValue) or added a phantom default element (int64, bool, enum, wrappers).google.protobuf.Value/NullValueelements keepnullas a value on every path (a wrappedNullValue, orNULL_VALUE). The typed and reflection paths used to drop them, so{"repeatedValue":[null,1]}lost an element, even though that is exactly what the encoder writes.3. Empty input. Empty or whitespace-only input decodes to
nullfrom every overload (byte[], slice,InputStream), like an emptyString. Without this, the new object-start check would have turned these inputs into exceptions.Behavior changes (invalid input only)
JSONExceptioninstead of looping or failing on the trailing-input check. Valid proto3 JSON always uses these shapes, andJsonFormatrejects everything else.Empty: a bare number or array forgoogle.protobuf.Emptyis now rejected, matching the runtime paths.byte[]/InputStreamor whitespace-only input now returnsnull. Onmainit returned an empty message; an emptyStringalready returnednull.Tests
New
BuffJsonMalformedContainerTest: each case runs on all three decode paths insideassertTimeoutPreemptively. It covers:repeated Valueandrepeated NullValuenulls, checked againstJsonFormatplus a round-trip;Against the unfixed code, the forked test JVM died with
Java heap space.New
TestRepeatedNullValuetest message: added to the test protos so the generatedNullValuebranch is compiled and exercised.mvn -B clean verify: all modules pass (729 tests inbuff-json-tests), including the benchmark and conformance-testee builds.Not run locally: the official
conformance_test_runner. CI runs it for all threeBUFFJSON_PATHs.Consumers must rebuild with
mvn clean installto regenerate decoders. The protobuf Maven plugin only regenerates when.protoinputs change.Not changed (possible follow-ups)
Valuerepeated fields are still skipped by the typed and reflection paths, while codegen rejects them.JsonFormat.runtimeNullMapValuesAndUnknownEnumNumbersMatchReflectionpins the runtime behavior, so this needs a decision on which is intended.[{"a":1, {"a":2}]is still accepted as two elements. The existing truncated-object leniency (BuffJsonErrorTest.truncatedObjectParsesLeniently) is unchanged.assertTimeoutPreemptivelycan't stop a runaway decode thread. If the fix regresses, the test fails at the timeout, but the fork may still run out of memory afterwards.🤖 Generated with Claude Code
https://claude.ai/code/session_013AbdzHqfNXSdynywckLnWY