classification
Title: Segfault in Unpickler_set_memo()
Type: crash Stage: resolved
Components: Versions: Python 3.4, Python 3.3
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: christian.heimes, python-dev, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2013-07-01 12:12 by christian.heimes, last changed 2013-07-01 21:16 by christian.heimes. This issue is now closed.

Files
File name Uploaded Description Edit
memo.patch christian.heimes, 2013-07-01 12:12 review
Messages (5)
msg192124 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2013-07-01 12:12
Unpickler_set_memo() crashes when the unpickler's memo attribute is set to a dict with negative numbers. The descriptor uses _Unpickler_MemoPut() which uses the dict key as index to a C array.

Python 3.3.0 (v3.3.0:bd8afb90ebf2, Feb  8 2013, 00:38:29) 
[GCC 4.7.2] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> import sys, pickle
>>> p = pickle.Unpickler(sys.stdin)
>>> p.memo = {-1: None}
segfault

The issue was found be Coverity Scan:

CID 486776 (#1 of 1): Improper use of negative value (NEGATIVE_RETURNS)
negative_returns: Passing variable "idx" to a parameter that cannot be negative.
5955            if (_Unpickler_MemoPut(self, idx, value) < 0)
msg192127 - (view) Author: Roundup Robot (python-dev) Date: 2013-07-01 13:18
New changeset 17b7af660f82 by Christian Heimes in branch '3.3':
Issue #18339: Negative ints keys in unpickler.memo dict no longer cause a
http://hg.python.org/cpython/rev/17b7af660f82

New changeset f3372692ca20 by Christian Heimes in branch 'default':
Issue #18339: Negative ints keys in unpickler.memo dict no longer cause a
http://hg.python.org/cpython/rev/f3372692ca20
msg192130 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2013-07-01 14:20
Sorry that I was late with review. Here is some nitpicks from me.

Using assertRaises() as context manager in this case looks cleaner to me:

    with self.assertRaises(ValueError):
        unpickler.memo = {-1: None}

Moving the `if (idx == -1 && PyErr_Occurred())` check inside the `if (idx < 0)` block will increase the perfomance a little.
msg192149 - (view) Author: Roundup Robot (python-dev) Date: 2013-07-01 21:00
New changeset fa0a03afe359 by Christian Heimes in branch '3.3':
Issue #18339: use with self.assertRaises() to make test case more readable
http://hg.python.org/cpython/rev/fa0a03afe359

New changeset de44cd866bb0 by Christian Heimes in branch 'default':
Issue #18339: use with self.assertRaises() to make test case more readable
http://hg.python.org/cpython/rev/de44cd866bb0
msg192150 - (view) Author: Christian Heimes (christian.heimes) * (Python committer) Date: 2013-07-01 21:16
I don't think that a reordered idx < 0 check is going to make any measurable difference. I like the separate checks as they make the code easier to understand. The first check tests for error in PyLong_AsSsize_t() and the second one checks for positive integers.
History
Date User Action Args
2013-07-01 21:16:42christian.heimessetstatus: open -> closed

messages: + msg192150
stage: commit review -> resolved
2013-07-01 21:00:33python-devsetmessages: + msg192149
2013-07-01 14:20:18serhiy.storchakasetstatus: pending -> open
nosy: + serhiy.storchaka
messages: + msg192130

2013-07-01 13:19:27christian.heimessetstatus: open -> pending
resolution: fixed
stage: needs patch -> commit review
2013-07-01 13:18:59python-devsetnosy: + python-dev
messages: + msg192127
2013-07-01 12:12:33christian.heimescreate