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

drmgr: unsafe behavior in signal handler #56

Open
nathanlynch opened this issue Mar 9, 2021 · 1 comment
Open

drmgr: unsafe behavior in signal handler #56

nathanlynch opened this issue Mar 9, 2021 · 1 comment
Labels

Comments

@nathanlynch
Copy link
Member

nathanlynch commented Mar 9, 2021

Consider drmgr's signal handler:

void
sighandler(int signo)
{
	say(ERROR, "Received signal %d, attempting to cleanup and exit\n",
	    signo);

	if (log_fd) {
		void *callstack[128];
		int sz;

		sz = backtrace(callstack, 128);
		backtrace_symbols_fd(callstack, sz, log_fd);
	}

	dr_fini();
	exit(-1);
}

There are several actions that are not safe[1] and could result in process hangs/crashes:

  1. say(), which uses a couple of functions from the printf family: AS-Unsafe[2]
  2. backtrace() and backtrace_symbols_fd(): AS-Unsafe[3]
  3. dr_fini() which performs too many AS-Unsafe things to list individually

I think that we would be OK to simply remove sighandler and sig_setup. Perhaps in the past there was legitimate reason for drmgr to handle normally fatal signals such as SIGSEGV and SIGBUS, but I don't think that's true any more. The only reason for caution I see is that this handler releases the dr_lock, but since that's an advisory record lock it will be released on process termination anyway.

A benefit of cleaning this up will be that drmgr will produce proper core dumps reliably when it crashes.

[1] https://www.gnu.org/software/libc/manual/html_node/POSIX-Safety-Concepts.html and https://man7.org/linux/man-pages/man7/signal-safety.7.html
[2] https://www.gnu.org/software/libc/manual/html_node/Formatted-Output-Functions.html
[3] https://www.gnu.org/software/libc/manual/html_node/Backtraces.html

@tyreld tyreld added the cleanup label Jan 27, 2022
@nathanlynch
Copy link
Member Author

proposed change from Scott: https://groups.google.com/g/powerpc-utils-devel/c/yNb4q339fM8

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants