Opened 3 years ago

Closed 9 months ago

#19566 closed enhancement (fixed)

SR: Use BUG() instead of tor_assert() when we can

Reported by: dgoulet Owned by: dgoulet
Priority: Medium Milestone: Tor: 0.3.5.x-final
Component: Core Tor/Tor Version:
Severity: Minor Keywords: tor-sr, dirauth, easy, disaster-waiting-to-happen, 034-triage-20180328, 034-removed-20180328 035-backport
Cc: asn, neel@… Actual Points:
Parent ID: Points: 0.2
Reviewer: dgoulet Sponsor:

Description

Example:

tor_assert(sr_state_get_phase() == SR_PHASE_REVEAL);

Should be replaced by:

if (BUG(sr_state_get_phase() != SR_PHASE_REVEAL))
  return;

The idea is to not kill the dirauth if this happens but still scream very loudly. Few other places in the SR subsystem can be found.

Child Tickets

Attachments (2)

bug_shared_random_patch_r1.patch (2.5 KB) - added by neel 2 years ago.
Patch to replace tor_assert() with BUG() in SR code whenever possible
bug_shared_random_patch_r2.patch (2.5 KB) - added by neel 2 years ago.
Patch to replace tor_assert() with BUG() in SR code whenever possible (Revision 2)

Download all attachments as: .zip

Change History (22)

comment:1 Changed 3 years ago by teor

Milestone: Tor: 0.2.???Tor: 0.3.???

Milestone renamed

comment:2 Changed 3 years ago by nickm

Keywords: tor-03-unspecified-201612 added
Milestone: Tor: 0.3.???Tor: unspecified

Finally admitting that 0.3.??? was a euphemism for Tor: unspecified all along.

comment:3 Changed 2 years ago by nickm

Keywords: tor-03-unspecified-201612 removed

Remove an old triaging keyword.

comment:4 Changed 2 years ago by nickm

Cc: dgoulet asn added
Keywords: disaster-waiting-to-happen added
Milestone: Tor: unspecifiedTor: 0.3.2.x-final
Priority: LowHigh

Changed 2 years ago by neel

Patch to replace tor_assert() with BUG() in SR code whenever possible

comment:5 Changed 2 years ago by neel

I have a patch to fix this as the file bug_shared_random_patch_r1.patch​.

comment:6 Changed 2 years ago by dgoulet

Cc: dgoulet removed
Reviewer: dgoulet
Status: newneeds_review

comment:7 Changed 2 years ago by nickm

Milestone: Tor: 0.3.2.x-finalTor: 0.3.3.x-final
Status: needs_reviewneeds_revision

Does this patch pass the unit tests? It looks like the BUG() conditions are mostly inverted. (That is, when the old code would say "assert(x)", this code says "if (BUG(x))" when "if (BUG(!x))" would make more sense.)

Also, is this patch just a matter of "replace all the assertions", or does it try to replace only the assertions that are fragile for one reason or another?

comment:8 Changed 2 years ago by neel

Cc: neel@… added

Changed 2 years ago by neel

Patch to replace tor_assert() with BUG() in SR code whenever possible (Revision 2)

comment:9 Changed 2 years ago by neel

I have made an updated patch under the file bug_shared_random_patch_r2.patch​.

The changes I have made this file is to use the correct use of the BUG() function.

I switched tor_assert() to BUG() in shared_random.c and shared_random_state.c in functions that seem related to SR and who's return values are void.

comment:10 Changed 20 months ago by dgoulet

Status: needs_revisionneeds_review

comment:11 Changed 20 months ago by dgoulet

Owner: set to dgoulet
Status: needs_reviewassigned

comment:12 Changed 20 months ago by dgoulet

Priority: HighMedium

comment:13 Changed 19 months ago by dgoulet

Milestone: Tor: 0.3.3.x-finalTor: 0.3.4.x-final

comment:14 Changed 17 months ago by nickm

Keywords: 034-triage-20180328 added

comment:15 Changed 17 months ago by nickm

Keywords: 034-removed-20180328 added

Per our triage process, these tickets are pending removal from 0.3.4.

comment:16 Changed 17 months ago by nickm

Milestone: Tor: 0.3.4.x-finalTor: unspecified

These tickets, tagged with 034-removed-*, are no longer in-scope for 0.3.4. We can reconsider any of them, if time permits.

comment:17 Changed 9 months ago by dgoulet

Status: assignedneeds_review

Woa... this ticket has a patch and somehow was put in "Assigned" state...

comment:18 Changed 9 months ago by nickm

Keywords: 035-backport added
Milestone: Tor: unspecifiedTor: 0.4.0.x-final

comment:19 Changed 9 months ago by dgoulet

Status: needs_reviewmerge_ready

Made a branch out of the patch. Added changes file. This lgtm; CI should pass in a jiffy.

Branch: ticket19566_035_01.
PR: https://github.com/torproject/tor/pull/554

comment:20 Changed 9 months ago by nickm

Milestone: Tor: 0.4.0.x-finalTor: 0.3.5.x-final
Resolution: fixed
Status: merge_readyclosed

looks okay to me too; merged to 0.3.5 and forward.

Note: See TracTickets for help on using tickets.