plistlib crashes too easily on bad files

[jackjansen]

BPO	985064
Nosy	@jackjansen, @birkenfeld, @ronaldoussoren, @ned-deily, @ezio-melotti, @merwok, @mher
Dependencies	bpo-775321: plistlib error handling
Files	plist_validation.diff: patch for validation plist_validation_v2.diff: 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 = 'https://github.com/ned-deily'
closed_at = <Date 2011-05-28.10:19:39.727>
created_at = <Date 2004-07-04.21:30:29.000>
labels = ['OS-mac', 'easy', 'type-bug']
title = 'plistlib crashes too easily on bad files'
updated_at = <Date 2011-05-28.10:19:39.725>
user = 'https://github.com/jackjansen'

bugs.python.org fields:

activity = <Date 2011-05-28.10:19:39.725>
actor = 'ned.deily'
assignee = 'ned.deily'
closed = True
closed_date = <Date 2011-05-28.10:19:39.727>
closer = 'ned.deily'
components = ['macOS']
creation = <Date 2004-07-04.21:30:29.000>
creator = 'jackjansen'
dependencies = ['775321']
files = ['19996', '20258']
hgrepos = []
issue_num = 985064
keywords = ['patch', 'easy']
message_count = 8.0
messages = ['60527', '123725', '123758', '125333', '125341', '126089', '137115', '137116']
nosy_count = 9.0
nosy_names = ['jackjansen', 'georg.brandl', 'jvr', 'ronaldoussoren', 'ned.deily', 'ezio.melotti', 'eric.araujo', 'mher', 'python-dev']
pr_nums = []
priority = 'normal'
resolution = 'fixed'
stage = 'resolved'
status = 'closed'
superseder = None
type = 'behavior'
url = 'https://bugs.python.org/issue985064'
versions = ['Python 3.1', 'Python 3.2', 'Python 3.3']

[jackjansen]

Plistlib doesn't do much error checking, and it can crash on bad
input. Moreover, it doesn't provide much help if it does crash (no
linenumbers, etc).

The problem I ran into was a dangling <key>foo</key>. After this
key the dict ended, but the next entry in the surrounding
datastructure, an array, picked up the key from self.currentKey
and crashed in addObject().

I was about to fix this when I noticed that there's lots of problems
with <key> handling, duplicates or missing ones aren't detected
either and can cause crashes too. It may be better to put a general
try/except in parse() and print a line number or something in case
of a failure.

[mher]

The attached patch fixes crashes on bad input. The patch implements validation for dict and array elements as well as some resource cleanup. The tests are included as well.

[ned-deily]

One review comment: the patch adds a new exception class that is used for the errors that are now additionally detected. Elsewhere plistlib uses non-specific exception classes like ValueError. If starting from scratch, it might be better to consistently use a specific exception class but that would create incompatibilities if changed now. I don't see a compelling need to add one now just for these errors. (But, if kept, it should be added to the docs.) Otherwise, looks good to me.

Thanks for taking this on!

[mher]

I've replaced plistlib.InvalidPlistError with ValueError

[birkenfeld]

LGTM.

[merwok]

See also reopened dependency bpo-775321.

[python-dev]

New changeset a2688e252204 by Ned Deily in branch '3.1':
Issue bpo-985064: Make plistlib more resilient to faulty input plists.
http://hg.python.org/cpython/rev/a2688e252204

New changeset f555d959a5d7 by Ned Deily in branch '3.2':
Issue bpo-985064: Make plistlib more resilient to faulty input plists.
http://hg.python.org/cpython/rev/f555d959a5d7

New changeset d0bc18a50bd1 by Ned Deily in branch 'default':
Issue bpo-985064: Make plistlib more resilient to faulty input plists.
http://hg.python.org/cpython/rev/d0bc18a50bd1

[ned-deily]

Thank you for the patch and tests! Applied in 3.1 (for 3.1.4), 3.2 (for 3.2.1), and 3.3. (The 2.x version of plistlib differs somewhat from the 3.x version so the patch would need some rework and testing for 2.7; that is probably not worth the effort at this point.)