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.

Sign up for GitHub

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

Jump to bottom

STORE_ATTR_WITH_HINT has potential use-after-free #123083

Closed
colesbury opened this issue Aug 16, 2024 · 1 comment
Closed

STORE_ATTR_WITH_HINT has potential use-after-free #123083

colesbury opened this issue Aug 16, 2024 · 1 comment
Labels
3.12 bugs and security fixes 3.13 bugs and security fixes 3.14 new features, bugs and security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error

Comments

@colesbury
Copy link
Contributor

colesbury commented Aug 16, 2024 •
edited by bedevere-app bot
Loading

Bug report

The order of operations in STORE_ATTR_WITH_HINT differs from the dictionary implementation in a way that is not safe:

cpython/Python/bytecodes.c

Lines 2235 to 2242 in 35d8ac7

new_version = _PyDict_NotifyEvent(tstate->interp, event, dict, name, PyStackRef_AsPyObjectBorrow(value));
ep->me_value = PyStackRef_AsPyObjectSteal(value);
Py_XDECREF(old_value);
STAT_INC(STORE_ATTR, hit);
/* Ensure dict is GC tracked if it needs to be */
if (!_PyObject_GC_IS_TRACKED(dict) && _PyObject_GC_MAY_BE_TRACKED(PyStackRef_AsPyObjectBorrow(value))) {
_PyObject_GC_TRACK(dict);
}

It's not safe to call _PyObject_GC_MAY_BE_TRACKED(value) after the Py_XDECREF call. The dictionary may hold the only strong reference to value in ep->me_value, and that can be modified during the Py_XDECREF call.

Note that dictobject.c does the tracking before modifying the dictionary -- not after it -- and so avoids this problem.

Linked PRs
@colesbury colesbury added type-bug An unexpected behavior, bug, or error interpreter-core (Objects, Python, Grammar, and Parser dirs) 3.12 bugs and security fixes 3.13 bugs and security fixes 3.14 new features, bugs and security fixes labels Aug 16, 2024
@colesbury
Copy link
Contributor Author

This segfaults in 3.11 to 3.14. The crash is more reliable in debug builds:

import gc

class MyObject:
    pass

class EvilAttr:
    def __init__(self, dict):
        self.dict = dict

    def __del__(self):
        if 'attr' in self.dict:
            del self.dict['attr']
        gc.collect()  # untracks dict

def set_attr(obj):
    obj.attr = EvilAttr(obj.__dict__)

def main():
    obj = MyObject()
    obj.__dict__ = {}
    for _ in range(10):
        set_attr(obj)


if __name__ == "__main__":
    main()

corona10 added a commit to corona10/cpython that referenced this issue Aug 17, 2024
@corona10
pythongh-123083: Fix STORE_ATTR_WITH_HINT has potential use-after-free
6cdacd8
@bedevere-app bedevere-app bot mentioned this issue Aug 17, 2024
Merged
corona10 added a commit to corona10/cpython that referenced this issue Aug 20, 2024
@corona10
pythongh-123083: Fix STORE_ATTR_WITH_HINT has potential use-after-free
c732019
corona10 added a commit to corona10/cpython that referenced this issue Aug 21, 2024
@corona10
Merge remote-tracking branch 'upstream/main' into pythongh-123083
2603cb4
corona10 added a commit that referenced this issue Aug 22, 2024
@corona10
gh-123083: Fix a potential use-after-free in STORE_ATTR_WITH_HINT (…
297f2e0 
…gh-123092)
corona10 added a commit to corona10/cpython that referenced this issue Aug 22, 2024
@corona10
[3.13] pythongh-123083: Fix a potential use-after-free in ``STORE_ATT…
0ab4e42 
…R_WITH_HINT`` (pythongh-123092)

(cherry picked from commit 297f2e0)

Co-authored-by: Donghee Na <donghee.na@python.org>
corona10 added a commit to corona10/cpython that referenced this issue Aug 22, 2024
@corona10
[3.13] pythongh-123083: Fix a potential use-after-free in ``STORE_ATT…
1c16bdf 
…R_WITH_HINT`` (pythongh-123092)

(cherry picked from commit 297f2e0)

Co-authored-by: Donghee Na <donghee.na@python.org>
This was referenced Aug 22, 2024
Merged
Merged
corona10 added a commit that referenced this issue Aug 22, 2024
@corona10
[3.13] gh-123083: Fix a potential use-after-free in ``STORE_ATTR_WITH… (
6cd67e4 
#123235)

[3.13] gh-123083: Fix a potential use-after-free in ``STORE_ATTR_WITH_HINT`` (gh-123092)
(cherry picked from commit 297f2e0)
corona10 added a commit that referenced this issue Aug 22, 2024
@corona10
[3.12] gh-123083: Fix a potential use-after-free in ``STORE_ATTR_WITH… (
6e68559 
#123237)

[3.12] gh-123083: Fix a potential use-after-free in ``STORE_ATTR_WITH_HINT`` (gh-123092)
(cherry picked from commit 297f2e0)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
3.12 bugs and security fixes 3.13 bugs and security fixes 3.14 new features, bugs and security fixes interpreter-core (Objects, Python, Grammar, and Parser dirs) type-bug An unexpected behavior, bug, or error
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@colesbury @corona10