Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#5049 closed defect (fixed)

When LearnCircuitBuildTimeout is off, we should ignore (most?)( CBT parameters from the consensus

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

Description

circuit_build_times_get_initial_timeout() and some related functions look at cbtmintimeout and other consensus parameters, and get invoked even when we have LearnCircuitBuildTimeout set.

When LearnCircuitBuildTimeout is off, we should probably be more circumspect about which parameters we use. In particular, we should not apply *any* changes to a user configured CircuitBuildTimeout value in this case.

Reported by wanoskarnet

Child Tickets

Change History (12)

comment:1 Changed 8 years ago by andrea

Status: newneeds_review

See c1b11f7d4607706fd7b6ea771d953251b9036910 through b8c6f45011add21867697f669006a6740f91a4bd of https://gitweb.torproject.org/user/andrea/tor.git/log/refs/heads/trac-5049

comment:2 Changed 8 years ago by nickm

Status: needs_reviewneeds_revision

Looks good, I think! Here are some thoughts, in no particular order.

I wonder if the debug logs should be if (LearnCircuitBuildTimeout) { log_debug(LD_BUG,... ) } instead of log_debug(LD_CIRC,). LD_BUG is what we generally use for stuff that will never get reached.

Can you explain the process you used to make sure that you covered all the cases here? Did you just add the debugging statements then modify the code until they stopped appearing, or did you do a call graph analysis to make sure the functions couldn't be reached when LearnCircuitBuildTimeout is 0, or both?

It is probably okay, if I am reading the code right, to just tor_assert() that circuit_build_times_recent_circuit_count() is greater than 0: Its use of networkstatus_get_param() clips the range of its output so that it should always lie between CBT_MIN_RECENT_CIRCUITS and CBT_MAX_RECENT_CIRCUITS.

The tor_free() macro sets its argument (an lvalue) to NULL; I think that our current convention is not to include an additional assign-to-NULL afterwards.

Perhaps the code that frees and clears the circuit-build-history data should get extracted into its own function?

Tor's house style is K&R C, so we stick the else on the same line as both surrounding braces, as in

  if (foo) {
     ...
  } else if (bar) {
     ...
  } else {
     ...
  }

Now, this one is an alternative design, not a problem:

Does anything still call circuit_build_times_get_initial_timeout() when LearnCBT is off? It seems that it's used to set the initial value of close_ms and timeout_ms, which are later used in circuituse.c when we are computing circuit timeouts. Perhaps instead of having the global "circ_times" be what use in circuituse.c, we should instead have a function that returns close_ms and timeout_ms (when LearnCBT is 1) or returns options->CircuitBuildTimeout * 1000 (when LearnCBT is 0). We could call this function in place of the current invocations of close_ms and timeout_ms in circuituse.c. Do you think that would be cleaner?

comment:3 Changed 8 years ago by andrea

Changed log_debug(LD_CIRC,...) per your suggestion

comment:4 Changed 8 years ago by andrea

To find these, I looked for the things that query the cbt* consensus parameters, and then for their callers, so I suppose you'd say call graph analysis. Then I inserted checks on LearnCircuitBuildTimeout in appropriate places, and used the debug logs to verify that I'd gotten them all.

comment:5 Changed 8 years ago by andrea

Circuit-build-history free code moved per your suggestion.

comment:6 Changed 8 years ago by andrea

Changed to K&R brace style.

comment:7 Changed 8 years ago by andrea

Status: needs_revisionneeds_review

As for the circ_times vs. function to get timeouts, I like the idea of abstracting the timeout query in a function, but LearnCBT == 0 isn't the only circumstance we want to disable this code in. It's also possible to get a cbtdisabled consensus parameter. If we do that, we'd want the function to call circuit_build_times_disabled() like everything else does rather than check LearnCBT directly.

comment:8 in reply to:  7 Changed 8 years ago by nickm

Replying to andrea:

As for the circ_times vs. function to get timeouts, I like the idea of abstracting the timeout query in a function, but LearnCBT == 0 isn't the only circumstance we want to disable this code in. It's also possible to get a cbtdisabled consensus parameter. If we do that, we'd want the function to call circuit_build_times_disabled() like everything else does rather than check LearnCBT directly.

Agreed. I'll squash what you have a bit, rebase, merge, and then open a new ticket for "abstract accesses to circ_times".

comment:9 Changed 8 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Added #6153 targetting 0.2.4.x; merging and closing. Thanks!

comment:10 Changed 8 years ago by arma

For next time, it would be great if the changes file listed the bug number.

I'm going to guess this is a bugfix on 0.2.2.14-alpha, which is where the config option and consensus param were first mentioned.

comment:11 Changed 7 years ago by nickm

Keywords: tor-client added

comment:12 Changed 7 years ago by nickm

Component: Tor ClientTor
Note: See TracTickets for help on using tickets.