ibm-power-utilities / powerpc-utils

Suite of utilities for Linux on Power systems
GNU General Public License v2.0
34 stars 53 forks source link

drmgr: unsafe behavior in signal handler #56

Open nathanlynch opened 3 years ago

nathanlynch commented 3 years ago

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

nathanlynch commented 1 year ago

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