Skip to content

[FIX] replace fprintf(), exit() with write(), _exit() to async-safe at SIGSEGV signal handler#433

Closed
yoochangyeon wants to merge 1 commit into
DataDog:mainfrom
yoochangyeon:SIGSEGV-async-safe-handler
Closed

[FIX] replace fprintf(), exit() with write(), _exit() to async-safe at SIGSEGV signal handler#433
yoochangyeon wants to merge 1 commit into
DataDog:mainfrom
yoochangyeon:SIGSEGV-async-safe-handler

Conversation

@yoochangyeon

@yoochangyeon yoochangyeon commented Jan 15, 2025

Copy link
Copy Markdown
Contributor

What does this PR do?

This PR replaces the usage of non-async-safe functions (fprintf, printf, exit) in the SIGSEGV handler with async-safe alternatives (write, _exit). This aims to ensure more reliable behavior when a segmentation fault occurs.

Motivation

https://docs.oracle.com/cd/E19455-01/806-5257/gen-26/index.html

When a SIGSEGV is triggered, the program’s state may already be corrupt. Using non-async-safe functions inside the signal handler can lead to reentrancy issues or additional faults. By switching to write and _exit, we minimize these risks and maintain a more stable crash-handling sequence.

Additional Notes

  • backtrace_symbols_fd is left as is for now, even though it’s not guaranteed to be fully async-safe, in order to keep the existing logic mostly unchanged.
  • By TODO comment in the code, it appears that removing fprintf-family functions was already planned, suggesting that this problem was anticipated.
  • I am not c++ developer, but the issue has already arisen and was known. This code must be updated in an async-safe manner, whether it’s through my proposal or another approach.

How to test the change?

  • Trigger a SIGSEGV intentionally and verify that the updated handler logs the message and terminates without additional faults.
  • Compare the behavior with the previous handler to ensure no unexpected regressions.
  • Observe whether the logs or any diagnostic outputs align with the expected error information under stress tests.

Comment thread src/ddprof.cc
(void)fprintf(
stderr, "ddprof[%d]: <%.*s> has encountered an error and will exit\n",
getpid(), static_cast<int>(str_version().size()), str_version().data());
const char msg[] = "ddprof: encountered an error and will exit\n";

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for raising this. We have more work to improve crash tracking, though this would be a reasonable first step.
You are removing a minor info here (the si->si_addr), I'll add this back in while keeping your changes.
The GLIBC version can stay as is as we do not deploy that version (we use a musl build for the releases).

@r1viollet

Copy link
Copy Markdown
Collaborator

Closing this PR in favour of #435
Thank you for your work and suggestion. I will make sure to keep your commit.

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.

2 participants