-
-
Notifications
You must be signed in to change notification settings - Fork 33.7k
gh-140815: Fix faulthandler for invalid/freed frame #140921
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
Conversation
faulthandler now detects if a frame or a code object is invalid or freed. Add helper functions: * _PyCode_SafeAddr2Line() * _PyFrame_SafeGetCode() * _PyFrame_SafeGetBytecode() * _PyFrame_SafeGetLasti() _PyMem_IsPtrFreed() now also considers the pointer 0x1 as freed.
|
I believe this PR minimizes the chances of getting a segfault. However, I have a feeling that it does not eliminate the possibility entirely. Unfortunately, I don't have a better solution. |
|
Perhaps we could add a new function that attempts to attach the thread state with a timeout. If successful, we will have a more reliable solution. WDYT? |
I don't think that it's worth it. Another faulthandler feature is faulthandler is a debugger, it's not designed to be 100% accurate, but to "just work" in any case (GIL or not, in a signal handler or not, etc.) without taking any lock. |
|
@sergey-miryanov @picnixz: I pushed a change to address your reviews. |
|
LGTM, but I have very little expertise here, so I'll not push approve button :-). |
faulthandler is implemented with multiple heuristics and best effort. It's ok if it does crash, since it should only be triggered when something already goes wrong. Only |
sergey-miryanov
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From point of my little experience, it looks good to me.
|
Thanks! |
| if (_PyMem_IsPtrFreed(ptr)) { | ||
| return NULL; | ||
| } | ||
| PyObject *executable = PyStackRef_AsPyObjectBorrow(f->f_executable); | ||
| if (_PyObject_IsFreed(executable)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this patch work on release builds? I thought _PyMem_IsPtrFreed and _PyObject_IsFreed only worked if debug allocators are enabled.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh. I didn't try on a release build! _PyMem_IsPtrFreed() and _PyObject_IsFreed() work better with debug allocators, you're correct. But they detect freed (reused) memory in some cases in a release build as well. They are just less reliable in release mode.
I pushed more changes to make the heuristics stricter.
|
@picnixz @ZeroIntensity: Do these changes look reasonable to you? Even if they are not perfect, they allow dumping the traceback in more cases. I wrote a script to run the reproducer in a loop (see below). I ran the stress test 3 times.
Stress test: import subprocess, sys
cmd=[sys.executable, 'reproducer.py']
success = 0
while True:
proc = subprocess.run(cmd)
if proc.returncode != 1:
print()
print(f"Exitcode {proc.returncode}")
break
success += 1
print(success)
import faulthandler
import sys
faulthandler.dump_traceback_later(10 * 1e-308, exit=True, file=sys.__stderr__)
assert False |
ZeroIntensity
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a good idea anyway, yeah. We might want to document that faulthandler.dump_traceback_later can sometimes crash during interpreter finalization.
| // Similar to _PyFrame_GetCode(), but return NULL if the frame is invalid or | ||
| // freed. Used by dump_frame() in Python/traceback.c. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be worth adding a comment that this isn't completely reliable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea. I added a sentence for that in the 3 added functions.
|
Merged, thanks for your reviews! |
|
Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.14. |
…40921) faulthandler now detects if a frame or a code object is invalid or freed. Add helper functions: * _PyCode_SafeAddr2Line() * _PyFrame_SafeGetCode() * _PyFrame_SafeGetLasti() _PyMem_IsPtrFreed() now detects pointers in [-0xff, 0xff] range as freed. (cherry picked from commit a84181c) Co-authored-by: Victor Stinner <vstinner@python.org>
|
GH-140981 is a backport of this pull request to the 3.14 branch. |
|
GH-140985 is a backport of this pull request to the 3.13 branch. |
) faulthandler now detects if a frame or a code object is invalid or freed. Add helper functions: * _PyCode_SafeAddr2Line() * _PyFrame_SafeGetCode() * _PyFrame_SafeGetLasti() _PyMem_IsPtrFreed() now detects pointers in [-0xff, 0xff] range as freed. (cherry picked from commit a84181c)
#140981) gh-140815: Fix faulthandler for invalid/freed frame (GH-140921) faulthandler now detects if a frame or a code object is invalid or freed. Add helper functions: * _PyCode_SafeAddr2Line() * _PyFrame_SafeGetCode() * _PyFrame_SafeGetLasti() _PyMem_IsPtrFreed() now detects pointers in [-0xff, 0xff] range as freed. (cherry picked from commit a84181c) Co-authored-by: Victor Stinner <vstinner@python.org>
…140985) gh-140815: Fix faulthandler for invalid/freed frame (#140921) faulthandler now detects if a frame or a code object is invalid or freed. Add helper functions: * _PyCode_SafeAddr2Line() * _PyFrame_SafeGetCode() * _PyFrame_SafeGetLasti() _PyMem_IsPtrFreed() now detects pointers in [-0xff, 0xff] range as freed. (cherry picked from commit a84181c)
faulthandler now detects if a frame or a code object is invalid or freed.
Add helper functions:
_PyMem_IsPtrFreed() now also considers the pointer 0x1 as freed.
faulthandler.dump_traceback_later#140815