Skip to content
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

Improve robustness of the crash signal handler #134

Merged
merged 3 commits into from
Oct 11, 2024
Merged

Conversation

jbachorik
Copy link
Collaborator

@jbachorik jbachorik commented Sep 24, 2024

What does this PR do?:
This is an attempt to increase the robustness of the crash signal handler by

  • preventing rerunning the signal setup routine (quite unlikely to happen, but just in case)
  • using separate stack space for the crash signal handlers to avoid triggering stack overflow
  • using TLS guard to prevent recursive crash handling in case the handler itself would cause sigseg
  • checking the PC for not being the cause of the original fault - if it is, we can not meaningfully recover and just call the JVM crash handler

Motivation:

Additional Notes:

How to test the change?:

For Datadog employees:

  • If this PR touches code that signs or publishes builds or packages, or handles
    credentials of any kind, I've requested a review from @DataDog/security-design-and-guidance.
  • This PR doesn't touch any of that.
  • JIRA: PROF-10605

Unsure? Have a question? Request a review!

Copy link

github-actions bot commented Sep 24, 2024

🔧 Report generated by pr-comment-cppcheck

CppCheck Report

Warnings (6)

Style Violations (161)

Copy link

github-actions bot commented Sep 24, 2024

🔧 Report generated by pr-comment-scanbuild

Scan-Build Report

User:runner@fv-az985-191
Working Directory:/home/runner/work/java-profiler/java-profiler/ddprof-lib/src/test/make
Command Line:make -j4 clean all
Clang Version:Ubuntu clang version 14.0.0-1ubuntu1.1
Date:Thu Oct 10 15:34:17 2024

Bug Summary

Bug TypeQuantityDisplay?
All Bugs5
Logic error
Assigned value is garbage or undefined1
Dereference of null pointer3
Unused code
Dead nested assignment1

Reports

Bug Group Bug Type ▾ File Function/Method Line Path Length
Logic errorAssigned value is garbage or undefineddwarf.cppparseInstructions24420
Unused codeDead nested assignmentvmStructs.cppcheckNativeBinding8791
Logic errorDereference of null pointersafeAccess.hload3318
Logic errorDereference of null pointersymbols_linux.hElfParser12932
Logic errorDereference of null pointerflightRecorder.cppflush15048

@jbachorik jbachorik force-pushed the jb/sigseg_handler branch 5 times, most recently from 81680ef to eb090dc Compare September 24, 2024 18:01
@jbachorik jbachorik marked this pull request as ready for review September 24, 2024 18:01
@jbachorik jbachorik marked this pull request as draft September 25, 2024 09:14
@jbachorik jbachorik force-pushed the jb/sigseg_handler branch 2 times, most recently from 573e392 to e5a8012 Compare September 25, 2024 12:32
@jbachorik jbachorik marked this pull request as ready for review September 25, 2024 13:00
@jbachorik jbachorik merged commit 44c2f8e into main Oct 11, 2024
31 checks passed
@jbachorik jbachorik deleted the jb/sigseg_handler branch October 11, 2024 08:33
@github-actions github-actions bot added this to the 1.16.0 milestone Oct 11, 2024
jbachorik added a commit that referenced this pull request Oct 14, 2024
@@ -906,14 +897,30 @@ void Profiler::busHandler(int signo, siginfo_t *siginfo, void *ucontext) {
}

bool Profiler::crashHandler(int signo, siginfo_t *siginfo, void *ucontext) {
ProfiledThread* thrd = ProfiledThread::current();
if (thrd != nullptr && !thrd->enterCrashHandler()) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this puts some weight on the thread object to be always valid or null

VMThread *vm_thread = VMThread::current();
if (vm_thread != NULL && sameStack(vm_thread->exception(), &vm_thread)) {
if (thrd) {
thrd->exitCrashHandler();
}
longjmp(*(jmp_buf *)vm_thread->exception(), 1);
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we std::atomic_thread_fence ?

VMThread *vm_thread = VMThread::current();
if (vm_thread != NULL && sameStack(vm_thread->exception(), &vm_thread)) {
if (thrd) {
thrd->exitCrashHandler();
Copy link
Collaborator

Choose a reason for hiding this comment

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

This does not work, we are doing a longjmp, so we should reset before exiting. Here the increments will build up.

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.

3 participants