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

["Tor Bug Tracker & Wiki" <blackhole@xxxxxxxxxxxxxx>]

#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 nickm):

 Replying to [comment:8 andrea]:
 > Athena's code review follows:
 >
 > * 5343ee1a06ebb959fc77753898015186b94a5daa
 >   - Looks okay to me, but (nitpick) I don't think '0' + digit is quite
 perfectly portable.

 Indeed it isn't, but it's one of the things Tor requires. (Specifically,
 we require that your character set contain ASCII as a subset.)  We check
 for this in compat.h:
 {{{
 #if 'a'!=97 || 'z'!=122 || 'A'!=65 || ' '!=32
 #error "It seems that you encode characters in something other than
 ASCII."
 #endif
 }}}

 > * 82743d6e560bbbbcbd22e09c3ec26f54d0656b75
 >   - Does TOR_CHECK_LDFLAGS actually add it to LDFLAGS for us?  what if
 it fails - shouldn't we set USE_BACKTRACE then?

 Yes, it does add it to LDFLAGS. If the check fails, then presumably the
 platform is one where the -rdynamic flag doesn't exist, and therefore
 isn't necessary.

 We might need more sophisticated checks, but the ones I had were
 sufficient to make it work everywhere I tried.

 > * ab2a00ac4378e85f67da6e681a7c8609e3153db1
 >   - Looks okay to me, but yeesh signal handlers can get hairy.  I
 presume you've tested all this by actually invoking it from a signal
 handler?

 Yes, though that's not sufficient.  The failure mode when invoking non-
 signal-safe stuff from signal handlers is not "the code fails" but rather
 "if the signal arrives while exactly the wrong thing is happening, the
 signal handler code fails, or the other code fails once the signal handler
 is done"  As such, the only real safe way to do this is to enumerate all
 the system and/or library functions you're calling and make sure they're
 all on the POSIX safe list.

 The ones we call from tor_log_err_sigsafe() are: time(), write(),
 va_start(), va_ag(), va_end(), strlen(), tor_assert()... whoops.
 tor_assert() shouldn't be on that list.

 Fixing in a fixup commit.

 > * c8ca168d0cfe58b8f86badbc17300f85adfedb31
 >   - Is not our code, and I sure hope Google didn't break it too badly.
 OpenBSD support seems to be missing and probably shouldn't be - and
 wouldn't be too hard to find out and add.  Linux/sparc{,64} is rather more
 obscure but I happen to have it at hand, so we could add that too.

 Indeed. I'm assuming that this will be easier for us to fix up and submit
 upstream fixes for than it would be for us to build one of these from
 scratch.  (For a while, I was trying the latter, and getting quite
 frustrated at the amount of research needed.)

 They mention OpenBSD support in the comments and later in the file;
 supposedly it works?

 > * 1d6af8099d9f3dafefdf75bb1411ec2bd669087f
 >   - Yay, unit tests!  Shouldn't tor_log_err_sigsafe() actually get
 invoked from a signal handler at some point though?

 It does in the "./test/test-bt-cl crash" test, I believe?

 > * e9f95a67ae4b35326293865034fe92e1df59ae43
 >   - Is an empty diff?

 Yes; it's only to add another missing bit of commit log message to the
 commit it's squashing on to.

 > * f0b9dd03abfd18a037f4ae0bbacecd34b35cad1a
 >   - Ouch! Am I reading that wrong, or did you just #error on any Linux
 that isn't one of the archs pc_from_ucontext.m4 supports? :(

 I think I #errored in the case where PC_FROM_UCONTEXT is defined, but the
 platfrom *isn't* one of linux, darwin, apple, openbsd, freebsd. I think
 instead going for '1' as a sane default makes more sense.  Doing that.

 > * 511a8af458bbf56edaf7d1110dfe28c11274b7fb
 >   - Looks okay, but wouldn't it be better to just compile stuff like
 this with -O0?

 Maybe?  It's nice to have confirmation that running with -O2 ''doesn't''
 prevent the backtrace code from working.


 Based on the above, I'm adding a few fixup commits, squashing as
 'backtrace_squashed', and merging.  Thanks!

-- 
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/9299#comment:9>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs