Issue 17710: SystemError in cPickle for incorrect input

>>> 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.".

Here is a patch for 2.7.

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.

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

And here is a 3.x patch.

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] == '"') {

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.

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

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.

> 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.

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

New changeset 4e412cbaaf96 by Antoine Pitrou in branch '3.3':
Issue #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 #17710: Fix pickle raising a SystemError on bogus input.
http://hg.python.org/cpython/rev/5a16d2992112

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

stage: patch review -> resolved

messages: + msg186836

messages: + msg186822

messages: + msg186791