Skip to content

fix(protobuf): reject cyclic collections when decoding Struct/Value - #309

Open
Amayyas wants to merge 1 commit into
googleapis:mainfrom
Amayyas:protobuf-cyclic-decode
Open

fix(protobuf): reject cyclic collections when decoding Struct/Value#309
Amayyas wants to merge 1 commit into
googleapis:mainfrom
Amayyas:protobuf-cyclic-decode

Conversation

@Amayyas

@Amayyas Amayyas commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

Decoding a cyclic Dart collection recursed until the stack overflowed:

final map = <String, Object?>{};
map['x'] = map;
Value.fromJson(map); // StackOverflowError

Value, Struct and ListValue decode into each other, so the decoding functions now thread the set of containers currently being decoded through the recursion, and throw a FormatException when one is re-entered.

Notes

  • Lists have the same problem, as do cycles that span both (a map holding a list that holds the map). Those are covered too, since the fix sits on the shared path.
  • FormatException was chosen for consistency with the existing behaviour for other undecodable input — Value.fromJson(DateTime.now()) already throws it. Happy to switch to ArgumentError if you'd rather treat it as a caller-side mistake.
  • Repeated-but-acyclic input still decodes. Entries leave the set once decoded, so only ancestors are rejected — e.g. the same map referenced by two keys is fine. Covered by tests.
  • Only decoding is affected. Struct.fields is final, so a cyclic object graph cannot be constructed in the first place and encoding cannot recurse.

dart format, dart analyze, dart test (180 passing) and the protojson conformance suite (211 passing) are all clean.

Fixes #285

Decoding a cyclic Dart collection recursed until the stack overflowed:

    final map = <String, Object?>{};
    map['x'] = map;
    Value.fromJson(map); // StackOverflowError

`Value`, `Struct` and `ListValue` decode into each other, so the decoding
functions now thread the set of containers currently being decoded through
the recursion and throw a `FormatException` when one is re-entered. This
matches the existing behaviour for other undecodable input, such as
`Value.fromJson(DateTime.now())`.

Lists are affected in the same way as maps, as are cycles that span both
(a map holding a list that holds the map), so all of them are covered.

A collection that is merely repeated rather than cyclic, such as the same
map referenced by two keys, still decodes: entries are removed from the set
once decoded, so only ancestors are rejected.

Only decoding is affected. `Struct.fields` is final, so a cyclic object
graph cannot be built in the first place and encoding cannot recurse.

Fixes googleapis#285

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request adds unit tests to verify the behavior of JSON decoding for cyclic and repeated acyclic structures in Struct, ListValue, and Value classes. Specifically, it introduces tests to ensure that cyclic maps and lists are rejected with a FormatException, while repeated but acyclic references are successfully decoded. There are no review comments, and I have no additional feedback to provide.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

_StructHelper.decode should deal with cyclic maps

1 participant