Opened 5 years ago

Closed 5 years ago

#15211 closed defect (fixed)

Audit all asserts to ensure they don't have side effects

Reported by: cypherpunks Owned by:
Priority: Low Milestone: Tor: 0.2.7.x-final
Component: Core Tor/Tor Version:
Severity: Keywords:
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

#15188 fixed one instance, we should make sure there aren't more. Assignments are clearly bad, function calls are potentially bad. Anything else I'm looking for?

Child Tickets

Change History (11)

comment:1 Changed 5 years ago by cypherpunks

A quick grep for tor_assert(.*( and manual parsing turned up with

src/common/compat_pthreads.c:281: tor_assert(0==pthread_attr_init(&attr_detached));
src/common/compat_pthreads.c:282: tor_assert(0==pthread_attr_setdetachstate(&attr_detached, 1));
src/or/rendservice.c:3323: tor_assert(!crypto_pk_generate_key(intro->intro_key));

comment:2 Changed 5 years ago by cypherpunks

yeah, tho we really want to look until the next semicolon. Also we should split any instances where we use a &&.

comment:3 Changed 5 years ago by nickm

Priority: normalminor

This is a cool thing to do, but let's remember it isn't critical: The tor source doesn't let you define NDEBUG, so we don't need to worry about assertions getting eliminated. This is a style issue, not a safety issue.

Still, it's worth fixing.

comment:4 Changed 5 years ago by Sebastian

Status: newneeds_review

I agree it's not critical, but please consider it regardless :) branch bug15211 in my repo

comment:5 Changed 5 years ago by nickm

Hmn. Is this actually making the code better? Second opinions welcome. I wonder if instead we shouldn't define a replacement for tor_assert that explicitly allows side-effects.

comment:6 Changed 5 years ago by dgoulet

My two cents. This makes the code better for the following reasons thus I ack Sebastian patch.

1) Readability which is important as more and more contributors start looking/using the code. I myself doesn't expect function calls in an assert() because they can't be removed (not in the case of tor though). It's just not something you expect in C code imo.

2) Maintainability over time, keep the code consistent which often is the starting point for new contributors to look at the code to understand the coding style before submitting a patch (yes not always the case, I know).

I absolutely agree, this is not critical but the change is trivial and useful imo. Probably the change file could be "Coding style" instead of "bugfix" because no bug is fixed here.

comment:7 Changed 5 years ago by Sebastian

I implemented this mostly because I need it for the feature where we're not using assertions for coverage builds

comment:8 Changed 5 years ago by nickm

Ah, that's reasonable.

comment:9 Changed 5 years ago by nickm

Status: needs_reviewnew

Merged, but turned some of the ints into const ints to make the code (IMO) even clearer. Leaving open since there are probably more of these?

comment:10 Changed 5 years ago by Sebastian

No I think I got all of them. I looked at:

  • every multi-line assert individually
  • every assert where we have an opening brace before ;
  • every assert where we have assignment, including +=, *=, ++, etc (we have zero of these)

Thanks for merging and the const fix. Clearly I should work on my const-correctness

comment:11 Changed 5 years ago by nickm

Resolution: fixed
Status: newclosed

ok, neat! I'll close and we can reopen if we find any more.

Note: See TracTickets for help on using tickets.