CircuitBuildTimeout default value does not match man page

changed milestone to %Tor: 0.2.4.x-final in legacy/trac

added component::core tor/tor in Legacy / Trac documentation in Legacy / Trac milestone::Tor: 0.2.4.x-final in Legacy / Trac priority::high in Legacy / Trac resolution::fixed in Legacy / Trac status::closed in Legacy / Trac tor-client in Legacy / Trac type::defect in Legacy / Trac labels

Trac:
Milestone: N/A to Tor: 0.2.3.x-final

Now I'm not so sure that this is only a problem with the warning. I reliably fail to connect using flash proxies using this command:

$ tor ClientTransportPlugin "websocket socks4 tor-facilitator.bamsoftware.com:9999" UseBridges 1 Bridge "websocket 0.0.1.0:1" LearnCircuitBuildTimeout 0 SocksPort 9051
<...>
Jul 10 10:50:48.000 [notice] Bootstrapped 5%: Connecting to directory server.
Jul 10 10:50:48.000 [notice] Heartbeat: Tor's uptime is 0:00 hours, with 0 circuits open. I've sent 0 kB and received 0 kB.
Jul 10 10:50:48.000 [notice] Bootstrapped 10%: Finishing handshake with directory server.
Jul 10 10:50:48.000 [warn] Problem bootstrapping. Stuck at 10%: Finishing handshake with directory server. (DONE; DONE; count 1; recommendation warn)
Jul 10 10:50:48.000 [warn] 1 connections have failed:
Jul 10 10:50:48.000 [warn]  1 connections died in state handshaking (TLS) with SSL state unknown state in HANDSHAKE

But adding what should be the default timeout allows me to connect:

$ tor ClientTransportPlugin "websocket socks4 tor-facilitator.bamsoftware.com:9999" UseBridges 1 Bridge "websocket 0.0.1.0:1" LearnCircuitBuildTimeout 0 CircuitBuildTimeout 60 SocksPort 9051
<...>
Jul 10 10:52:42.000 [notice] Tor has successfully opened a circuit. Looks like client functionality is working.
Jul 10 10:52:42.000 [notice] Bootstrapped 100%: Done.

This persists even after totally deleting my ~/.tor directory (I thought a short timeout might be cached there). You should be able to run these flash proxy commands without installing any flash proxy software.

Trac:
0001-Make-the-default-CircuitBuildTimeout-match-the-man-p.patch

This warning happened to me a few days ago, while setting up Tor on a laptop. It is accurate.

Trac:
Status: new to needs_review
Milestone: Tor: 0.2.3.x-final to Tor: 0.2.2.x-final
Summary: Too-low CircuitBuildTimeout warning is misleading with default timeout to CircuitBuildTimeout default value does not match man page
Priority: minor to major

NAK. I believe that "0" is supposed to mean "use the default". See the logic in circuit_build_times_get_initial_timeout().

That said, the logic is screwy, and it looks like should change the outer "else" case of that function so that if LearnCircuitBuildTimeout and CircuitBuildTimeout, we fall back to circuit_build_times_initial_timeout.

Trac:
Status: needs_review to needs_revision

Trac:
Keywords: N/A deleted, documentation added
Milestone: Tor: 0.2.2.x-final to Tor: 0.2.3.x-final

Trac:
Keywords: documentation deleted, documentation tor-client added

Trac:
Component: Tor Client to Tor

Just closed legacy/trac#8444 (moved) as a duplicate of this.

I've got a possible fix as branch "bug6034_v2" in my public repository.

Trac:
Status: needs_revision to needs_review

Oh, I might have just tested the wrong branch (8444_v1). I got the following error when trying to run with tor2webmode. I don't see bug6034_v2 in your repository yet.

Mar 10 02:34:12.143 [notice] Tor v0.2.3.18-rc-dev (git-43d224233f8b6a88) running on Linux.
Mar 10 02:34:12.144 [notice] Tor can't help you if you use it wrong! Learn how to be safe at https://www.torproject.org/download/download#warning
Mar 10 02:34:12.144 [notice] Read configuration file /home/user/code/tor/torrc.
Mar 10 02:34:12.149 [notice] Tor2webMode is enabled; turning LearnCircuitBuildTimeout off.
Mar 10 02:34:12.149 [warn] CircuitBuildTimeout is shorter (0 seconds) than recommended (10 seconds), and LearnCircuitBuildTimeout is disabled.  If tor isn't working, raise this value or enable LearnCircuitBuildTimeout.
Mar 10 02:34:12.150 [notice] Initialized libevent version 2.0.18-stable using method epoll (with changelist). Good.
Mar 10 02:34:12.150 [notice] Opening Socks listener on 127.0.0.1:9333
Mar 10 02:34:12.000 [err] This copy of Tor was not compiled to run in 'tor2web mode'. It cannot be run with the Tor2webMode torrc option enabled. To enable Tor2webMode recompile with the --enable-tor2webmode option.
Mar 10 02:34:12.000 [err] set_options(): Bug: Acting on config options left us in a broken state. Dying.

That's the error you get when you enable Tor2WebMode without configuring with --enable-tor2webmode option.

oops. mild dyslexia strikes again... your branch (bug6304_v2) successfully builds a circuit :-)

(and I am pretty sure I built bug8444_v1 with --enable-tor2webmode, because I did it twice to make sure)

Let's try this in 0.2.4 first. Can somebody review the code?

Trac:
Keywords: documentation tor-client deleted, documentation tor-client 023-backport added
Milestone: Tor: 0.2.3.x-final to Tor: 0.2.4.x-final

Replying to nickm:

Let's try this in 0.2.4 first. Can somebody review the code?

I looked at commit d13b996d and it doesn't immediately make sense to me. I don't see why different conditions are used in different places. Simplifying a bit,

  if (get_options()->LearnCircuitBuildTimeout) {
    if (get_options()->CircuitBuildTimeout)
      timeout = get_options()->CircuitBuildTimeout*1000;
    else
      timeout = circuit_build_times_initial_timeout();
  } else {
    if (get_options()->CircuitBuildTimeout > 0)
      timeout = get_options()->CircuitBuildTimeout*1000;
    else
      timeout = circuit_build_times_initial_timeout();
  }

In the first branch of the if (LearnCircuitBuildTimeout is true), we check CircuitBuildTimeout != 0. But in the second branch (LearnCircuitBuildTimeout is false), we rather check CircuitBuildTimeout > 0. I realize there must be some difference in the two branches, or the outer if could disappear, but it's not obvious to me why the conditions differ here. Can CircuitBuildTimeout be negative (-1 means uninitialized or something)?

Actually, it's 0 that means "uninitialized" here. The >0 vs true tests could be made more uniform.

This code wouldn't actually be too hard to simplify, into something like what I have now in "bug6304_v3". I didn't do that in the initial version, since I wanted the patch to be easier to follow. I think that commit 7543fb61 changes nothing, but it's a nonobvious refactor.