classification
Title: Use after free in PyDict_merge
Type: crash Stage: resolved
Components: Interpreter Core Versions: Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: Arfrever, Mark.Shannon, benjamin.peterson, berker.peksag, pkt, python-dev, r.david.murray, rhettinger, serhiy.storchaka
Priority: normal Keywords: patch

Created on 2015-06-08 11:57 by pkt, last changed 2015-07-18 20:52 by Arfrever. This issue is now closed.

Files
File name Uploaded Description Edit
dict_merge.py pkt, 2015-06-08 11:57
24407.patch Mark.Shannon, 2015-07-04 14:37 review
dict_merge.patch benjamin.peterson, 2015-07-04 15:28 review
Messages (11)
msg245001 - (view) Author: paul (pkt) Date: 2015-06-08 11:57
# PyDict_Merge:
# 
# 1       for (i = 0, n = DK_SIZE(other->ma_keys); i < n; i++) {
#             ...
# 3           entry = &other->ma_keys->dk_entries[i];
#             ...
# 2               if (insertdict(mp, entry->me_key,
#                                entry->me_hash,
#                                value) != 0)
#                     return -1;
#             ...
#         }
# 
# 1. n is set once 
# 2. it's possible to run a custom __eq__ method from inside the insertdict. 
#    __eq__ clears the "other" dict. "n" variables is now out of date
# 3. out of bounds read
# 
# CRASH:
# ------
#
# * thread #1: tid = 27715, 0x080d1c1d python`insertdict(mp=0xb71d66f4, key=0x61682044, hash=543582496, value=0xb71d6664) + 132 at dictobject.c:819, name = 'python', stop reason = invalid address (fault address: 0x61682050)
#     frame #0: 0x080d1c1d python`insertdict(mp=0xb71d66f4, key=0x61682044, hash=543582496, value=0xb71d6664) + 132 at dictobject.c:819
#    816      if (ep == NULL) {
#    817          return -1;
#    818      }
# -> 819      assert(PyUnicode_CheckExact(key) || mp->ma_keys->dk_lookup == lookdict);
#    820      Py_INCREF(value);
#    821      MAINTAIN_TRACKING(mp, key, value);
#    822      old_value = *value_addr;
#
msg246065 - (view) Author: paul (pkt) Date: 2015-07-02 10:26
ping
msg246144 - (view) Author: paul (pkt) Date: 2015-07-03 07:45
ping
msg246250 - (view) Author: paul (pkt) Date: 2015-07-04 11:18
ping
msg246251 - (view) Author: Berker Peksag (berker.peksag) * (Python committer) Date: 2015-07-04 11:36
Thanks for the report, paul. Please do not ping an issue after a day.

Quoting from https://docs.python.org/devguide/patch.html?#reviewing

"If your patch has not received any notice from reviewers (i.e., no comment made) after one month, first “ping” the issue on the issue tracker to remind the nosy list that the patch needs a review. If you don’t get a response within a few days after pinging the issue, then you can try emailing python-dev@python.org asking for someone to review your patch."
msg246258 - (view) Author: Mark Shannon (Mark.Shannon) * (Python committer) Date: 2015-07-04 14:03
The tracker won't let me assign this to myself.
Consider it assigned.
msg246260 - (view) Author: Mark Shannon (Mark.Shannon) * (Python committer) Date: 2015-07-04 14:37
There are two parts to this fix.

First, we raise a runtime exception if the other dict is modified during the update/merge.
Second, refcounts must be incremented around the PyDict_GetItem and insertdict calls in case the key or value is otherwise deallocated.
 
Patch attached.
msg246262 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2015-07-04 15:28
Hmm, I just wrote a very similar patch. Tell me what you think. :)
msg246263 - (view) Author: Mark Shannon (Mark.Shannon) * (Python committer) Date: 2015-07-04 15:48
If the tracker had let me assign the issue, you need not have wasted your time. Oh well.

Indeed, your patch looks very similar to mine.
msg246269 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2015-07-04 18:01
Oh, he still might have written the patch, after all there isn't a lot of operational difference between the email that says "assigned to XXX" and the email that contains your text "consider this assigned".

However, Benjamin has given you developer privs on the tracker, so in the future you can use assignment if you wish to :)
msg246293 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2015-07-05 03:54
New changeset 37fed8b02f00 by Benjamin Peterson in branch '3.3':
protect against mutation of the dict during insertion (closes #24407)
https://hg.python.org/cpython/rev/37fed8b02f00

New changeset 75da5acbfbe4 by Benjamin Peterson in branch '3.4':
merge 3.3 (#24407)
https://hg.python.org/cpython/rev/75da5acbfbe4

New changeset 6a7ee97cb0b1 by Benjamin Peterson in branch '3.5':
merge 3.4 (#24407)
https://hg.python.org/cpython/rev/6a7ee97cb0b1

New changeset 88814ddd5e9e by Benjamin Peterson in branch 'default':
merge 3.5 (#24407)
https://hg.python.org/cpython/rev/88814ddd5e9e
History
Date User Action Args
2015-07-18 20:52:29Arfreversetnosy: + Arfrever
2015-07-05 03:54:16python-devsetstatus: open -> closed

nosy: + python-dev
messages: + msg246293

resolution: fixed
stage: needs patch -> resolved
2015-07-04 18:01:33r.david.murraysetnosy: + r.david.murray
messages: + msg246269
2015-07-04 15:48:23Mark.Shannonsetmessages: + msg246263
2015-07-04 15:28:09benjamin.petersonsetfiles: + dict_merge.patch
nosy: + benjamin.peterson
messages: + msg246262

2015-07-04 14:37:21Mark.Shannonsetfiles: + 24407.patch
keywords: + patch
messages: + msg246260
2015-07-04 14:03:20Mark.Shannonsetnosy: + Mark.Shannon
messages: + msg246258
2015-07-04 11:36:28berker.peksagsetnosy: + berker.peksag
messages: + msg246251
2015-07-04 11:18:47pktsetmessages: + msg246250
2015-07-03 07:45:18pktsetmessages: + msg246144
2015-07-02 10:26:08pktsetmessages: + msg246065
2015-06-08 14:20:47serhiy.storchakasetnosy: + rhettinger, serhiy.storchaka

components: + Interpreter Core
stage: needs patch
2015-06-08 11:57:05pktcreate