Give a nested stringref namespace its own index space - #335
Open
dylanpulver wants to merge 2 commits into
Open
Conversation
encode_semantic() turned string_referencing on for tag 256 but left the outer
string_references/bytes_references maps in place -- the two TODOs in that
function marked exactly this. So references emitted inside a nested namespace
were numbered against the outer namespace, and strings registered inside it
leaked back out afterwards. The decoder already does this correctly: it pushes
a fresh namespace and resolves tag 25 against the innermost one.
The result is that the encoder produces output the decoder cannot read, and
sometimes reads as different data:
>>> v = ["aaa", CBORTag(256, ["bbb", "ccc", "ddd", "bbb"])]
>>> loads(dumps(v, string_referencing=True))
['aaa', ['bbb', 'ccc', 'ddd', 'ccc']] # 'bbb' came back as 'ccc'
The stringref spec (http://cbor.schmorp.de/stringref) requires it:
Within a value tagged with stringref-namespace, every string that is
encoded with a definite length and has a minimum length is assigned an
implicit index, starting from zero.
Since stringref-namespace tags can be nested, the decoder needs to save
and restore the outer array before starting and after ending the decoding
of the tagged value.
Set the outer maps aside on entry and restore them on exit. With this, the
encoder reproduces the nested worked example published in the spec byte for
byte; the test asserts against those published bytes.
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.
Changes
encode_semantic()(rust/encoder.rs:915) turnedstring_referencingon for tag256 but left the outer
string_references/bytes_referencesmaps in place —the two
TODOs in that function marked exactly this. References emitted inside anested namespace were therefore numbered against the outer namespace, and
strings registered inside it leaked back out. The decoder
(
rust/decoder.rs:1735) already handles it correctly: it pushes a freshnamespace and resolves tag 25 against the innermost one.
So the encoder emits output its own decoder cannot read — and sometimes reads
back as different data, with no exception:
The stringref spec requires the reset:
The fix sets the outer maps aside on entry and restores them on exit.
Oracle: the spec's own worked example
That page publishes a nested example with its bytes and its decoding. On
master,loads()reproduces the documented value exactly, butdumps()of that samevalue produces different bytes, and
loads()then fails on them:masterstring reference 2 not foundThe new test asserts against those published bytes rather than against anything
this implementation produced.
Test runs (same command and environment each time)
python -m pytest tests -qmaster464 passedmaster+ the new tests2 failed, 465 passed467 passedI also built the half-fix — clear the maps on entry, don't restore them — and it
still fails both new tests, which is why the restore case is tested separately.
test_decode_..._matches_spec_examplepasses either way; it is there as acontrol showing the decoder was never the faulty side.
Not a security issue as far as I can tell: it is encoder-side,
string_referencingis opt-in, and it needs a
CBORTag(256, …)in the caller's own data. I'll add thechangelog PR link once this has a number.
Checklist
tests/) which would fail without your patchdocs/), in case of behavior changes or new featuresdocs/versionhistory.rst)AI disclosure: found and drafted with Claude Code (model Claude Opus 5,
claude-opus-5) — differential fuzzing of encode→decode round trips, then thespec's published vector as the oracle. Reviewed before submission.