Skip to content

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!

@github-actions
Copy link

github-actions bot commented Sep 24, 2024

🔧 Report generated by pr-comment-cppcheck

CppCheck Report

Warnings (6)

Style Violations (161)

@github-actions
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

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

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

if (thrd) {
thrd->exitCrashHandler();
}
longjmp(*(jmp_buf *)vm_thread->exception(), 1);
Copy link
Contributor

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
Contributor

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.

4 participants