Pickle INT/LONG base discrepancy

Bug report

Bug description:

The INT opcode in pickle is the I character followed by an ASCII number and a newline. There are multiple comments asking if the base should be explicitly set to 10, or kept as 0. However, a discrepancy exists between pickle implementations:

_pickle.c uses strtol(s, &endptr, 0); with a base of 0, meaning 0xf would succeed
pickle.py uses int(data, 0) with a base of 0, meaning 0xf would succeed
pickletools.py uses read_decimalnl_short(), which calls int(s), meaning any non-decimal base would fail

This same inconsistency exists with the LONG opcode:

This means an attempt to disassemble a pickle bytestream using pickletools would fail here, while the actual unpickling process would proceed undisputed.

Personally, I don't really care whether all implementations are changed to base 10 or base 0 (save_long() only puts it in decimal form), but I think it should be consistent across all implementations. I'd submit a pull request for one way or the other, but I'm not sure which way you'd prefer it.

Also as a note, the pickle bytestream b'I0001\n.' (INT with the argument 0001) fails in pickle.py because having leading 0s in a number with base 0 causes an error. Note that no errors are thrown in _pickle.c because it uses strtol or pickletools.py because it doesn't have base 0 specified. If we keep the implementation as base 0, that discrepancy between pickle.py and other pickle implementations would stay, whereas if we change it to base 10 (aka remove base 0), that inconsistency would also go away. For LONG, both pickle.py and _pickle.c fail with b'L0001L\n.', but pickletools.py has no problem displaying that number (since it has no base specified).

CPython versions tested on:

3.11

Operating systems tested on:

Linux

Linked PRs

gh-126992: Change pickle code to base 10 for load_long and load_int #127042

I think that using base 10 would be reasonable here.

I'll make a pull request then

I think that using base 10 would be reasonable here.

I think it's an incompatible change. Why not adjust pickletools instead?

BTW, I see no failing tests in #127042. Probably we should add some regression tests.

No pickles produced by pickle.dumps() would use any base other than 10, so only custom-made pickles would be broken by this change (which is also why it wouldn't break any tests).

Yes, this compatibility break doesn't look too severe. After all, I'm not sure that many projects use the version 0 protocol.

On another hand, it's really easy to not introduce backward-incompatible changes.

I think it's an incompatible change.

Yes, and IMO it's a good change.

In practice, no one should be impacted since everybody should use the Python module to serialize to pickle, and in this case, you cannot get a base different than 10 in the serialized files.

I thought about using hexadecimals for LONG. This would fix the issue of quadratic complexity for converting long to/from string. But it was not supported in all implementations, so it would be a breaking change, while the purpose of the LONG opcode is compatibility with old Python versions and non-Python implementations.

The CPython implementation always accepted non-decimal input for INT and LONG (except short period from 12 April to 15 May in 1996). It can be considered a hidden CPython feature.

I do not think it is an issue if the CPython implementation is more lenient than the specification (which was written after the implementation and still not complete).

@tim-one

@tim-one, this may be interesting to you, as it relates to both pickletools and the problems with decimal representation of long integers.

Using hexadecimal representation for long integers would be more efficient in terms of time and size. But it is less efficient than using pickle protocol 1 or higher. And not all third-party implementations (for different programming languages) support hexadecimals.

@tim-one

It is more complicated for other prefixes. strtol() parses numbers starting with 0 as octals, but it is an error for int().

>>> import pickle, pickletools, io
>>> pickletools.dis(b'I010\n.')
    0: I    INT        10
    5: .    STOP
highest protocol among opcodes = 0
>>> pickle.Unpickler(io.BytesIO(b'I010\n.')).load()
8
>>> pickle._Unpickler(io.BytesIO(b'I010\n.')).load()
Traceback (most recent call last):
  File "<python-input-19>", line 1, in <module>
    pickle._Unpickler(io.BytesIO(b'I010\n.')).load()
    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^
  File "/home/serhiy/py/cpython/Lib/pickle.py", line 1327, in load
    dispatch[key[0]](self)
    ~~~~~~~~~~~~~~~~^^^^^^
  File "/home/serhiy/py/cpython/Lib/pickle.py", line 1390, in load_int
    val = int(data, 0)
ValueError: invalid literal for int() with base 0: b'010\n'

@tim-one, will we support non-decimal bases for pickle protocol 0 in future? If no, we should close this door in pickle. If yes, we should fix support of octals in pickle and make pickletools consisting with pickle.

@tim-one

@tim-one, will we support non-decimal bases for pickle protocol 0 in future?

I don't think that protocol 0 should change. It uses base 10 and this property has to stay. Otherwise, we introduce a backward incompatibility, no? Like loading new serialized data with old Python may no longer work.

Like loading new serialized data with old Python may no longer work.

It works with all old CPython versions (except if there were releases in short period from 12 April to 15 May in 1996). It may not work with implementations of pickle format in other programming languages.

@tim-one

@tim-one, will we support non-decimal bases for pickle protocol 0 in future?

The only reason we have still have "protocol 0" at all is "backward compatibility". That's the original protocol, and, at the start, the only protocol. The only sane use for it now would be if someone needs to write a pickle readable by an ancient version of Python. The ancient version can no longer be changed in any way, so there is no sane reason to add "new features" to protocol 0 now. So, "no" is my answer to the question.

A semi-sane reason to use protocol 0 was to get a human-readable text file for learning something about how pickles work, but that purpose is far better served now by pickletools.dis(), which gives a human-readable display of pickles using all protocols.

gh-126992: Change pickle code to base 10 for load_long and load_int (G…

@Legoclones

Fixed by change ce76b54. Thanks @Legoclones.

pythongh-126992: Change pickle code to base 10 for load_long and load…

Navigation Menu

Repository navigation

Pickle `INT`/`LONG` base discrepancy #126992

Bug report

Bug description:

CPython versions tested on:

Operating systems tested on:

Linked PRs

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Pickle INT/LONG base discrepancy #126992

Description

Bug report

Bug description:

CPython versions tested on:

Operating systems tested on:

Linked PRs

Activity

vstinner commented on Nov 19, 2024

Legoclones commented on Nov 19, 2024

skirpichev commented on Nov 20, 2024

Legoclones commented on Nov 20, 2024

skirpichev commented on Nov 20, 2024

vstinner commented on Nov 20, 2024

serhiy-storchaka commented on Nov 20, 2024

serhiy-storchaka commented on Nov 22, 2024

serhiy-storchaka commented on Nov 25, 2024

serhiy-storchaka commented on Dec 5, 2024

vstinner commented on Dec 5, 2024

serhiy-storchaka commented on Dec 5, 2024

tim-one commented on Dec 5, 2024

vstinner commented on Dec 11, 2024

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions

Pickle `INT`/`LONG` base discrepancy #126992