• meiravgri's avatar
    Signal handler attributes (#12426) · fe47c202
    meiravgri authored
    This PR purpose is to make the crash report process thread safe.
    main changes include:
    
    1. `setupSigSegvHandler()` is introduced to initialize the signal handler.
    This function first initializes the signal handler mutex (if not initialized yet)
    and then registers the process to the signal handler. 
    
    2. **sigsegvHandler** flags :
    SA_NODEFER - don't add the signal to the process signal mask. We use this
    flag because we want to be able to handle a second call to the signal manually.
    removed SA_RESETHAND: this flag resets the signal handler function upon the first
    entrance to the registered function. The reason to use this flag is to protect from
    recursively entering the signal handler by the same thread. But, it also means
    that if a second thread crashes while handling a signal, the process will be
    terminated immediately and we won't get the crash report.
    In this PR we discard this flag. The signal handler guard described below purpose
    is to solve the above issues.
    
    3. Add a **signal handler lock** with ERRORCHECK attributes. 
    The lock's purpose is to ensure that only one thread generates a crash report.
    Once a second thread enters the signal handler it will be blocked.
    We use the ERRORCHECK lock in order to protect from possible deadlock in
    case the thread handling the crash gets a signal. In the latest scenario, we log
    what we have collected until the handler crashed.
    
    At the end of the crash report we reset the signal handler SIG_DFL, with no flags, and
    rethrow the signal to generate a core dump (if enabled) and exit the process.
    
    During the work on this PR we wanted to understand the historical reasons for
    how crash is handled.
    With respect to the choice of the flag, we believe the **SA_RESETHAND** was not
    added for any specific purpose.
    **SA_ONSTACK** which is removed here from bugReportEnd(), was originally also
    set in the initial registration to signal handler, but removed in 3ada43e7. In addition,
    it was removed from another location in deee2c1e with the following description,
    which is also relevant to why it should be removed from bugReportEnd:
    
    > it seems to be some valgrind bug with SA_ONSTACK.
    > SA_ONSTACK seems unneeded since WD is not recursive (SA_NODEFER was removed),
    > also, not sure if it's even valid without a call to sigaltstack()
    fe47c202
debug.c 91.3 KB