I agree with Anders, the risk of interference with snprintf and backtrace should be low, the other functions used in the signal handler are async signal safe. I guess it is ok to push this after updating with the review comments. /Thanks HansN
From: Anders Widell Sent: den 9 april 2015 12:39 To: ramesh betham Cc: Hans Nordebäck; [email protected]; [email protected] Subject: Re: [PATCH 1 of 1] base: base: Dump stack trace to file on fatal signals, V3 #1281 The use of the backtrace functions in the signal handler is unavoidable (I think) for this feature to work. Although they are not listed as safe, I think the risk of interference should be very low. And in any case, this code will only execute when the service has already crashed, so the worst thing that can happen is that the process hangs instead of generating a core dump. The use of snprintf could be avoided by writing our own code that does the same job (print the signal number as decimal (or hexadecimal, which could be slightly easier), but I am not sure if it is worth the effort. snprintf is likely to be implemented as reentrant in glibc. / Anders Widell On 04/09/2015 08:41 AM, ramesh betham wrote: Please don't use those functions (like syslog etc.) that are not Async-signal-safe functions in signal handler function. Thanks, Ramesh. On 4/8/2015 6:03 PM, Anders Widell wrote: Ack with comments. * Make sure to have a properly formatted commit message ("base:" was duplicated, square brackets missing around ticket number). * It could be useful to see the identity of one who sent the signal (PID, maybe also UID). * See below for inline comments. / Anders Widell On 04/01/2015 10:32 AM, Hans Nordeback wrote: osaf/libs/core/common/daemon.c | 117 +++++++++++++++++++++++++++++++++++++++++ 1 files changed, 117 insertions(+), 0 deletions(-) Use dlsym on bactrace symbols to circumvent lsb compliance problems. diff --git a/osaf/libs/core/common/daemon.c b/osaf/libs/core/common/daemon.c --- a/osaf/libs/core/common/daemon.c +++ b/osaf/libs/core/common/daemon.c @@ -34,6 +34,8 @@ #include <sys/stat.h> #include <sys/file.h> +#include <dlfcn.h> + #include <configmake.h> #include "daemon.h" @@ -43,6 +45,8 @@ #include <os_defs.h> #include <osaf_secutil.h> +#include <sys/types.h> +#include <time.h> #define DEFAULT_RUNAS_USERNAME "opensaf" @@ -57,6 +61,8 @@ static unsigned int __tracemask; static unsigned int __nofork = 0; static int __logmask; +static void install_fatal_signal_handlers(); C functions taking no parameters should be declared as (void) instead of (). + static void __print_usage(const char* progname, FILE* stream, int exit_code) { fprintf(stream, "Usage: %s [OPTIONS]...\n", progname); @@ -350,6 +356,9 @@ void daemonize(int argc, char *argv[]) } #endif + /* install fatal signal handlers for writing backtrace to file */ + install_fatal_signal_handlers(); + Whitespace problems on the three lines above (space instead of tab, whitespace at end of line). /* Initialize the log/trace interface */ if (logtrace_init_daemon(basename(argv[0]), __tracefile, __tracemask, __logmask) != 0) exit(EXIT_FAILURE); @@ -414,3 +423,111 @@ void daemon_sigterm_install(int *term_fd *term_fd = term_sel_obj.rmv_obj; } + +static char bt_filename[80]; This variable shall hold the result of sprintf(..., PKGLOGDIR "/bt_%s_%d", ...). PKLOGDIR is a path that can be configured by the user. Therefore, this buffer ought to be either PATH_MAX bytes large, or dynamically allocated. + +static int (*plibc_backtrace) (void **buffer, int size) = NULL; +static int (*plibc_backtrace_symbols_fd) (void *const *buffer, int size, int fd) = NULL; + +static int init_backtrace_fptrs() C functions taking no parameters should be declared as (void) instead of (). +{ + plibc_backtrace = dlsym(RTLD_NEXT, "backtrace"); Should use RTLD_DEFAULT instead of RTLD_NEXT. + if (plibc_backtrace == NULL) { + syslog(LOG_ERR, "unable to find \"backrace\" symbol"); + return -1; + } + plibc_backtrace_symbols_fd = dlsym(RTLD_NEXT, "backtrace_symbols_fd"); Should use RTLD_DEFAULT instead of RTLD_NEXT. + if (plibc_backtrace_symbols_fd == NULL) { + syslog(LOG_ERR, "unable to find \"backrace_symbols_fd\" symbol"); + return -1; + } + return 0; +} + +/** + * Signal handler for fatal errors. Writes a backtrace and "re-throws" the signal. + */ +static void fatal_signal_handler(int sig, siginfo_t* siginfo, void* ctx) +{ + const int BT_ARRAY_SIZE = 20; + void *bt_array[BT_ARRAY_SIZE]; + size_t bt_size; + int fd; + char bt_header[30]; + + if ((fd = open(bt_filename, O_RDWR|O_CREAT, 0644)) < 0) { + goto done; + } + + snprintf(bt_header, sizeof(bt_header), "signal: %d\n", sig); + + if (write(fd, bt_header, strlen(bt_header)) < 0) { + close(fd); + goto done; + } + + bt_size = plibc_backtrace(bt_array, BT_ARRAY_SIZE); + plibc_backtrace_symbols_fd(bt_array, bt_size, fd); + + close(fd); +done: + // re-throw the signal + raise(sig); +} + +/** + * Install signal handlers for fatal errors to be able to print a backtrace. + */ +static void install_fatal_signal_handlers() C functions taking no parameters should be declared as (void) instead of (). +{ + + time_t current_time; + char time_string[20]; + + struct sigaction action; + int i = 0; + const int HANDLED_SIGNALS_MAX = 7; + static const int handled_signals[] = { + SIGHUP, + SIGILL, + SIGABRT, + SIGFPE, + SIGSEGV, + SIGPIPE, + SIGBUS + }; + + // to circumvent lsb use dlsym to retrieve backtrace in runtime + if (init_backtrace_fptrs() < 0) { + syslog(LOG_WARNING, "backtrace symbols not found, no fatal signal handlers will be installed"); + } + else { No new line before the "else" keyword. + + // prepare a filename for backtrace + time_string[0] = '\0'; + + if (time(¤t_time) < 0) { + syslog(LOG_WARNING, "time failed: %s", strerror(errno)); + } + else { No new line before the "else" keyword. + if (strftime(time_string, sizeof(time_string), "%Y%m%d_%T", localtime(¤t_time)) == 0) { + syslog(LOG_WARNING, "strftime failed"); + } + } + + snprintf(bt_filename, sizeof(bt_filename), PKGLOGDIR "/bt_%s_%d", time_string, getpid()); + + memset(&action, 0, sizeof(action)); + action.sa_sigaction = fatal_signal_handler; + sigfillset(&action.sa_mask); + action.sa_flags = SA_RESETHAND | SA_SIGINFO; + + for (i = 0; i < HANDLED_SIGNALS_MAX; ++i) + { No new line before opening brace. + if (sigaction(handled_signals[i], &action, NULL) < 0) + { No new line before opening brace. + syslog(LOG_WARNING, "sigaction %d failed: %s", handled_signals[i], strerror(errno)); + } + } + } +} ------------------------------------------------------------------------------ BPM Camp - Free Virtual Workshop May 6th at 10am PDT/1PM EDT Develop your own process in accordance with the BPMN 2 standard Learn Process modeling best practices with Bonita BPM through live exercises http://www.bonitasoft.com/be-part-of-it/events/bpm-camp-virtual- event?utm_ source=Sourceforge_BPM_Camp_5_6_15&utm_medium=email&utm_campaign=VA_SF _______________________________________________ Opensaf-devel mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/opensaf-devel
