Opened 13 months ago

Closed 13 months ago

Last modified 13 months ago

#18613 closed enhancement (implemented)

Tor could use a set of BUG_ON, BUG, etc macros to help us use tor_assert less.

Reported by: nickm Owned by: nickm
Priority: High Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: TorCoreTeam201604, tor-dos
Cc: Actual Points: small
Parent ID: Points: small
Reviewer: dgoulet Sponsor: SponsorU-can

Description

Every time we use tor_assert, it's kind of a DoS bug waiting to happen. We should have macros for recoverable handling of recoverable assertion-like conditions.

Child Tickets

Change History (9)

comment:1 Changed 13 months ago by nickm

  • Owner set to nickm
  • Status changed from new to accepted

comment:2 Changed 13 months ago by nickm

  • Type changed from defect to enhancement

comment:3 Changed 13 months ago by nickm

  • Priority changed from Medium to High

comment:4 Changed 13 months ago by nickm

  • Keywords TorCoreTeam201604 added

For March, I aim to have alpha code here. For april I aim to merge.

comment:5 Changed 13 months ago by nickm

The name "tor_assert_nonfatal" seems to be winning the straw-poll I took. I'm defining it with the variants:

  • tor_assert_nonfatal()
  • tor_assert_nonfatal_once()
  • tor_assert_nonfatal_unreached()
  • tor_assert_nonfatal_unreached_once()

I'm also changing the default definition for tor_fragile_assert() to tor_assert_nonfatal_unreached_once().

comment:6 Changed 13 months ago by nickm

  • Status changed from accepted to needs_review

Please review branch assert_nonfatal in my public repository.

comment:7 Changed 13 months ago by dgoulet

  • Reviewer set to dgoulet
  • Status changed from needs_review to needs_revision

First thing is that would be nice to identify the #else/#endif statement that are long in util_bug.h because it's already a huge amount of defines for which someone can get lost quickly.. Maybe something like:

#ifdef ALL_BUGS_ARE_FATAL
...
#else /* ALL_BUGS_ARE_FATAL */
...
#endif /* ALL_BUGS_ARE_FATAL */

Second, I think you can remove the fragile assert() here because BUG() basically does what the fragile assert do except doing it once:

+  if (BUG(conn->linked_conn)) {
     tor_fragile_assert();

Rest lgtm!

comment:8 Changed 13 months ago by nickm

  • Actual Points set to small
  • Resolution set to implemented
  • Status changed from needs_revision to closed

fixed, squashed,merged. Thanks!

comment:9 Changed 13 months ago by nickm

  • Keywords tor-dos added
Note: See TracTickets for help on using tickets.