Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#3228 closed defect (fixed)

Setting BridgeRelay Triggers Assertion Failure

Reported by: atagar Owned by:
Priority: High Milestone: Tor: 0.2.2.x-final
Component: Core Tor/Tor Version:
Severity: Keywords: tor-bridge
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Hi, with the current git checkout...
Tor v0.2.2.27-beta (git-e3b8457eb38ee76a)

And also back at:
Tor v0.2.2.23-alpha (git-b85eb949b528f4d7)

running "SETCONF BridgeRelay=1" triggers an assertion failure and Tor crash:
May 18 20:42:13.855 [err] assert_identity_keys_ok(): Bug: router.c:154: assert_identity_keys_ok: Assertion 0!=crypto_pk_cmp_keys(client_identitykey, server_identitykey) failed; aborting.
router.c:154 assert_identity_keys_ok: Assertion 0!=crypto_pk_cmp_keys(client_identitykey, server_identitykey) failed; aborting.
Aborted

Assertion Source:
https://gitweb.torproject.org/tor.git/blob/HEAD:/src/or/router.c#l154

My torrc before the crash:

ORPort 9001
Nickname armTest
ExitPolicy reject *:* 

ControlPort 9051
CookieAuthentication 1

AccountingMax 100 GB

Cheers! -Damian

Child Tickets

Change History (24)

comment:1 Changed 8 years ago by arma

Component: Tor RelayTor Bridge
Milestone: Tor: 0.2.2.x-final
Priority: normalmajor

comment:2 Changed 8 years ago by nickm

Hi! It would be great to have a traceback for this.

comment:3 Changed 8 years ago by rransom

options_act -> configure_accounting -> accounting_set_wakeup_time -> get_server_identity_key

We should rip out assert_identity_keys_ok.

comment:4 Changed 8 years ago by arma

For completeness:

#3  0x08076195 in assert_identity_keys_ok () at router.c:149
#4  0x08076399 in get_server_identity_key () at router.c:167
#5  0x081049e2 in accounting_set_wakeup_time (now=1306023199)
    at hibernate.c:536
#6  configure_accounting (now=1306023199) at hibernate.c:404
#7  0x080bb4ae in options_act (old_options=<value optimized out>)
    at config.c:1256
#8  0x080bc272 in set_options (new_val=0x9ddf248, msg=0xbf9d7e98)
    at config.c:667
#9  0x080bd441 in options_trial_assign (list=0x9e32200, use_defaults=0, 
    clear_first=1, msg=0xbf9d7e98) at config.c:2255
#10 0x080d8b16 in control_setconf_helper (conn=<value optimized out>, 
    len=<value optimized out>, body=0x2 <Address 0x2 out of bounds>, 
    use_defaults=0) at control.c:760
#11 0x080d9870 in handle_control_resetconf (conn=0x9d06fe0) at control.c:807
#12 connection_control_process_inbuf (conn=0x9d06fe0) at control.c:3005
#13 0x080bf9d5 in connection_process_inbuf (conn=0x0, package_partial=6)
    at connection.c:3351
#14 0x080c5812 in connection_handle_read_impl (conn=0x9d06fe0)
    at connection.c:2510
#15 connection_handle_read (conn=0x9d06fe0) at connection.c:2550
#16 0x08051514 in conn_read_callback (fd=16, event=2, _conn=0x9d06fe0)
    at main.c:514
#17 0xb784eee4 in event_base_loop () from /usr/lib/libevent-1.4.so.2
#18 0x0804f2c9 in do_main_loop () at main.c:1560
#19 0x0804f5ed in tor_main (argc=5, argv=0xbf9d8234) at main.c:2222
#20 0x0804dbab in main (argc=5, argv=0xbf9d8234) at tor_main.c:30

comment:5 Changed 8 years ago by arma

It looks like commit 03adb8caad made some assumptions that aren't true in transition.

I suggest we fix it in 0.2.2 by doing as rransom suggests -- basically revert 03adb8caa. Then Nick should decide whether we wants to try revising his assumptions for 0.2.3, or what.

comment:6 Changed 8 years ago by Sebastian

it seems totally weird to do all of this server identity key setting here in accounting_set_wakeup_time(). That is a weird side effect of calling that function and really unexpected. I think the assert triggers for a reasonable case here, and we shouldn't just rip it out to silence it. We should explore Roger's XXX later in accounting_set_wakeup_time(), and if we can run with that rip out the identity key retrieval business.

