Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use after free in PyDict_merge #68595

Closed
pkt mannequin opened this issue Jun 8, 2015 · 11 comments
Closed

Use after free in PyDict_merge #68595

pkt mannequin opened this issue Jun 8, 2015 · 11 comments
Labels
interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump

Comments

@pkt
Copy link
Mannequin

pkt mannequin commented Jun 8, 2015

BPO 24407
Nosy @rhettinger, @benjaminp, @bitdancer, @markshannon, @berkerpeksag, @serhiy-storchaka
Files
  • dict_merge.py
  • 24407.patch
  • dict_merge.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 = None
    closed_at = <Date 2015-07-05.03:54:16.528>
    created_at = <Date 2015-06-08.11:57:05.103>
    labels = ['interpreter-core', 'type-crash']
    title = 'Use after free in PyDict_merge'
    updated_at = <Date 2015-07-18.20:52:29.640>
    user = 'https://bugs.python.org/pkt'

    bugs.python.org fields:

    activity = <Date 2015-07-18.20:52:29.640>
    actor = 'Arfrever'
    assignee = 'none'
    closed = True
    closed_date = <Date 2015-07-05.03:54:16.528>
    closer = 'python-dev'
    components = ['Interpreter Core']
    creation = <Date 2015-06-08.11:57:05.103>
    creator = 'pkt'
    dependencies = []
    files = ['39659', '39860', '39861']
    hgrepos = []
    issue_num = 24407
    keywords = ['patch']
    message_count = 11.0
    messages = ['245001', '246065', '246144', '246250', '246251', '246258', '246260', '246262', '246263', '246269', '246293']
    nosy_count = 9.0
    nosy_names = ['rhettinger', 'benjamin.peterson', 'Arfrever', 'r.david.murray', 'Mark.Shannon', 'python-dev', 'berker.peksag', 'serhiy.storchaka', 'pkt']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'crash'
    url = 'https://bugs.python.org/issue24407'
    versions = ['Python 3.5']

    @pkt
    Copy link
    Mannequin Author

    pkt mannequin commented Jun 8, 2015

    # 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;

    @pkt pkt mannequin added the type-crash A hard crash of the interpreter, possibly with a core dump label Jun 8, 2015
    @serhiy-storchaka serhiy-storchaka added the interpreter-core (Objects, Python, Grammar, and Parser dirs) label Jun 8, 2015
    @pkt
    Copy link
    Mannequin Author

    pkt mannequin commented Jul 2, 2015

    ping

    2 similar comments
    @pkt
    Copy link
    Mannequin Author

    pkt mannequin commented Jul 3, 2015

    ping

    @pkt
    Copy link
    Mannequin Author

    pkt mannequin commented Jul 4, 2015

    ping

    @berkerpeksag
    Copy link
    Member

    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."

    @markshannon
    Copy link
    Member

    The tracker won't let me assign this to myself.
    Consider it assigned.

    @markshannon
    Copy link
    Member

    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.

    @benjaminp
    Copy link
    Contributor

    Hmm, I just wrote a very similar patch. Tell me what you think. :)

    @markshannon
    Copy link
    Member

    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.

    @bitdancer
    Copy link
    Member

    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 :)

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Jul 5, 2015

    New changeset 37fed8b02f00 by Benjamin Peterson in branch '3.3':
    protect against mutation of the dict during insertion (closes bpo-24407)
    https://hg.python.org/cpython/rev/37fed8b02f00

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

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

    New changeset 88814ddd5e9e by Benjamin Peterson in branch 'default':
    merge 3.5 (bpo-24407)
    https://hg.python.org/cpython/rev/88814ddd5e9e

    @python-dev python-dev mannequin closed this as completed Jul 5, 2015
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    interpreter-core (Objects, Python, Grammar, and Parser dirs) type-crash A hard crash of the interpreter, possibly with a core dump
    Projects
    None yet
    Development

    No branches or pull requests

    5 participants