-
-
Notifications
You must be signed in to change notification settings - Fork 33.2k
gh-139269: Fix unaligned memory access in JIT code patching functions #139271
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
|
Lol, when I tried to restart the tests with these changes, I now got a Segmentation Fault, now I'm starting a new bug. |
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.
Looks good to me
Misc/NEWS.d/next/Core_and_Builtins/2025-09-23-21-01-12.gh-issue-139269.1rIaxy.rst
Outdated
Show resolved
Hide resolved
|
#139288 |
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.
Fixed.
Misc/NEWS.d/next/Core_and_Builtins/2025-09-23-21-01-12.gh-issue-139269.1rIaxy.rst
Outdated
Show resolved
Hide resolved
Misc/NEWS.d/next/Core_and_Builtins/2025-09-23-21-01-12.gh-issue-139269.1rIaxy.rst
Outdated
Show resolved
Hide resolved
…e-139269.1rIaxy.rst Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
|
I'll repeat my question as it's now hidden (I should have made a standalone comment in the first place):
|
The sanitizers aren't flagging any more alignment issues after my recent fixes, which suggests the code paths covered by the tests are now clean. Is there a specific spot in the AArch64 code you think I should look at? |
FTR, AArch64 patches are only used for AArch64 so be sure to test this on the corresponding architecture as well (in the reported issue, I see that the architecture is x86_64, but I don't know if you have access to an Apple Darwin with ARM64). Note that
Not really, just that all aarch64-based functions use |
The Arm runners should cover Linux, Windows and macOS on Arm. They seem all to pass but also we are not running with the JIT. Maybe we should have some coverage for that? Besides I'm pondering the cost of introducing |
|
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
|
@picnixz
So maybe you could consider merging my current changes and reopening my issue related to this bug, since it appears to be the same underlying problem. |
|
I have made the requested changes; please review again |
|
Thanks for making the requested changes! @picnixz: please review the changes made to this pull request. |
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.
maybe you could consider merging my current changes and reopening my #139288 related to this bug,
Since this issue first considered the problem as a consequence of this PR, I would prefer a fresh one with the new independent reproducer and the complete traceback. Are the tracebacks actually the same whether you include this PR or not? (and check with this new reproducer)
|
@picnixz Understood—I’ll open a new issue with an updated description, an independent reproducer, and the full traceback; you’re right that this is the better approach. Also, the segmentation faults are identical with and without that PR. Logs/tracebacks below: fix-version
main-version
|
In the future, prefer pasting the content of these into the issue without having external files. It's easier for readers. You can hide the long text by using a |
|
Please avoid merging main into the branch if there is nothing to do. I'm ok with this PR but I still want another core dev to approve this one. |
|
I'm not sure if I bumped into this in #140310. But some UB is causing random fields in the frame to get overridden/read garbage values read out. Only when JIT is on. I'm inclined to merge to get rid of random UB in the JIT. |
This PR fixes a series of misaligned memory access errors in the JIT engine that were causing segmentation faults when running tests with AddressSanitizer (ASan) and UndefinedBehaviorSanitizer (UBSan) enabled.
The unsafe direct pointer casts in
patch_32,patch_32r,patch_64, andpatch_x86_64_32rxhave been replaced withmemcpyto ensure safe, alignment-agnostic memory operations.patch_*functions #139269