Opened 4 years ago

Closed 4 years ago

#18625 closed defect (fixed)

Assertion conn->chosen_exit_name failed in circuit_is_acceptable at src/or/circuituse.c:119

Reported by: arma Owned by:
Priority: High Milestone: Tor: 0.2.8.x-final
Component: Core Tor/Tor Version:
Severity: Blocker Keywords: must-fix-before-028-rc, regression, TorCoreTeam201603
Cc: dgoulet, teor, nickm, tor@… Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

moria1 running d5f50cb052a535b5 died very soon after start-up, with

Mar 24 15:33:37.030 [err] Bug: Assertion conn->chosen_exit_name failed in circuit_is_acceptable at src/or/circuituse.c:119. Stack trace: (on Tor 0.2.8.1-alpha-dev d5f50cb052a535b5)
Mar 24 15:33:37.030 [err] Bug:     ../git/src/or/tor(log_backtrace+0x42) [0x7f435950bde2] (on Tor 0.2.8.1-alpha-dev d5f50cb052a535b5)
Mar 24 15:33:37.030 [err] Bug:     ../git/src/or/tor(tor_assertion_failed_+0xa3) [0x7f435951c943] (on Tor 0.2.8.1-alpha-dev d5f50cb052a535b5)
Mar 24 15:33:37.030 [err] Bug:     ../git/src/or/tor(+0xcd493) [0x7f435949b493] (on Tor 0.2.8.1-alpha-dev d5f50cb052a535b5)
Mar 24 15:33:37.030 [err] Bug:     ../git/src/or/tor(+0xce1ea) [0x7f435949c1ea] (on Tor 0.2.8.1-alpha-dev d5f50cb052a535b5)
Mar 24 15:33:37.030 [err] Bug:     ../git/src/or/tor(connection_ap_handshake_attach_circuit+0x1ad) [0x7f435949cdfd] (on Tor 0.2.8.1-alpha-dev d5f50cb052a535b5)
Mar 24 15:33:37.030 [err] Bug:     ../git/src/or/tor(connection_ap_attach_pending+0x1a8) [0x7f43594c22b8] (on Tor 0.2.8.1-alpha-dev d5f50cb052a535b5)
Mar 24 15:33:37.030 [err] Bug:     ../git/src/or/tor(do_main_loop+0x3e5) [0x7f4359409485] (on Tor 0.2.8.1-alpha-dev d5f50cb052a535b5)
Mar 24 15:33:37.030 [err] Bug:     ../git/src/or/tor(tor_main+0xdf5) [0x7f435940a4f5] (on Tor 0.2.8.1-alpha-dev d5f50cb052a535b5)
Mar 24 15:33:37.030 [err] Bug:     ../git/src/or/tor(main+0x19) [0x7f4359406559] (on Tor 0.2.8.1-alpha-dev d5f50cb052a535b5)
Mar 24 15:33:37.030 [err] Bug:     /lib64/libc.so.6(__libc_start_main+0xfd) [0x7f4357d26d5d] (on Tor 0.2.8.1-alpha-dev d5f50cb052a535b5)
Mar 24 15:33:37.030 [err] Bug:     ../git/src/or/tor(+0x38469) [0x7f4359406469] (on Tor 0.2.8.1-alpha-dev d5f50cb052a535b5)

I currently think this is related to the same stuff as #18623.

Child Tickets

Change History (14)

comment:1 Changed 4 years ago by nickm

Cc: dgoulet teor nickm added
Keywords: must-fix-before-028-rc regression added
Priority: MediumHigh
Severity: NormalBlocker

comment:2 Changed 4 years ago by nickm

This is reproducible with chutney: you spin up a network, and the authorities immediately crash. On OSX I see:

