Opened 8 years ago

Closed 7 years ago

#6174 closed defect (fixed)

De-kludgify marking circs as unsuitable for new streams

Reported by: rransom Owned by:
Priority: Medium Milestone: Tor: 0.2.4.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client
Cc: Actual Points:
Parent ID: #7755 Points:
Reviewer: Sponsor:

Description

      /* XXXX024 This is a screwed-up way to say "This is too dirty
       * for new circuits. */
      circ->timestamp_dirty -= options->MaxCircuitDirtiness;
    /* XXXX024 this is a kludgy way to do this. */
    tor_assert(circ->timestamp_dirty);
    circ->timestamp_dirty -= options->MaxCircuitDirtiness;
    /* XXXX024 this is a kludgy way to do this. */
    tor_assert(circ->_base.timestamp_dirty);
    circ->_base.timestamp_dirty -= get_options()->MaxCircuitDirtiness;

Yes, it is, and wrong in several different ways too.

Child Tickets

Change History (9)

comment:1 Changed 8 years ago by rransom

The first step is to grep for timestamp_dirty and replace every occurrence of that pattern with a call to one function.

comment:2 Changed 8 years ago by nickm

Yes, it is, and wrong in several different ways too.

Unless you're planning to do this yourself, you need to say what you think are the several things that must be fixed for this bug to be closed.

comment:3 in reply to:  2 Changed 8 years ago by rransom

Replying to nickm:

Yes, it is, and wrong in several different ways too.

Unless you're planning to do this yourself, you need to say what you think are the several things that must be fixed for this bug to be closed.

The only things that need to be done for this bug to be closed are (a) extract that hack into a separate function, and (b) add a separate do_not_attach_new_streams circuit_t field (preferably a one-bit bitfield; there should be room for more of those) to mark a circuit as not suitable for new streams.

The current hack has at least the following problems:

  • If a user increases the value of MaxCircuitDirtiness before the circuit is closed, new streams could get attached to it.
  • This hack could underflow timestamp_dirty (known bug, but not obviously fixed in every occurrence).
  • Until newnym_epoch was added with the stream-isolation code, if a user clicked ‘New Identity’, then the system clock jumped backward 10 minutes, new streams could be put on old circuits.

There is no need to find or consider every problem with circ->timestamp_dirty -= options->MaxCircuitDirtiness in order to avoid all of those problems.

comment:4 Changed 8 years ago by nickm

Keywords: tor-client added

comment:5 Changed 8 years ago by nickm

Component: Tor ClientTor

comment:6 Changed 8 years ago by rransom

Parent ID: #7755

The do_not_attach_new_streams flag should be put in origin_circuit_t, not circuit_t.

comment:7 Changed 7 years ago by nickm

Status: newneeds_review

There's a patch in branch "bug6174" in my public repo. Could use some review. It was more straightforward than I expected, but I probably missed something.

comment:8 Changed 7 years ago by andrea

This looks mergeable to me.

comment:9 Changed 7 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Okay, merged!

Note: See TracTickets for help on using tickets.