Opened 18 months ago

Closed 18 months ago

Last modified 18 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 18 months ago by nickm

Owner: set to nickm
Status: newaccepted

comment:2 Changed 18 months ago by nickm

Type: defectenhancement

comment:3 Changed 18 months ago by nickm

Priority: MediumHigh

comment:4 Changed 18 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 18 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 18 months ago by nickm

Status: acceptedneeds_review

Please review branch assert_nonfatal in my public repository.

comment:7 Changed 18 months ago by dgoulet

Reviewer: dgoulet
Status: needs_reviewneeds_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 18 months ago by nickm

Actual Points: small
Resolution: implemented
Status: needs_revisionclosed

fixed, squashed,merged. Thanks!

comment:9 Changed 18 months ago by nickm

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