Mar 24 16:11:01.000 [err] tor_assertion_failed_: Bug: src/or/circuituse.c:119: circuit_is_acceptable: Assertion conn->chosen_exit_name failed; aborting. (on Tor 0.2.8.1-alpha-dev d5f50cb052a535b5)
Mar 24 16:11:01.000 [err] Bug: Assertion conn->chosen_exit_name failed in circuit_is_acceptable at src/or/circuituse.c:119. Stack trace: (on Tor 0.2.8.1-alpha-dev d5f50cb052a535b5)
Mar 24 16:11:01.000 [err] Bug:     0   tor                                 0x000000010bab3f49 log_backtrace + 73 (on Tor 0.2.8.1-alpha-dev d5f50cb052a535b5)
Mar 24 16:11:01.000 [err] Bug:     1   tor                                 0x000000010bac1e97 tor_assertion_failed_ + 151 (on Tor 0.2.8.1-alpha-dev d5f50cb052a535b5)
Mar 24 16:11:01.000 [err] Bug:     2   tor                                 0x000000010b9e7131 circuit_get_best + 1841 (on Tor 0.2.8.1-alpha-dev d5f50cb052a535b5)
Mar 24 16:11:01.000 [err] Bug:     3   tor                                 0x000000010b9e6239 circuit_get_open_circ_or_launch + 1001 (on Tor 0.2.8.1-alpha-dev d5f50cb052a535b5)
Mar 24 16:11:01.000 [err] Bug:     4   tor                                 0x000000010b9e5a60 connection_ap_handshake_attach_circuit + 752 (on Tor 0.2.8.1-alpha-dev d5f50cb052a535b5)
Mar 24 16:11:01.000 [err] Bug:     5   tor                                 0x000000010ba069f7 connection_ap_attach_pending + 167 (on Tor 0.2.8.1-alpha-dev d5f50cb052a535b5)
Mar 24 16:11:01.000 [err] Bug:     6   tor                                 0x000000010ba4d857 do_main_loop + 1351 (on Tor 0.2.8.1-alpha-dev d5f50cb052a535b5)
Mar 24 16:11:01.000 [err] Bug:     7   tor                                 0x000000010ba4fbf6 tor_main + 230 (on Tor 0.2.8.1-alpha-dev d5f50cb052a535b5)
Mar 24 16:11:01.000 [err] Bug:     8   tor                                 0x000000010b9bae79 main + 25 (on Tor 0.2.8.1-alpha-dev d5f50cb052a535b5)
Mar 24 16:11:01.000 [err] Bug:     9   libdyld.dylib                       0x00007fff9c8255ad start + 1 (on Tor 0.2.8.1-alpha-dev d5f50cb052a535b5)

comment:3 Changed 4 years ago by nickm

I ran git bisect on it to find out which commit caused this problem. It appears that ba6509e9e1f9bfd052ea8bdfef104c87ee8ca6b9 is the one that broke things.

commit ba6509e9e1f9bfd052ea8bdfef104c87ee8ca6b9
Author: David Goulet <dgoulet@ev0ke.net>
Date:   Thu Mar 24 13:57:53 2016 -0400

    Fix broken directory request to the DirPort

comment:4 Changed 4 years ago by arma

My branch bug18625 stops the crashing for me.

I'm currently testing it on moria1.

While it's testing, I want to make a few more commits to it before we consider merging.

comment:5 Changed 4 years ago by arma

Status: newneeds_review

Ok, it looks like it's clearly better than the code that triggers an assert immediately. :)

comment:6 Changed 4 years ago by Christian

Cc: tor@… added

comment:7 Changed 4 years ago by nickm

I'll be able to take a look later today when I'm done travelling.

I'd very much like it if we can get teor and dgoulet to have a look as well to confirm that this doesn't unsolve the problems they were trying to solve with the commits you mention in your commit message. ("This change backs out some of the changes made in commit e72cbf7a, and most of the changes made in commit ba6509e9."

comment:8 Changed 4 years ago by nickm

Keywords: TorCoreTeam201603 added

comment:9 Changed 4 years ago by nickm

Keywords: must-fix-before-028-rc, regression TorCoreTeam201603must-fix-before-028-rc, regression, TorCoreTeam201603

comment:10 Changed 4 years ago by nickm

Reviewing now: It all looks plausible at first glance, but I want to make more certain that the reverted commits in question are not undone.

wrt ba6509e9: the point there was to fix #18623, and I am pretty convinced that this patch should do so properly.

wrt e72cbf7a: the point there was IPv6 support, and nothing in this patch should jeopardize that.

So, I'm merging this to master. But I don't want to close it until dgoulet and teor have had a chance to double-check.

comment:11 Changed 4 years ago by teor

f590a303db:

+    /* XXX the below line is suspicious and uncommented. does it close all
+     * consensus fetches if we've already bootstrapped? investigate. */
     if (connection_dir_close_consensus_conn_if_extra(conn)) {
       return;
     }

I'll call this issue A1, split off into #18649.

If we've bootstrapped, this line checks:

  • connection_dir_would_close_consensus_conn_helper which checks:
    • networkstatus_consensus_has_excess_connections which checks:
      • if there's more than one connection (including this connection) retrieving a consensus.
  • if there is, it closes the connection, if not, it leaves it alone.

So it only closes this connection if there are other consensus connections already open.

#18649 resolves this issue 18625.A1 by adding explanatory comments to this and related functions.

comment:12 Changed 4 years ago by teor

Code review:

This all looks good. I am happy that it was merged.

f590a303db: This correctly reverts the parts of e72cbf7a4e where I wrongly assumed that all anonymised connections used the ORPort. The parts dealing with ba6509e9 look ok, but I'd like dgoulet to review those.
fbd79f38c2: yes, that check is now redundant...
98abd49f6f: which makes this variable redundant
8251fe5150: improves readability, doesn't change behaviour

comment:13 Changed 4 years ago by nickm

Thanks, Teor! Leaving this open pending dgoulet having another look too.

comment:14 Changed 4 years ago by dgoulet

Resolution: fixed
Status: needs_reviewclosed

ACK on this patch! I also confirm it works on my two relays and chutney.

Closing this because it has been merged.

Note: See TracTickets for help on using tickets.