Issue #985064: Make plistlib more resilient to faulty input plists.

Patch by Mher Movsisyan.

Author: ned-deily

Lib/plistlib.py

@@ -68,13 +68,15 @@ def readPlist(pathOrFile):
     usually is a dictionary).
     """
     didOpen = False
-    if isinstance(pathOrFile, str):
-        pathOrFile = open(pathOrFile, 'rb')
-        didOpen = True
-    p = PlistParser()
-    rootObject = p.parse(pathOrFile)
-    if didOpen:
-        pathOrFile.close()
+    try:
+        if isinstance(pathOrFile, str):
+            pathOrFile = open(pathOrFile, 'rb')
+            didOpen = True
+        p = PlistParser()
+        rootObject = p.parse(pathOrFile)
+    finally:
+        if didOpen:
+            pathOrFile.close()
     return rootObject
 
 
@@ -83,15 +85,17 @@ def writePlist(rootObject, pathOrFile):
     file name or a (writable) file object.
     """
     didOpen = False
-    if isinstance(pathOrFile, str):
-        pathOrFile = open(pathOrFile, 'wb')
-        didOpen = True
-    writer = PlistWriter(pathOrFile)
-    writer.writeln("<plist version=\"1.0\">")
-    writer.writeValue(rootObject)
-    writer.writeln("</plist>")
-    if didOpen:
-        pathOrFile.close()
+    try:
+        if isinstance(pathOrFile, str):
+            pathOrFile = open(pathOrFile, 'wb')
+            didOpen = True
+        writer = PlistWriter(pathOrFile)
+        writer.writeln("<plist version=\"1.0\">")
+        writer.writeValue(rootObject)
+        writer.writeln("</plist>")
+    finally:
+        if didOpen:
+            pathOrFile.close()
 
 
 def readPlistFromBytes(data):
@@ -352,7 +356,6 @@ def __eq__(self, other):
     def __repr__(self):
         return "%s(%s)" % (self.__class__.__name__, repr(self.data))
 
-
 class PlistParser:
 
     def __init__(self):
@@ -362,11 +365,11 @@ def __init__(self):
 
     def parse(self, fileobj):
         from xml.parsers.expat import ParserCreate
-        parser = ParserCreate()
-        parser.StartElementHandler = self.handleBeginElement
-        parser.EndElementHandler = self.handleEndElement
-        parser.CharacterDataHandler = self.handleData
-        parser.ParseFile(fileobj)
+        self.parser = ParserCreate()
+        self.parser.StartElementHandler = self.handleBeginElement
+        self.parser.EndElementHandler = self.handleEndElement
+        self.parser.CharacterDataHandler = self.handleData
+        self.parser.ParseFile(fileobj)
         return self.root
 
     def handleBeginElement(self, element, attrs):
@@ -385,12 +388,18 @@ def handleData(self, data):
 
     def addObject(self, value):
         if self.currentKey is not None:
+            if not isinstance(self.stack[-1], type({})):
+                raise ValueError("unexpected element at line %d" %
+                                 self.parser.CurrentLineNumber)
             self.stack[-1][self.currentKey] = value
             self.currentKey = None
         elif not self.stack:
             # this is the root object
             self.root = value
         else:
+            if not isinstance(self.stack[-1], type([])):
+                raise ValueError("unexpected element at line %d" %
+                                 self.parser.CurrentLineNumber)
             self.stack[-1].append(value)
 
     def getData(self):
@@ -405,9 +414,15 @@ def begin_dict(self, attrs):
         self.addObject(d)
         self.stack.append(d)
     def end_dict(self):
+        if self.currentKey:
+            raise ValueError("missing value for key '%s' at line %d" %
+                             (self.currentKey,self.parser.CurrentLineNumber))
         self.stack.pop()
 
     def end_key(self):
+        if self.currentKey or not isinstance(self.stack[-1], type({})):
+            raise ValueError("unexpected key at line %d" %
+                             self.parser.CurrentLineNumber)
         self.currentKey = self.getData()
 
     def begin_array(self, attrs):

Lib/test/test_plistlib.py

@@ -175,6 +175,32 @@ def test_nondictroot(self):
         self.assertEqual(test1, result1)
         self.assertEqual(test2, result2)
 
+    def test_invalidarray(self):
+        for i in ["<key>key inside an array</key>",
+                  "<key>key inside an array2</key><real>3</real>",
+                  "<true/><key>key inside an array3</key>"]:
+            self.assertRaises(ValueError, plistlib.readPlistFromBytes,
+                              ("<plist><array>%s</array></plist>"%i).encode())
+
+    def test_invaliddict(self):
+        for i in ["<key><true/>k</key><string>compound key</string>",
+                  "<key>single key</key>",
+                  "<string>missing key</string>",
+                  "<key>k1</key><string>v1</string><real>5.3</real>"
+                  "<key>k1</key><key>k2</key><string>double key</string>"]:
+            self.assertRaises(ValueError, plistlib.readPlistFromBytes,
+                              ("<plist><dict>%s</dict></plist>"%i).encode())
+            self.assertRaises(ValueError, plistlib.readPlistFromBytes,
+                              ("<plist><array><dict>%s</dict></array></plist>"%i).encode())
+
+    def test_invalidinteger(self):
+        self.assertRaises(ValueError, plistlib.readPlistFromBytes,
+                          b"<plist><integer>not integer</integer></plist>")
+
+    def test_invalidreal(self):
+        self.assertRaises(ValueError, plistlib.readPlistFromBytes,
+                          b"<plist><integer>not real</integer></plist>")
+
 
 def test_main():
     support.run_unittest(TestPlistlib)

Misc/ACKS

@@ -615,6 +615,7 @@ Paul Moore
 Derek Morr
 James A Morrison
 Pablo Mouzo
+Mher Movsisyan
 Sjoerd Mullender
 Sape Mullender
 Michael Muller

Misc/NEWS

@@ -172,6 +172,9 @@ Core and Builtins
 Library
 -------
 
+- Issue #985064: Make plistlib more resilient to faulty input plists.
+  Patch by Mher Movsisyan.
+
 - Issue #1625: BZ2File and bz2.decompress() now support multi-stream files.
   Initial patch by Nir Aides.