comment:7 Changed 8 years ago by nickm

Status: newneeds_review

I'm with sebastian: we really want our keys to be consistent with our options, and killing the assert seems like a bad answer.

Instead, let's move the init_keys() to the front of options_act(). Have a look at my branch "bug3228". I've confirmed that it fixes the problem here for me.

comment:8 Changed 8 years ago by Sebastian

Yes, that looks good to me.

comment:9 Changed 8 years ago by arma

A) You're still calling init_keys() later in that function for people who just became v3 authorities? Why twice?

B) You probably want to check running_tor in the new location. (Note that you moved the clause above the "if (!running_tor) return" clause.)

C) if options->V3AuthoritativeDir but !old_options it'll seg fault.

comment:10 Changed 8 years ago by Sebastian

a) looks like the later call should just be removed and c) should also definitely be fixed. b) seems to be a nonissue tho, it doesn't really matter afaict? Calling options_transition_affects_workers() has no sideeffects

comment:11 Changed 8 years ago by nickm

Pushed a fixup commit to address these issues. Better now?

We should really merge this soon; assertion failures are nobody's friend.

comment:12 Changed 8 years ago by Sebastian

Yup, looks like you addressed everything.

comment:13 Changed 8 years ago by atagar

Tested with your bug3228 branch and works beautifully. Thanks!

comment:14 Changed 8 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

great; squashed and merged.

comment:15 Changed 8 years ago by arma

Resolution: fixed
Status: closedreopened

comment:16 Changed 8 years ago by arma

moria1 running master asserts at boot:

May 30 16:59:32.000 [err] get_or_state(): Bug: config.c:5132: get_or_state: Assertion global_state failed; aborting.
config.c:5132 get_or_state: Assertion global_state failed; aborting.

#0  0x00007f9aa147fed5 in raise () from /lib/libc.so.6
#1  0x00007f9aa14813f3 in abort () from /lib/libc.so.6
#2  0x000000000046b739 in get_or_state () at config.c:5132
#3  0x0000000000433e05 in init_keys () at router.c:598
#4  0x0000000000472983 in options_act (old_options=0x0) at config.c:1211
#5  0x0000000000473fab in set_options (new_val=<value optimized out>, 
    msg=0x7fffbb141d50) at config.c:686
#6  0x000000000047469f in options_init_from_string (
    cf=0x16aaac0 "#refuseunknownexits 1\n\npidfile moria1.pid\n\ngeoipfile ../git/src/config/geoip\n\n#V3AuthVotingInterval 30 minutes\n\n#protocolwarnings 1\n\nlog notice file moria1-notice\nlog info file moria1-info\nlog debug f"..., 
    command=0, command_arg=0x0, msg=0x7fffbb141d50) at config.c:4352
#7  0x0000000000474a8c in options_init_from_torrc (argc=3, argv=0x7fffbb141fd8)
    at config.c:4226
#8  0x00000000004084c5 in tor_init (argc=3, argv=0x7fffbb141fd8) at main.c:2144
#9  0x0000000000409a56 in tor_main (argc=3, argv=<value optimized out>)
    at main.c:2457
#10 0x00007f9aa146c1a6 in __libc_start_main () from /lib/libc.so.6
#11 0x0000000000407ba9 in _start ()

comment:17 Changed 8 years ago by arma

Apparently this clause in options_act() wants to get run before we call init_keys:

  /* Load state */
  if (! global_state && running_tor) {
    if (or_state_load())
      return -1;
    rep_hist_load_mtbf_data(time(NULL));
  }

comment:18 Changed 8 years ago by arma

I moved the init_keys stanza down below the finish_daemon stanza, and now things start for me.

Did we really need to have init_keys so early?

comment:19 Changed 8 years ago by nickm

We needed it to be before we set up accounting, or did anything else that might try to ask for an identity key.

comment:20 Changed 8 years ago by arma

I believe my bug3228b will make all sides happy.

comment:21 Changed 8 years ago by nickm

seems plausible; feel free to merge.

comment:22 Changed 8 years ago by arma

Resolution: fixed
Status: reopenedclosed

Merged.

Atagar, please reopen if this breaks stuff for you.

comment:23 Changed 7 years ago by nickm

Keywords: tor-bridge added

comment:24 Changed 7 years ago by nickm

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