gh-123083: Fix a potential use-after-free in `STORE_ATTR_WITH_HINT` #123092

corona10 · 2024-08-17T08:27:07Z

Issue: STORE_ATTR_WITH_HINT has potential use-after-free #123083

corona10 · 2024-08-17T08:27:55Z

Lib/test/test_dict.py

        gc.collect()
        self.assertTrue(gc.is_tracked(next(it)))

+    def test_store_evilattr(self):


I am not sure that this is the proper place.

bedevere-bot · 2024-08-17T08:36:12Z

🤖 New build scheduled with the buildbot fleet by @corona10 for commit 6cdacd8 🤖

If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again.

Misc/NEWS.d/next/Core_and_Builtins/2024-08-17-17-26-25.gh-issue-123083.9xWLJ-.rst

bedevere-bot · 2024-08-17T10:47:42Z

🤖 New build scheduled with the buildbot fleet by @corona10 for commit a71d20d 🤖

If you want to schedule another build, you need to add the 🔨 test-with-refleak-buildbots label again.

colesbury · 2024-08-19T15:02:05Z

Python/bytecodes.c

            DEOPT_IF(ep->me_key != name);
            old_value = ep->me_value;
            PyDict_WatchEvent event = old_value == NULL ? PyDict_EVENT_ADDED : PyDict_EVENT_MODIFIED;
            new_version = _PyDict_NotifyEvent(tstate->interp, event, dict, name, PyStackRef_AsPyObjectBorrow(value));


I think we should structure this to match the ordering in dictobject.c:

cpython/Objects/dictobject.c

Lines 1693 to 1707 in b6d0a40

MAINTAIN_TRACKING(mp, key, value);

PyObject *old_value = mp->ma_values->values[ix];

if (old_value == NULL) {

uint64_t new_version = _PyDict_NotifyEvent(interp, PyDict_EVENT_ADDED, mp, key, value);

STORE_SPLIT_VALUE(mp, ix, Py_NewRef(value));

_PyDictValues_AddToInsertionOrder(mp->ma_values, ix);

STORE_USED(mp, mp->ma_used + 1);

mp->ma_version_tag = new_version;

}

else {

uint64_t new_version = _PyDict_NotifyEvent(interp, PyDict_EVENT_MODIFIED, mp, key, value);

STORE_SPLIT_VALUE(mp, ix, Py_NewRef(value));

mp->ma_version_tag = new_version;

Py_DECREF(old_value);

}

Update GC tracking if necessary

Notify event

Store value and version tag

Decref old value

markshannon · 2024-08-19T15:48:26Z

Could you add a comment, specifically why this is the correct order?
It isn't obvious and it will be easy to reintroduce the error without some warning not to.

corona10 · 2024-08-20T15:33:06Z

Thank you for the kind review!
I followed the suggestions. Please take a look. :)

corona10 · 2024-08-22T09:51:18Z

@colesbury @markshannon gentle ping :)

markshannon

This looks fine to me. But let @colesbury review it before merging.

colesbury

LGTM too

miss-islington-app · 2024-08-22T14:49:12Z

Thanks @corona10 for the PR 🌮🎉.. I'm working now to backport this PR to: 3.12, 3.13.
🐍🍒⛏🤖

miss-islington-app · 2024-08-22T14:49:16Z

Sorry, @corona10, I could not cleanly backport this to 3.13 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 297f2e093ec95800ae2184330b8408c875523467 3.13

miss-islington-app · 2024-08-22T14:49:31Z

Sorry, @corona10, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker 297f2e093ec95800ae2184330b8408c875523467 3.12

…R_WITH_HINT`` (pythongh-123092) (cherry picked from commit 297f2e0) Co-authored-by: Donghee Na <[email protected]>

bedevere-app · 2024-08-22T15:24:18Z

GH-123235 is a backport of this pull request to the 3.13 branch.

bedevere-app · 2024-08-22T15:53:00Z

GH-123237 is a backport of this pull request to the 3.12 branch.

#123235) [3.13] gh-123083: Fix a potential use-after-free in ``STORE_ATTR_WITH_HINT`` (gh-123092) (cherry picked from commit 297f2e0)

#123237) [3.12] gh-123083: Fix a potential use-after-free in ``STORE_ATTR_WITH_HINT`` (gh-123092) (cherry picked from commit 297f2e0)

corona10 requested review from Fidget-Spinner, brandtbucher and colesbury August 17, 2024 08:27

corona10 requested a review from markshannon as a code owner August 17, 2024 08:27

bedevere-app bot mentioned this pull request Aug 17, 2024

STORE_ATTR_WITH_HINT has potential use-after-free #123083

Closed

bedevere-app bot added the awaiting core review label Aug 17, 2024

corona10 added needs backport to 3.12 only security fixes needs backport to 3.13 bugs and security fixes labels Aug 17, 2024

corona10 commented Aug 17, 2024

View reviewed changes

