Opened 7 years ago

Closed 7 years ago

#9543 closed defect (fixed)

weird behaviour when MaxCircuitDirtiness is set to a high value

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

Description

I tested this with the rttprober script[0] which opens circuits and attaches multiple streams to them. Each stream connects to 127.0.0.1, fails and quits. When I set MaxCircuitDirtiness to the default value (600) and tell the script to open circuits and run 1000 stream-probes on each circuit, it completes for most of the circuits. When I set MaxCircuitDirtiness to a very large value (tested with 2147483647 and 1073741823) almost all circuits where closed with reason FINISHED before all probes where run. Using a MaxCircuitDirtiness of 10000 works as expected though.

[0] https://bitbucket.org/ra_/tor-rtt/src

Child Tickets

Change History (5)

comment:1 Changed 7 years ago by ra

Summary of what I observed: Circuits are marked as dirty *earlier* when MaxCircuitDirtiness is set to a *large* value.

This should not be the case and it might be a sign of an integer underflow.

comment:2 Changed 7 years ago by nickm

Keywords: tor-client added
Milestone: Tor: 0.2.4.x-final

Yeah, it looks like there are some overflow opportunities with huge MaxCircuitDirtiness values.

Fortunately, they're easy enough to find: every use of MaxCircuitDirtiness is in src/or/circuituse.c.

As I see it, there are two options for a fix: one not too hard, and one easier.

  • Fix the code to avoid overflow.
  • Add a maximum value to MaxCircuitDirtiness in config.c. (See how MIN_MAX_CIRCUIT_DIRTINESS is enforced there.)

comment:3 Changed 7 years ago by nickm

Status: newneeds_review

Trivial fix (30 day maximum) in branch "bug9543" in my public repository. Needs review.

comment:4 Changed 7 years ago by arma

Patch looks fine.

Just for my own clarification, the issue is that lines like

       circ->timestamp_dirty+get_options()->MaxCircuitDirtiness <= now)

will add circ->timestamp_dirty to MaxCircuitDirtiness, overflow it, get a negative number, and say "yes it's less than now"?

comment:5 Changed 7 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Yes, in practice that's what happens. In principle, though, remember that this is signed addition, and so overflow is undefined.

Merged.

Note: See TracTickets for help on using tickets.