Skip to content

fix: POP_MARK and POP at a mark decode like CPython (#49) - #58

Merged
jensens merged 2 commits into
mainfrom
fix/pop-mark
Sep 30, 2026
Merged

jensens merged 2 commits into
mainfrom
fix/pop-mark

Conversation

@jensens

@jensens jensens commented Sep 30, 2026

Copy link
Copy Markdown
Member

Fixes #49

Two mark opcodes were not handled the way CPython handles them: POP_MARK raised unknown pickle opcode, and POP with nothing above the last MARK raised stack underflow where CPython discards the mark.

The issue assumed these only occur at protocols 0 and 1. The reviewer's differential run showed otherwise: CPython writes MARK ... POP_MARK GET for any recursive tuple of four or more elements at every binary protocol, protocol 3 included, so a persisted ZODB record holding such a tuple (a tuple whose element refers back to it) failed to decode on main. Protocol 1 writes it for any recursive tuple; protocol 0 writes POPs instead.

What changes

  • POP_MARK: everything above the last MARK and the mark itself are discarded; the memo bindings of the discarded slots are flushed with their live values first (a list bound to a memo index and read after the POP_MARK comes back with the element that was appended to it).
  • POP at the mark discards the mark, as CPython's load_pop does; POP on an empty stack without a mark still raises.
  • The memo pre-scan knows the new no-argument opcode.

A recursive tuple decodes to the value the memo holds at the closing GET, the same rule the codec already applies to recursive lists and dicts and to short recursive tuples at protocol 2, so the output is identical across protocols (([], 11, 12, 13) for the four-element case).

Evidence

  • 240 Rust and 393 Python tests; the Python tests pin the four-element tuple at protocols 1 to 3, the memo flush and a protocol 3 record.
  • The 1,692-record sample is byte-identical to main (it contains no POP_MARK).
  • A fresh reviewer ran 18 recursive structures at protocols 0 to 5 through every path (108 cases: 64 raise on main, all decode on the branch, deterministic, the 44 that main decodes byte-identical), 19 hand-built streams against both CPython unpicklers, and proved the pre-scan stays active and safe. Its findings (the protocol scope and the test gap) are the last commit.

Assisted-by: Claude Fable 5.1

`POP_MARK` discards everything above the last MARK and the mark itself,
flushing the memo bindings of the discarded slots first; `POP` with nothing
above the last MARK discards the mark, as CPython's load_pop does. Protocol
0 and 1 picklers write these to unwind a recursive tuple; the pre-scan
learns the new no-argument opcode. The recursive tuple decodes to the
snapshot the memo holds at the closing GET, the tests pin it.

Assisted-by: Claude Fable 5.1
…l 3 (#49)

Fresh review finding: CPython writes POP_MARK for a recursive tuple of four
or more elements at every binary protocol, protocol 3 included, so a
persisted record with such a tuple failed to decode on main; the changelog
and the test docstring said protocol 0 or 1 only. Tests now pin the
four-element tuple at protocols 1 to 3, the memo flush of the discarded
slots (a list bound to a memo index and read after the POP_MARK holds its
live value) and a protocol 3 record.

Assisted-by: Claude Fable 5.1
@jensens
jensens merged commit 02beaf7 into main Sep 30, 2026
5 checks passed
@jensens
jensens deleted the fix/pop-mark branch September 30, 2026 17:10
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.

Decoder: POP_MARK unsupported and POP does not pop a MARK (protocol 0/1 recursive tuples)

1 participant