SystemError in cPickle for incorrect input

[serhiy-storchaka]

BPO	17710
Nosy	@pitrou, @vstinner, @avassalotti, @serhiy-storchaka
Files	cpickle_systemerror.patch pickle_systemerror.patch fix_quoted_string_python3.patch

^{Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.}

Show more details

GitHub fields:

assignee = None
closed_at = <Date 2013-04-15.20:23:43.211>
created_at = <Date 2013-04-13.11:00:21.985>
labels = ['extension-modules', 'type-bug']
title = 'SystemError in cPickle for incorrect input'
updated_at = <Date 2013-04-15.20:23:43.210>
user = 'https://github.com/serhiy-storchaka'

bugs.python.org fields:

activity = <Date 2013-04-15.20:23:43.210>
actor = 'pitrou'
assignee = 'none'
closed = True
closed_date = <Date 2013-04-15.20:23:43.211>
closer = 'pitrou'
components = ['Extension Modules']
creation = <Date 2013-04-13.11:00:21.985>
creator = 'serhiy.storchaka'
dependencies = []
files = ['29813', '29816', '29826']
hgrepos = []
issue_num = 17710
keywords = ['patch']
message_count = 13.0
messages = ['186704', '186782', '186786', '186788', '186791', '186821', '186822', '186823', '186836', '187017', '187022', '187026', '187027']
nosy_count = 5.0
nosy_names = ['pitrou', 'vstinner', 'alexandre.vassalotti', 'python-dev', 'serhiy.storchaka']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue17710'
versions = ['Python 2.7', 'Python 3.3', 'Python 3.4']

[serhiy-storchaka]

>>> import cPickle
>>> cPickle.loads(b"S' \np0\n.")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
SystemError: Negative size passed to PyString_FromStringAndSize
>>> pickle.loads(b"S' \np0\n.")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/serhiy/py/cpython2.7/Lib/pickle.py", line 1382, in loads
    return Unpickler(file).load()
  File "/home/serhiy/py/cpython2.7/Lib/pickle.py", line 858, in load
    dispatch[key](self)
  File "/home/serhiy/py/cpython2.7/Lib/pickle.py", line 966, in load_string
    raise ValueError, "insecure string pickle"
ValueError: insecure string pickle
>>> cPickle.loads(b"S'\np0\n.")
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
SystemError: Negative size passed to PyString_FromStringAndSize
>>> pickle.loads(b"S'\np0\n.")
''

Python 3 has the same behavior except C implementation raises UnpicklingError for b"S'\np0\n.".

[pitrou]

Here is a patch for 2.7.

[pitrou]

I don't think the UnpicklingError in 3.x is a bug, though: it's just a different exception than ValueError. It's just a shame that the two are used interchangeably for the same purpose.

[pitrou]

Hmm, forget what I just said. A SystemError can actually be triggered through a slightly longer line:

$ ./python -c "import pickle; print (repr(pickle.loads(b\"S'    \np0\n.\")))"
Traceback (most recent call last):
  File "<string>", line 1, in <module>
SystemError: Negative size passed to PyBytes_FromStringAndSize

[pitrou]

And here is a 3.x patch.

[serhiy-storchaka]

Of course, UnpicklingError is not a bug. Perhaps almost any exception except SystemError is not a bug. I mention it because it's a case where Python 3 differs from Python 2.

I think _pickle.c patches can be simplified.

+    if (len < 2)
+        goto insecure;
     if (s[0] == '"' && s[len - 1] == '"') {

[avassalotti]

I also wrote a patch for this. I took I slightly different approach though. I fixed the C implementation to be more strict on the quoting. Currently, it strips trailing non-printable characters, something pickle.py doesn't do.

I also cleaned up the tests to check this behavior.

[pitrou]

Alexandre, I don't like your patch very much:

• you are changing the exception from ValueError to UnpicklingError (which doesn't derive from ValueError) in a bugfix release
• you aren't actually adding any guards in the C code, so it's not demonstrably more robust

[avassalotti]

I was targeting head, not the release branches. It is fine to change the exception there as we don't make any guarantee about the exceptions raised during the unpickling process. It is easy enough to fix patch use ValueError for the release branch.

And adding guards is unnecessary after the removing the code for stripping trailing whitespace.

Here's slightly updated patch that check if readline() reached an EOF instead of a newline.

[pitrou]

I was targeting head, not the release branches.

Perhaps, but I don't see the point of choosing a different fix in the default branch than in the bugfix branches.

[python-dev]

New changeset 527b7f88b53c by Antoine Pitrou in branch '2.7':
Issue bpo-17710: Fix cPickle raising a SystemError on bogus input.
http://hg.python.org/cpython/rev/527b7f88b53c

[python-dev]

New changeset 4e412cbaaf96 by Antoine Pitrou in branch '3.3':
Issue bpo-17710: Fix pickle raising a SystemError on bogus input.
http://hg.python.org/cpython/rev/4e412cbaaf96

New changeset 5a16d2992112 by Antoine Pitrou in branch 'default':
Issue bpo-17710: Fix pickle raising a SystemError on bogus input.
http://hg.python.org/cpython/rev/5a16d2992112

[pitrou]

I've committed the patches. Feel free to improve the default branch if you like.