classification
Title: SystemError in cPickle for incorrect input
Type: behavior Stage: resolved
Components: Extension Modules Versions: Python 3.4, Python 3.3, Python 2.7
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: alexandre.vassalotti, haypo, pitrou, python-dev, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2013-04-13 11:00 by serhiy.storchaka, last changed 2013-04-15 20:23 by pitrou. This issue is now closed.

Files
File name Uploaded Description Edit
cpickle_systemerror.patch pitrou, 2013-04-13 18:26
pickle_systemerror.patch pitrou, 2013-04-13 18:40
fix_quoted_string_python3.patch alexandre.vassalotti, 2013-04-13 20:42
Messages (13)
msg186704 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-04-13 11:00
>>> 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.".
msg186782 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-04-13 18:26
Here is a patch for 2.7.
msg186786 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-04-13 18:32
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.
msg186788 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-04-13 18:34
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
msg186791 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-04-13 18:40
And here is a 3.x patch.
msg186821 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-04-13 20:17
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] == '"') {
msg186822 - (view) Author: Alexandre Vassalotti (alexandre.vassalotti) * (Python committer) Date: 2013-04-13 20:17
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.
msg186823 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-04-13 20:20
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
msg186836 - (view) Author: Alexandre Vassalotti (alexandre.vassalotti) * (Python committer) Date: 2013-04-13 20:42
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.
msg187017 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-04-15 19:21
> 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.
msg187022 - (view) Author: Roundup Robot (python-dev) Date: 2013-04-15 19:35
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
msg187026 - (view) Author: Roundup Robot (python-dev) Date: 2013-04-15 20:20
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
msg187027 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2013-04-15 20:23
I've committed the patches. Feel free to improve the default branch if you like.
History
Date User Action Args
2013-04-15 20:23:43pitrousetstatus: open -> closed
resolution: fixed
messages: + msg187027

stage: patch review -> resolved
2013-04-15 20:20:05python-devsetmessages: + msg187026
2013-04-15 19:35:34python-devsetnosy: + python-dev
messages: + msg187022
2013-04-15 19:21:57pitrousetmessages: + msg187017
2013-04-13 20:42:56alexandre.vassalottisetfiles: - fix_quoted_string_python3.patch
2013-04-13 20:42:10alexandre.vassalottisetfiles: + fix_quoted_string_python3.patch

messages: + msg186836
2013-04-13 20:20:36pitrousetmessages: + msg186823
2013-04-13 20:17:54alexandre.vassalottisetfiles: + fix_quoted_string_python3.patch

messages: + msg186822
2013-04-13 20:17:12serhiy.storchakasetmessages: + msg186821
2013-04-13 18:40:18pitrousetfiles: + pickle_systemerror.patch

messages: + msg186791
2013-04-13 18:35:59pitrousetversions: + Python 3.3, Python 3.4
2013-04-13 18:34:11pitrousetmessages: + msg186788
2013-04-13 18:32:15pitrousetstage: needs patch -> patch review
messages: + msg186786
versions: - Python 3.3, Python 3.4
2013-04-13 18:26:43pitrousetfiles: + cpickle_systemerror.patch
keywords: + patch
messages: + msg186782
2013-04-13 11:38:04hayposetnosy: + haypo
2013-04-13 11:00:22serhiy.storchakacreate