Skip to content

gh-109496: Detect Py_DECREF() after dealloc in debug mode #109539

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

Merged
merged 1 commit into from
Sep 18, 2023

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Sep 18, 2023

On a Python built in debug mode, Py_DECREF() now calls _Py_NegativeRefcount() if the object is a dangling pointer to deallocated memory: memory filled with 0xDD "dead byte" by the debug hook on memory allocators. The fix is to check the reference count before checking for _Py_IsImmortal().

Add test_decref_freed_object() to test_capi.test_misc.

@vstinner
Copy link
Member Author

@vstinner
Copy link
Member Author

Fix regression introduced in Python 3.12 PEP 683 Immortal Objects: https://peps.python.org/pep-0683/

On a Python built in debug mode, Py_DECREF() now calls
_Py_NegativeRefcount() if the object is a dangling pointer to
deallocated memory: memory filled with 0xDD "dead byte" by the debug
hook on memory allocators. The fix is to check the reference count
*before* checking for _Py_IsImmortal().

Add test_decref_freed_object() to test_capi.test_misc.
@vstinner vstinner force-pushed the detect_double_decref branch from e96fc04 to 8ea43e9 Compare September 18, 2023 14:32
@vstinner
Copy link
Member Author

vstinner force-pushed the detect_double_decref branch from e96fc04 to 8ea43e9

Sorry, I forgot a local change to fix a typo in the NEWS entry.

@vstinner vstinner enabled auto-merge (squash) September 18, 2023 14:38
Comment on lines +2048 to +2049
Py_DECREF(obj);
// obj is a now a dangling pointer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a chance this could be an actual pointer to deallocated memory, or memory written to by another thread? Would it be safer to just call Py_DECREF on a fake object which is explicitly memset to 0xDD bytes?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer to use a functional test: not mock anything here.

This test is very fragile, it makes sure that a dangling pointer has a determinstic behavior. Problem: by definition, it has an undefined behavior :-) But in practice, almost always, the memory should remain around and should just be filled by 0xDD bytes.

If tomorrow the test starts to fail randomly, sure, we can rewrite it by "mocking" the first DECREF() and just pass memory filled by 0xDD byte pattern.

test_capi should not be run in a process with more than 1 thread. libregrtest works hard to make sure that previous tests don't leak tests. Also, it's strongly recommended to run tests with -jN: each test file is run in a fresh process, so with a known number of threads: just 1 :-)

@vstinner vstinner merged commit 0bb0d88 into python:main Sep 18, 2023
@vstinner vstinner deleted the detect_double_decref branch September 18, 2023 14:59
@miss-islington
Copy link
Contributor

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 18, 2023
…onGH-109539)

On a Python built in debug mode, Py_DECREF() now calls
_Py_NegativeRefcount() if the object is a dangling pointer to
deallocated memory: memory filled with 0xDD "dead byte" by the debug
hook on memory allocators. The fix is to check the reference count
*before* checking for _Py_IsImmortal().

Add test_decref_freed_object() to test_capi.test_misc.
(cherry picked from commit 0bb0d88)

Co-authored-by: Victor Stinner <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Sep 18, 2023

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

@bedevere-app bedevere-app bot removed the needs backport to 3.12 only security fixes label Sep 18, 2023
Yhg1s pushed a commit that referenced this pull request Sep 18, 2023
…109539) (#109545)

gh-109496: Detect Py_DECREF() after dealloc in debug mode (GH-109539)

On a Python built in debug mode, Py_DECREF() now calls
_Py_NegativeRefcount() if the object is a dangling pointer to
deallocated memory: memory filled with 0xDD "dead byte" by the debug
hook on memory allocators. The fix is to check the reference count
*before* checking for _Py_IsImmortal().

Add test_decref_freed_object() to test_capi.test_misc.
(cherry picked from commit 0bb0d88)

Co-authored-by: Victor Stinner <[email protected]>
csm10495 pushed a commit to csm10495/cpython that referenced this pull request Sep 28, 2023
…on#109539)

On a Python built in debug mode, Py_DECREF() now calls
_Py_NegativeRefcount() if the object is a dangling pointer to
deallocated memory: memory filled with 0xDD "dead byte" by the debug
hook on memory allocators. The fix is to check the reference count
*before* checking for _Py_IsImmortal().

Add test_decref_freed_object() to test_capi.test_misc.
@pablogsal
Copy link
Member

@vstinner This PR broke the AMD64 Arch Linux Asan Debug 3.12 builder. I will make a fix

@pablogsal
Copy link
Member

Nevermind I can see this was fixed in 0a31ff0050e but the backport didn't land

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants