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
JDK-8289477: Memory corruption with CPU_ALLOC, CPU_FREE on muslc #9328
JDK-8289477: Memory corruption with CPU_ALLOC, CPU_FREE on muslc #9328
Conversation
👋 Welcome back stuefe! A progress list of the required criteria for merging this PR into |
9d9125e
to
b022763
Compare
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.
This seems to rely on the compiler not recognising that the single use of the new function could be inlined into the original.
No. Using free() (without global scope "::") inside os::Linux:: will resolve the the nearest outer scope os::free(). Using free() inside a global function will not do that since free() is already in global scope, so gets resolved to ::free(). This is not dependent on any inlining decisions. |
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.
Okay so the "binding" of free()
will happen before any potential inlining could occur.
It is a nice simple workaround.
Thanks.
@tstuefe This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be:
You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 8 new commits pushed to the
Please see this link for an up-to-date comparison between the source branch of this pull request and the ➡️ To integrate this PR with the above commit message to the |
Thank you! |
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.
LGTM, thanks for fixing.
Thanks @dholmes-ora and @RealCLanger. /integrate |
Going to push as commit da6d1fc.
Your commit was automatically rebased without conflicts. |
@tstuefe Unknown command |
@tstuefe Unknown command |
@tstuefe Unknown command |
In
os::Linux::active_processor_count()
, we use the CPU_xxx macros to manage sets of CPU information.muslc defines those macros to call
calloc(3)
andfree(3)
:whereas glibc uses intermediate functions:
which in the end also takes from C-heap, but those calls are not inlined.
So, on muslc we call
calloc()
andfree()
. Call happens inside theos::Linux
namespace,free()
resolves toos::free()
. We have no wrapper in os for calloc though, socalloc()
calls into muslc right away.That means we have raw ::malloc() -> os::free(), which is unbalanced. Raw
::malloc()
does not write the headeros::free()
expects. If NMT is on, we assert now, because NMT does not find its header in os::free().This can be very easily reproduced by starting an Alpine VM with NMT on (or, a debug VM) and
-XX:+UnlockDiagnosticVMOptions -XX:+UseCpuAllocPath
. This causes a NMT fence alert right away:The position of the musl devs is that "calloc" and "free" are reserved words in C, and should not be used [1]. I think they are right. The way we reuse known C- and Posix symbol names in the os namespace has bitten me in the past in similar cases.
The fix is minimally invasive for easy backporting. I just move the content of
os::Linux::active_processor_count()
into its own local function, outside the os namespace. That way, CPU_FREE cannot pick upos::free()
accidentally, and the error is fixed.I really would like a more thorough solution though, renaming all the potential conflict candidates, but leave that for a follow-up RFE.
[1] https://www.openwall.com/lists/musl/2022/06/29/3
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk pull/9328/head:pull/9328
$ git checkout pull/9328
Update a local copy of the PR:
$ git checkout pull/9328
$ git pull https://git.openjdk.org/jdk pull/9328/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 9328
View PR using the GUI difftool:
$ git pr show -t 9328
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/9328.diff