Opened 7 years ago

Closed 7 years ago

#9671 closed defect (fixed)

Disabling cbt by consensus doesn't disable exploratory circuit building

Reported by: arma Owned by:
Priority: High Milestone: Tor: 0.2.3.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-client 023-backport
Cc: Actual Points:
Parent ID: #9657 Points:
Reviewer: Sponsor:

Description

At the bottom of circuit_predict_and_launch_new() we check

  /* Finally, check to see if we still need more circuits to learn
   * a good build timeout. But if we're close to our max number we
   * want, don't do another -- we want to leave a few slots open so
   * we can still build circuits preemptively as needed. */
  if (num < MAX_UNUSED_OPEN_CIRCUITS-2 &&
      get_options()->LearnCircuitBuildTimeout &&
      circuit_build_times_needs_circuits_now(&circ_times)) {
    flags = CIRCLAUNCH_NEED_CAPACITY;
    log_info(LD_CIRC,
             "Have %d clean circs need another buildtime test circ.", num);
    circuit_launch(CIRCUIT_PURPOSE_C_GENERAL, flags);
    return;
  }

Here we check get_options()->LearnCircuitBuildTimeout directly, rather than calling circuit_build_times_disabled(). That means we do these exploratory circuits even when cbt is disabled by consensus, or when we're a directory authority, or when we've failed to write cbt history to our state file lately.

We also have the same bug in circuit_expire_old_circuits_clientside() where

  if (get_options()->LearnCircuitBuildTimeout &&
      circuit_build_times_needs_circuits(&circ_times)) {
    /* Circuits should be shorter lived if we need more of them
     * for learning a good build timeout */
    cutoff.tv_sec -= IDLE_TIMEOUT_WHILE_LEARNING;
  } else {
    cutoff.tv_sec -= get_options()->CircuitIdleTimeout;
  }

Child Tickets

Change History (5)

comment:1 Changed 7 years ago by arma

Parent ID: #9657

comment:2 Changed 7 years ago by nickm

Keywords: 023-backport added
Milestone: Tor: 0.2.3.x-finalTor: 0.2.4.x-final

comment:3 Changed 7 years ago by nickm

Status: newneeds_review

Looks like a flaw in our #5049 fixes of 0c3c0b1ddd97d9b.

See "bug9671_023" for the obvious fix. I didn't see any other suspicious uses, though I am unsure about all of the LearnCircuitBuildTimeouts calls in circuitbuild.c.

comment:4 Changed 7 years ago by arma

patch looks ok to me

comment:5 Changed 7 years ago by nickm

Milestone: Tor: 0.2.4.x-finalTor: 0.2.3.x-final
Resolution: fixed
Status: needs_reviewclosed

Mike liked it too. Merged to 0.2.3 and later.

Note: See TracTickets for help on using tickets.