[tor-bugs] #9299 [Tor]: Dump stack traces on assertion, crash, or general trouble

[Tor Bug Tracker & Wiki]

#9299: Dump stack traces on assertion, crash, or general trouble
-----------------------------+---------------------------------
     Reporter:  nickm        |      Owner:
         Type:  enhancement  |     Status:  needs_review
     Priority:  normal       |  Milestone:  Tor: 0.2.5.x-final
    Component:  Tor          |    Version:
   Resolution:               |   Keywords:  tor-relay debugging
Actual Points:               |  Parent ID:
       Points:               |
-----------------------------+---------------------------------

Comment (by lunar):

 Ok, here's a review. I hope I'm not wasting anyone's time.

 Interesting how the new `format_dec_number_sigsafe()` and
 `format_hex_number_sigsafe()` are slightly different given they do almost
 the same thing.

 {{{
 +++ b/configure.ac
          crt_externs.h \
 +       execinfo.h \
          grp.h \
 }}}

 Small whitespace issue.

 {{{
 +    tor_assert(0 * 0 == 1);
 }}}

 Leaving that in the history is probably bad for future bisection, no?

 `tor_log_err_sigsafe_helper()` might be better named
 `tor_log_err_sigsafe_write()`

 {{{
 +  if (log_time_granularity >= 2000) {
 +    int g = log_time_granularity / 1000;
 }}}

 A comment for the magic numbers? You previously had 15 minutes. Does this
 mean that the
 `stack_dump` file will have second granularity by default? Is that a
 problem?

 {{{
 +tor_log_get_sigsafe_err_fds(const int **out)
 +{
 +  *out = sigsafe_log_fds;
 +  return n_sigsafe_log_fds;
 }}}

 Can't we have a race here if `tor_log_get_sigsafe_err_fds()` is called and
 then `tor_log_update_sigsafe_err_fds()`? Or do the usage of the first
 mandate `LOCK_LOGS`? Or are we sure there's no way this can happen?

 `fd7aa1af` commit message does not say anything about the
 `found_real_stderr` thing.

 {{{
 +    assert(n_sigsafe_log_fds >= 2);
 }}}

 Using `assert()` because `tor_assert()` will not be happy with an
 unconfigured crash handler, right? Might worth a comment.

 {{{
 +#ifdef PC_FROM_UCONTEXT
 +#if defined(__linux__)
 +  const int n = 1;
 +#elif defined(__darwin__) || defined(__APPLE__) || defined(__OpenBSD__) \
 +  || defined(__FreeBSD__)
 +  const int n = 2;
 +#endif
 }}}

 Maybe add an #else #error here ? The compiler should be unhappy because
 `n` will be uninitialized, but it might save a little bit of time for a
 future port.

 {{{
 +    puts("Argument should be \"assert\" or \"crash\"");
 }}}

 “or none”

 {{{
 +  return crash(x) + crash(x*2);
 }}}

 What's the point of testing the reader's knowledge about C evaluation
 order? :)



 Running the test program with assert has the logging function in the
 trace:

 {{{
 $ ./test-bt-cl assert
 Oct 11 22:55:39.705 [err] tor_assertion_failed_(): Bug:
 src/test/test_bt_cl.c:36: crash: Assertion 1 == 0 failed; aborting.
 Oct 11 22:55:39.705 [err] Bug: Assertion 1 == 0 failed in crash at
 src/test/test_bt_cl.c:36. Stack trace:
 Oct 11 22:55:39.705 [err] Bug:     ./test-bt-cl(log_backtrace+0x35)
 [0x7f3236a5e095]
 Oct 11 22:55:39.705 [err] Bug:     ./test-bt-
 cl(tor_assertion_failed_+0x9f) [0x7f3236a68a8f]
 Oct 11 22:55:39.705 [err] Bug:     ./test-bt-cl(crash+0x79)
 [0x7f3236a5de69]
 […]
 }}}

 Is that something we want to keep?

 For the other one:

 {{{
 $ ./test-bt-cl crash
 ============================================================ T=1381524943
 Tor  died: Caught signal 11
 ./test-bt-cl(+0x8fc7)[0x7f156871afc7]
 ./test-bt-cl(crash+0x48)[0x7f156871ae38]
 ./test-bt-cl(crash+0x48)[0x7f156871ae38]
 ./test-bt-cl(oh_what+0x25)[0x7f156871ae95]
 }}}

 There's no symbol for the first function and `crash()` is there twice. Is
 that expected?

-- 
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/9299#comment:5>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online