fix(jsonpb): eliminate non-deterministic behaviour in unmarshalling#161
Open
aarmoa wants to merge 1 commit intocosmos:mainfrom
Open
fix(jsonpb): eliminate non-deterministic behaviour in unmarshalling#161aarmoa wants to merge 1 commit intocosmos:mainfrom
aarmoa wants to merge 1 commit intocosmos:mainfrom
Conversation
Two independent sources of non-determinism existed in the JSON
unmarshaller, both caused by iterating over Go maps whose key order is
randomised per program execution.
**Oneof conflict (critical)**
`sprops.OneofTypes` is a `map[string]*OneofProperties`. When the JSON
input contains multiple keys that belong to the same oneof group (e.g.
`{"title":"foo","salary":31000}` for `MsgWithOneof.union`), every
matched alternative overwrote the same interface field via
`target.Field(oop.Field).Set(nv)`. The winner depended on which key the
runtime happened to visit last, making the decoded value non-
deterministic.
The fix collects the map keys, sorts them with `slices.Sort`, and tracks
which oneof interface fields have been set. If a second key from the same
group is encountered an error is returned, consistent with the behaviour
of the text-format parser (`text_parser.go:589`) and the proto oneof
contract (at most one field may be set at a time).
**Extension map iteration (low)**
`proto.RegisteredExtensions` returns a `map[int32]*ExtensionDesc`. The
unmarshaller iterated it without sorting. Extension names are unique, so
this did not produce an incorrect result, but it was structurally
inconsistent with the marshalling side (lines 401-408) which explicitly
sorts extension IDs for stable output. The fix applies the same pattern:
collect IDs, sort with `sort.Sort(int32Slice(extIDs))`, then iterate.
**Tests**
Following TDD, the tests were added first and confirmed to fail before
the logic changes were applied:
- Two new entries in `unmarshalingShouldError` cover the oneof-conflict
cases (orig_name and camelCase variants). `TestUnmarshalingBadInput`
now asserts that both inputs produce a non-nil error.
- `TestUnmarshalOneofConflictDeterminism` runs the same conflicting JSON
through the unmarshaller 100 times and asserts only one unique outcome
is observed, directly reproducing the non-determinism that existed
before the fix.
aarmoa
added a commit
to InjectiveLabs/gogoproto
that referenced
this pull request
Mar 19, 2026
- Sort oneof keys and error on multiple fields in same oneof (critical). - Sort extension IDs before iterating (consistency with marshal). - Add unmarshalingShouldError cases and TestUnmarshalOneofConflictDeterminism. Ref: cosmos#161
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
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.
Two independent sources of non-determinism existed in the JSON unmarshaller, both caused by iterating over Go maps whose key order is randomised per program execution.
Oneof conflict (critical)
sprops.OneofTypesis amap[string]*OneofProperties. When the JSON input contains multiple keys that belong to the same oneof group (e.g.{"title":"foo","salary":31000}forMsgWithOneof.union), every matched alternative overwrote the same interface field viatarget.Field(oop.Field).Set(nv). The winner depended on which key the runtime happened to visit last, making the decoded value non- deterministic.The fix collects the map keys, sorts them with
slices.Sort, and tracks which oneof interface fields have been set. If a second key from the same group is encountered an error is returned, consistent with the behaviour of the text-format parser (text_parser.go:589) and the proto oneof contract (at most one field may be set at a time).Extension map iteration (low)
proto.RegisteredExtensionsreturns amap[int32]*ExtensionDesc. The unmarshaller iterated it without sorting. Extension names are unique, so this did not produce an incorrect result, but it was structurally inconsistent with the marshalling side (lines 401-408) which explicitly sorts extension IDs for stable output. The fix applies the same pattern: collect IDs, sort withsort.Sort(int32Slice(extIDs)), then iterate.Tests
Following TDD, the tests were added first and confirmed to fail before the logic changes were applied:
unmarshalingShouldErrorcover the oneof-conflict cases (orig_name and camelCase variants).TestUnmarshalingBadInputnow asserts that both inputs produce a non-nil error.TestUnmarshalOneofConflictDeterminismruns the same conflicting JSON through the unmarshaller 100 times and asserts only one unique outcome is observed, directly reproducing the non-determinism that existed before the fix.