Opened 3 months ago

Last modified 4 weeks ago

#24903 merge_ready defect

Bug: Line unexpectedly reached at pathbias_should_count at src/or/circpathbias.c:372

Reported by: starlight Owned by: nickm
Priority: High Milestone: Tor: 0.3.2.x-final
Component: Core Tor/Tor Version: Tor: 0.3.2.9
Severity: Normal Keywords: tor-assert-nonfatal, regression?, 033-must, 033-triage-20180320, 033-included-20180320, 032-backport 031-backport 029-backport
Cc: mikeperry Actual Points:
Parent ID: #24907 Points: 1
Reviewer: dgoulet Sponsor:

Description

Experienced bug-check while testing an explict exit with:

setconf __DisablePredictedCircuits=1
setconf ExcludeExitNodes={c1},{c2},{c3}
setconf SocksPort="x.x.x.x:x NoIsolateSOCKSAuth NoIsolateClientProtocol NoIsolateClientAddr NoIsolateDestAddr NoIsolateDestPort"
setconf ExitNodes=X
Bug: Line unexpectedly reached at pathbias_should_count at src/or/circpathbias.c:372.
Stack trace: (on Tor 0.3.2.9 9e8b762fcecfece6)

log_backtrace                 src/common/backtrace.c:108
tor_bug_occurred_             src/common/util_bug.c:118
pathbias_should_count        *src/or/circpathbias.c:372
pathbias_count_build_attempt  src/or/circpathbias.c:418
circuit_finish_handshake      src/or/circuitbuild.c:1479
                              src/or/command.c:424
command_process_cell          src/or/command.c:209
channel_tls_handle_cell       src/or/channeltls.c:1152
                              src/or/connection_or.c:2095
                              src/or/connection.c:3468
                              src/or/main.c:738
event_base_loop               event.c:1373
do_main_loop                  src/or/main.c:2637
tor_main                      src/or/main.c:3780
main                          src/or/tor_main.c:35

Child Tickets

Change History (14)

comment:1 Changed 3 months ago by teor

Keywords: tor-assert-nonfatal regression? added
Milestone: Tor: 0.3.3.x-final
Points: 1
Priority: MediumHigh

comment:2 Changed 3 months ago by starlight

Resolution: not a bug
Status: newclosed

Looked at the code and this is a sanity-check complaining it saw a 1-hop circuit and ignored it. As part of the testing I was doing I manually extended a 1-hop circuit, which appears to have triggered this message.

I take it that 1-hop circuits are now so vigorously prevented that one cannot even use them for testing. The configuration for permitting them has been eliminated. This is annoying; single-hop circuit are handy for testing and only an expert who knows what they are doing can create them. Should be allowed.

comment:3 Changed 3 months ago by teor

Parent ID: #24907
Resolution: not a bug
Status: closedreopened

This is a bug, because we shouldn't log that message when the controller does something permitted by the spec.

We broke RefuseUnknownExits on exits in #24907, when we deprecated the AllowSingleHopExits client option. And yes, we did lock that down because we don't recommend that anyone uses it in production.

For testing, please open another ticket to have it restored if you really need it.

comment:4 Changed 4 weeks ago by nickm

Keywords: 033-must added

comment:5 Changed 4 weeks ago by nickm

Keywords: 033-triage-20180320 added

Marking all tickets reached by current round of 033 triage.

comment:6 Changed 4 weeks ago by nickm

Keywords: 033-included-20180320 added

Mark 033-must tickets as triaged-in for 0.3.3

comment:7 Changed 4 weeks ago by nickm

Cc: mikeperry added

Hm. So, the issue here has nothing to do with AllowOneHopExits -- it happens when the 'onehop_tunnel' and 'desired_path_len' fields don't match each other any more. Ordinarily, this is always true, UNLESS the controller has been messing with things.

One fix here would be to set a flag on a circuit when the controller has extended it, and ignore all circuits with this flag in pathbias_should_count().

Another fix here would be to clear onehop_tunnel when a controller has extended a circuit that previously had it. I don't know the implication there.

comment:8 Changed 4 weeks ago by nickm

Owner: set to nickm
Status: reopenedaccepted

comment:9 Changed 4 weeks ago by nickm

Mike suggests that clearing the onehop_tunnel flag might be smart, since other places in the codebase look at it too. I should check.

comment:10 Changed 4 weeks ago by nickm

Status: acceptedneeds_review

Likely fix in my branch bug24903_029.

comment:11 in reply to:  10 Changed 4 weeks ago by dgoulet

Reviewer: dgoulet
Status: needs_reviewneeds_information

Replying to nickm:

Likely fix in my branch bug24903_029.

Wait, how can onehop_tunnel be equal to 1 there? I don't see a code path that allows the EXTENDCIRCUIT command to set that value to 1. I can't see a way to specify CIRCLAUNCH_ONEHOP_TUNNEL as an extend flag?

comment:12 Changed 4 weeks ago by nickm

Status: needs_informationneeds_review

The onehop_tunnel flag can already be set if the controller takes a circuit that Tor previously created as a one-hop circuit, and then extends it.

comment:13 Changed 4 weeks ago by dgoulet

Status: needs_reviewmerge_ready

lgtm;

comment:14 Changed 4 weeks ago by nickm

Keywords: 032-backport 031-backport 029-backport added
Milestone: Tor: 0.3.3.x-finalTor: 0.3.2.x-final

Merged to 0.3.3 and forward; marking for possible backport.

Note: See TracTickets for help on using tickets.