pickletools.dis() doesn't allow redefinition of memo items while pickle itself doesn't mind

[birkenfeld]

Bug report

Bug description:

pickletools' disassembler has a check here that doesn't allow memo indices to be redefined, while the pickle.Unpickler doesn't care about it, an existing item is handled without complaint here. FWIW, the opcode doc doesn't mention a restriction either.

Such pickles aren't produced by the Pickler, but it is strange for a debug tool to be more restrictive than the thing it is supposed to help debug. (I found this while fuzzing a non-Python unpickling library.) I think the check in pickletools L2497 can just be removed. A warning could also be emitted, but that would be the first such warning in pickletools.

CPython versions tested on:

3.12

Operating systems tested on:

Linux

Linked PRs

• gh-123309: Add more tests for the pickletools module #123355
• gh-123309: Remove check for redefined memo entry in pickletools.dis() #123374
• [3.13] gh-123309: Add more tests for the pickletools module (GH-123355) #123533
• [3.12] gh-123309: Add more tests for the pickletools module (GH-123355) #123534

[picnixz]

cc @serhiy-storchaka as someone who works on pickle a lot recently

[serhiy-storchaka]

I agree, this check can be removed. It is perhaps not worth to emit a warning, as no warnings are emitted for other weird code.

The error message is not even always correct. For example, if there is "PUT 1" and then "MEMOIZE", the error message will be "memo key None already defined".

>>> import pickletools
>>> pickletools.dis(b'Np1\nN\x94')
    0: N    NONE
    1: p    PUT        1
    4: N    NONE
    5: \x94 MEMOIZE    (as 1)
Traceback (most recent call last):
  File "<python-input-1>", line 1, in <module>
    pickletools.dis(b'Np1\nN\x94')
    ~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^
  File "/home/serhiy/py/cpython/Lib/pickletools.py", line 2531, in dis
    raise ValueError(errormsg)
ValueError: memo key None already defined

[serhiy-storchaka]

Surprisingly, there are not any tests for pickletools.dis(), except doctests, which do not cover all corner cases. So I am going first to add tests.

This check itself is not a bug, it is even explicitly mentioned as a feature in the module comments. We can remove it because current unpickling implementations support redefinition of memo items, and this does not left unpickler in inconsistent state. All other checks are necessary for correct work of dis().