corona10 added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Aug 17, 2024

bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Aug 17, 2024

picnixz reviewed Aug 17, 2024

View reviewed changes

Misc/NEWS.d/next/Core_and_Builtins/2024-08-17-17-26-25.gh-issue-123083.9xWLJ-.rst Outdated Show resolved Hide resolved

corona10 changed the title ~~gh-123083: Fix STORE_ATTR_WITH_HINT has potential use-after-free~~ gh-123083: Fix a potential use-after-free in STORE_ATTR_WITH_HINT Aug 17, 2024

pythongh-123083: Fix STORE_ATTR_WITH_HINT has potential use-after-free

c732019

corona10 added the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Aug 17, 2024

bedevere-bot removed the 🔨 test-with-refleak-buildbots Test PR w/ refleak buildbots; report in status section label Aug 17, 2024

colesbury reviewed Aug 19, 2024

View reviewed changes

Address code review

59ea4f2

corona10 force-pushed the gh-123083 branch from a71d20d to 59ea4f2 Compare August 20, 2024 15:28

corona10 requested a review from methane as a code owner August 20, 2024 15:28

Address code review

4237506

corona10 requested a review from colesbury August 20, 2024 15:30

Merge remote-tracking branch 'upstream/main' into pythongh-123083

2603cb4

markshannon approved these changes Aug 22, 2024

View reviewed changes

bedevere-app bot removed the awaiting core review label Aug 22, 2024

bedevere-app bot added the awaiting merge label Aug 22, 2024

colesbury approved these changes Aug 22, 2024

View reviewed changes

corona10 merged commit 297f2e0 into python:main Aug 22, 2024

bedevere-app bot removed the awaiting merge label Aug 22, 2024

miss-islington-app bot assigned corona10 Aug 22, 2024

corona10 added a commit to corona10/cpython that referenced this pull request Aug 22, 2024

[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 <[email protected]>

corona10 added a commit to corona10/cpython that referenced this pull request Aug 22, 2024

[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 <[email protected]>

corona10 mentioned this pull request Aug 22, 2024

[3.13] gh-123083: Fix a potential use-after-free in ``STORE_ATTR_WITH… #123235

Merged

bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Aug 22, 2024

corona10 mentioned this pull request Aug 22, 2024

[3.12] gh-123083: Fix a potential use-after-free in ``STORE_ATTR_WITH… #123237

Merged

bedevere-app bot removed the needs backport to 3.12 only security fixes label Aug 22, 2024

corona10 added a commit that referenced this pull request Aug 22, 2024

[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 pull request Aug 22, 2024

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

	MAINTAIN_TRACKING(mp, key, value);
	PyObject *old_value = mp->ma_values->values[ix];
	if (old_value == NULL) {
	uint64_t new_version = _PyDict_NotifyEvent(interp, PyDict_EVENT_ADDED, mp, key, value);
	STORE_SPLIT_VALUE(mp, ix, Py_NewRef(value));
	_PyDictValues_AddToInsertionOrder(mp->ma_values, ix);
	STORE_USED(mp, mp->ma_used + 1);
	mp->ma_version_tag = new_version;
	}
	else {
	uint64_t new_version = _PyDict_NotifyEvent(interp, PyDict_EVENT_MODIFIED, mp, key, value);
	STORE_SPLIT_VALUE(mp, ix, Py_NewRef(value));
	mp->ma_version_tag = new_version;
	Py_DECREF(old_value);
	}

Uh oh!

gh-123083: Fix a potential use-after-free in STORE_ATTR_WITH_HINT #123092

gh-123083: Fix a potential use-after-free in STORE_ATTR_WITH_HINT #123092

Uh oh!

Conversation

corona10 commented Aug 17, 2024 • edited by bedevere-app bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

corona10 Aug 17, 2024

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Aug 17, 2024

Uh oh!

Uh oh!

bedevere-bot commented Aug 17, 2024

Uh oh!

colesbury Aug 19, 2024

Choose a reason for hiding this comment

Uh oh!

markshannon commented Aug 19, 2024

Uh oh!

corona10 commented Aug 20, 2024

Uh oh!

corona10 commented Aug 22, 2024

Uh oh!

markshannon left a comment

Choose a reason for hiding this comment

Uh oh!

colesbury left a comment

Choose a reason for hiding this comment

Uh oh!

miss-islington-app bot commented Aug 22, 2024

Uh oh!

miss-islington-app bot commented Aug 22, 2024

Uh oh!

miss-islington-app bot commented Aug 22, 2024

Uh oh!

bedevere-app bot commented Aug 22, 2024

Uh oh!

bedevere-app bot commented Aug 22, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

gh-123083: Fix a potential use-after-free in `STORE_ATTR_WITH_HINT` #123092

gh-123083: Fix a potential use-after-free in `STORE_ATTR_WITH_HINT` #123092

corona10 commented Aug 17, 2024 •

edited by bedevere-app bot

Loading