gh-139269: Fix unaligned memory access in JIT code patching functions #139271
Conversation
ashm-dev
requested review from
brandtbucher,
savannahostrowski and
diegorusso
as code owners
|
Lol, when I tried to restart the tests with these changes, I now got a Segmentation Fault, now I'm starting a new bug. |
Misc/NEWS.d/next/Core_and_Builtins/2025-09-23-21-01-12.gh-issue-139269.1rIaxy.rst
Outdated
Show resolved
Hide resolved
|
#139288 |
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 <[email protected]>
|
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 |
Oh, that's why what I did had no effect. I used -O3 :') Ok. So, all compilers and supported archs seem to be optimized properly, except when hints are not given sometimes but that's only on an old architecture. On my side, I would say it's fine. However, I'd like to know the opinion of JIT experts and maybe some benchmarks as well to see if this is true in practice. JIT+PGO would suffice (LTO takes some time...). |
|
And since we accept these changes, this issue is already becoming relevant) |
Oh right, I forgot about this one. However, this is annoying then. Should we first care about fixing the possible tests before actually fixing this specific UB? Because if we merge this PR, then we will have Do you have a fix for that issue in particular? Because I would prefer not fixing the UB here rather than having a real crash on ASAN. |
picnixz
dismissed
their stale review
I want to avoid merging a fix that can make ASAN fail.
Yes, so far the description of the problem is in the form of a hypothesis, now I'm looking for options to fix it. A preceding test in test_asyncio is corrupting some internal state (e.g., memory layout, an object's type, or internal JIT caches). This corruption does not cause an immediate crash but leads to a fatal error later when the JIT compiler attempts to process code during the "victim" test. |
|
@picnixz The JIT optimizer had a memory leak because it wasn't tracking dependencies on global and builtin namespaces when encountering |
|
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 |
|
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.
The issue still persists in debug builds and I'm currently working on fixing that.
If this is the case, then the fix should be instead complete. I don't want a fix that is only working for release builds. I'll try to review the complete fix later. Also, is it possible to have a reproducer that shows the leak without having to enable UBSan? UBs here shouldn't be critical I guess, but if there is a crash because of the memcpy() fixes, then the issue might already exist, in which case, I would like a reproducer and an independent fix.
| case LOAD_GLOBAL: | ||
| _Py_BloomFilter_Add(dependencies, frame->f_globals); | ||
| _Py_BloomFilter_Add(dependencies, frame->f_builtins); | ||
| _Py_FALLTHROUGH; |
There was a problem hiding this comment.
I'm not well-versed enough here to be sure at first glance that this is correct. So I'd wait for Mark's input, or anyone else that can confirm this is correct.
There was a problem hiding this comment.
This looks wrong to me.
@ashm-dev why did you make this change?
There was a problem hiding this comment.
The release build was crashing with a segfault without this change. There’s a related issue linked in the PR, although it’s currently closed.
There was a problem hiding this comment.
That doesn't explain why you made this change. It seems unrelated to the issue.
This change is incorrect: LOAD_GLOBAL doesn't introduce a dependency on the globals or builtins. A trace containing LOAD_GLOBAL remains valid if either are modified.
|
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 |
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