Fix Bug #114293:

    Strings are unpickled by calling eval on the string's repr. This
    change makes pickle work like cPickle; it checks if the pickled
    string is safe to eval and raises ValueError if it is not.

test suite modifications:
    Verify that pickle catches a variety of insecure string pickles
    Make test_pickle and test_cpickle use exactly the same test suite
    Add test for pickling recursive object

5 files changed, 131 insertions, 112 deletions

diff --git a/Lib/pickle.py b/Lib/pickle.py
index 6c1bcbe..128a627 100644
--- a/Lib/pickle.py
+++ b/Lib/pickle.py
@@ -577,10 +577,50 @@ class Unpickler:
     dispatch[BINFLOAT] = load_binfloat
 
     def load_string(self):
-        self.append(eval(self.readline()[:-1],
+        rep = self.readline()[:-1]
+        if not self._is_string_secure(rep):
+            raise ValueError, "insecure string pickle"
+        self.append(eval(rep,
                          {'__builtins__': {}})) # Let's be careful
     dispatch[STRING] = load_string
 
+    def _is_string_secure(self, s):
+        """Return true if s contains a string that is safe to eval
+
+        The definition of secure string is based on the implementation
+        in cPickle.  s is secure as long as it only contains a quoted
+        string and optional trailing whitespace.
+        """
+        q = s[0]
+        if q not in ("'", '"'):
+            return 0
+        # find the closing quote
+        offset = 1
+        i = None
+        while 1:
+            try:
+                i = s.index(q, offset)
+            except ValueError:
+                # if there is an error the first time, there is no
+                # close quote
+                if offset == 1:
+                    return 0
+            if s[i-1] != '\\':
+                break
+            # check to see if this one is escaped
+            nslash = 0
+            j = i - 1
+            while j >= offset and s[j] == '\\':
+                j = j - 1
+                nslash = nslash + 1
+            if nslash % 2 == 0:
+                break
+            offset = i + 1
+        for c in s[i+1:]:
+            if ord(c) > 32:
+                return 0
+        return 1
+
     def load_binstring(self):
         len = mloads('i' + self.read(4))
         self.append(self.read(len))
diff --git a/Lib/test/output/test_cpickle b/Lib/test/output/test_cpickle
index 4f78690..696288a 100644
--- a/Lib/test/output/test_cpickle
+++ b/Lib/test/output/test_cpickle
@@ -9,3 +9,17 @@ loads() binary
 ok
 loads() BINDATA
 ok
+dumps() RECURSIVE
+ok
+dumps()
+loads()
+ok
+loads() DATA
+ok
+dumps() binary
+loads() binary
+ok
+loads() BINDATA
+ok
+dumps() RECURSIVE
+ok
diff --git a/Lib/test/output/test_pickle b/Lib/test/output/test_pickle
index 78a80a9..4b054b7 100644
--- a/Lib/test/output/test_pickle
+++ b/Lib/test/output/test_pickle
@@ -9,3 +9,5 @@ loads() binary
 ok
 loads() BINDATA
 ok
+dumps() RECURSIVE
+ok
diff --git a/Lib/test/test_cpickle.py b/Lib/test/test_cpickle.py
index f5e920f..f2aa0fe 100644
--- a/Lib/test/test_cpickle.py
+++ b/Lib/test/test_cpickle.py
@@ -1,107 +1,5 @@
 # Test the cPickle module
 
-DATA = """(lp0
-I0
-aL1L
-aF2.0
-ac__builtin__
-complex
-p1
-(F3.0
-F0.0
-tp2
-Rp3
-a(S'abc'
-p4
-g4
-(i__main__
-C
-p5
-(dp6
-S'foo'
-p7
-I1
-sS'bar'
-p8
-I2
-sbg5
-tp9
-ag9
-aI5
-a.
-"""
-
-BINDATA = ']q\000(K\000L1L\012G@\000\000\000\000\000\000\000c__builtin__\012complex\012q\001(G@\010\000\000\000\000\000\000G\000\000\000\000\000\000\000\000tq\002Rq\003(U\003abcq\004h\004(c__main__\012C\012q\005oq\006}q\007(U\003fooq\010K\001U\003barq\011K\002ubh\006tq\012h\012K\005e.'
-
 import cPickle
-
-class C:
-    def __cmp__(self, other):
-        return cmp(self.__dict__, other.__dict__)
-
-import __main__
-__main__.C = C
-
-def dotest():
-    c = C()
-    c.foo = 1
-    c.bar = 2
-    x = [0, 1L, 2.0, 3.0+0j]
-    y = ('abc', 'abc', c, c)
-    x.append(y)
-    x.append(y)
-    x.append(5)
-    print "dumps()"
-    s = cPickle.dumps(x)
-    print "loads()"
-    x2 = cPickle.loads(s)
-    if x2 == x: print "ok"
-    else: print "bad"
-    print "loads() DATA"
-    x2 = cPickle.loads(DATA)
-    if x2 == x: print "ok"
-    else: print "bad"
-    print "dumps() binary"
-    s = cPickle.dumps(x, 1)
-    print "loads() binary"
-    x2 = cPickle.loads(s)
-    if x2 == x: print "ok"
-    else: print "bad"
-    print "loads() BINDATA"
-    x2 = cPickle.loads(BINDATA)
-    if x2 == x: print "ok"
-    else: print "bad"
-
-    # Test protection against closed files
-    import tempfile, os
-    fn = tempfile.mktemp()
-    f = open(fn, "w")
-    f.close()
-    try:
-        cPickle.dump(123, f)
-    except ValueError:
-        pass
-    else:
-        print "dump to closed file should raise ValueError"
-    f = open(fn, "r")
-    f.close()
-    try:
-        cPickle.load(f)
-    except ValueError:
-        pass
-    else:
-        print "load from closed file should raise ValueError"
-    os.remove(fn)
-
-    # Test specific bad cases
-    for i in range(10):
-        try:
-            x = cPickle.loads('garyp')
-        except cPickle.BadPickleGet, y:
-            del y
-        else:
-            print "unexpected success!"
-            break
-    
-
-dotest()
+import test_pickle
+test_pickle.dotest(cPickle)
diff --git a/Lib/test/test_pickle.py b/Lib/test/test_pickle.py
index 8fb534d..ff9c467 100644
--- a/Lib/test/test_pickle.py
+++ b/Lib/test/test_pickle.py
@@ -1,5 +1,6 @@
 # Test the pickle module
 
+# break into multiple strings to please font-lock-mode
 DATA = """(lp0
 I0
 aL1L
@@ -7,17 +8,20 @@ aF2.0
 ac__builtin__
 complex
 p1
-(F3.0
+""" \
+"""(F3.0
 F0.0
 tp2
 Rp3
 a(S'abc'
 p4
 g4
-(i__main__
+""" \
+"""(i__main__
 C
 p5
-(dp6
+""" \
+"""(dp6
 S'foo'
 p7
 I1
@@ -33,8 +37,6 @@ a.
 
 BINDATA = ']q\000(K\000L1L\012G@\000\000\000\000\000\000\000c__builtin__\012complex\012q\001(G@\010\000\000\000\000\000\000G\000\000\000\000\000\000\000\000tq\002Rq\003(U\003abcq\004h\004(c__main__\012C\012q\005oq\006}q\007(U\003fooq\010K\001U\003barq\011K\002ubh\006tq\012h\012K\005e.'
 
-import pickle
-
 class C:
     def __cmp__(self, other):
         return cmp(self.__dict__, other.__dict__)
@@ -42,7 +44,7 @@ class C:
 import __main__
 __main__.C = C
 
-def dotest():
+def dotest(pickle):
     c = C()
     c.foo = 1
     c.bar = 2
@@ -51,6 +53,8 @@ def dotest():
     x.append(y)
     x.append(y)
     x.append(5)
+    r = []
+    r.append(r)
     print "dumps()"
     s = pickle.dumps(x)
     print "loads()"
@@ -71,5 +75,66 @@ def dotest():
     x2 = pickle.loads(BINDATA)
     if x2 == x: print "ok"
     else: print "bad"
+    s = pickle.dumps(r)
+    print "dumps() RECURSIVE"
+    x2 = pickle.loads(s)
+    if x2 == r: print "ok"
+    else: print "bad"
 
-dotest()
+    # Test protection against closed files
+    import tempfile, os
+    fn = tempfile.mktemp()
+    f = open(fn, "w")
+    f.close()
+    try:
+        pickle.dump(123, f)
+    except ValueError:
+        pass
+    else:
+        print "dump to closed file should raise ValueError"
+    f = open(fn, "r")
+    f.close()
+    try:
+        pickle.load(f)
+    except ValueError:
+        pass
+    else:
+        print "load from closed file should raise ValueError"
+    os.remove(fn)
+
+    # Test specific bad cases
+    for i in range(10):
+        try:
+            x = pickle.loads('garyp')
+        except KeyError, y:
+            # pickle
+            del y
+        except pickle.BadPickleGet, y:
+            # cPickle
+            del y
+        else:
+            print "unexpected success!"
+            break
+
+    # Test insecure strings
+    insecure = ["abc", "2 + 2", # not quoted
+                "'abc' + 'def'", # not a single quoted string
+                "'abc", # quote is not closed
+                "'abc\"", # open quote and close quote don't match
+                "'abc'   ?", # junk after close quote
+                # some tests of the quoting rules
+                "'abc\"\''",
+                "'\\\\a\'\'\'\\\'\\\\\''",
+                ]
+    for s in insecure:
+        buf = "S" + s + "\012p0\012."
+        try:
+            x = pickle.loads(buf)
+        except ValueError:
+            pass
+        else:
+            print "accepted insecure string: %s" % repr(buf)
+        
+
+import pickle
+dotest(pickle)