Opened 3 years ago

Closed 3 years ago

#20472 closed defect (fixed)

circuit_pick_extend_handshake: Non-fatal assertion !(node_prev == NULL) failed

Reported by: cypherpunks Owned by:
Priority: Medium Milestone: Tor: 0.2.9.x-final
Component: Core Tor/Tor Version: Tor: 0.2.9.3-alpha
Severity: Normal Keywords: regression, 029-backport, TorCoreTeam201611
Cc: Actual Points: 0.5
Parent ID: Points: 0.3
Reviewer: Sponsor:

Description

Oct 26 16:32:51.000 [warn] tor_bug_occurred_: Bug: src/or/circuitbuild.c:859: circuit_pick_extend_handshake: Non-fatal assertion !(node_prev == NULL) failed. (on Tor 0.2.9.3-alpha-dev 153ff4f559d9b7ee)
Oct 26 16:32:51.000 [warn] Bug: Non-fatal assertion !(node_prev == NULL) failed in circuit_pick_extend_handshake at src/or/circuitbuild.c:859. Stack trace: (on Tor 0.2.9.3-alpha-dev 153ff4f559d9b7ee)
Oct 26 16:32:51.000 [warn] Bug:     0   tor                                 0x000000010b4d8f00 log_backtrace + 64 (on Tor 0.2.9.3-alpha-dev 153ff4f559d9b7ee)
Oct 26 16:32:51.000 [warn] Bug:     1   tor                                 0x000000010b586906 tor_bug_occurred_ + 790 (on Tor 0.2.9.3-alpha-dev 153ff4f559d9b7ee)
Oct 26 16:32:51.000 [warn] Bug:     2   tor                                 0x000000010ad024f3 circuit_pick_extend_handshake + 435 (on Tor 0.2.9.3-alpha-dev 153ff4f559d9b7ee)
Oct 26 16:32:51.000 [warn] Bug:     3   tor                                 0x000000010acfbf6a circuit_send_next_onion_skin + 12362 (on Tor 0.2.9.3-alpha-dev 153ff4f559d9b7ee)
Oct 26 16:32:51.000 [warn] Bug:     4   tor                                 0x000000010ad0b99f circuit_extend_to_new_exit + 335 (on Tor 0.2.9.3-alpha-dev 153ff4f559d9b7ee)
Oct 26 16:32:51.000 [warn] Bug:     5   tor                                 0x000000010b2dcd0a rend_client_reextend_intro_circuit + 1450 (on Tor 0.2.9.3-alpha-dev 153ff4f559d9b7ee)
Oct 26 16:32:51.000 [warn] Bug:     6   tor                                 0x000000010b2df3dc rend_client_introduction_acked + 5276 (on Tor 0.2.9.3-alpha-dev 153ff4f559d9b7ee)
Oct 26 16:32:51.000 [warn] Bug:     7   tor                                 0x000000010b2f8848 rend_process_relay_cell + 1464 (on Tor 0.2.9.3-alpha-dev 153ff4f559d9b7ee)
Oct 26 16:32:51.000 [warn] Bug:     8   tor                                 0x000000010b2a3fe2 connection_edge_process_relay_cell + 37938 (on Tor 0.2.9.3-alpha-dev 153ff4f559d9b7ee)
Oct 26 16:32:51.000 [warn] Bug:     9   tor                                 0x000000010b2960ce circuit_receive_relay_cell + 2798 (on Tor 0.2.9.3-alpha-dev 153ff4f559d9b7ee)
Oct 26 16:32:51.000 [warn] Bug:     10  tor                                 0x000000010add98a0 command_process_relay_cell + 5488 (on Tor 0.2.9.3-alpha-dev 153ff4f559d9b7ee)
Oct 26 16:32:51.000 [warn] Bug:     11  tor                                 0x000000010add38f9 command_process_cell + 409 (on Tor 0.2.9.3-alpha-dev 153ff4f559d9b7ee)
Oct 26 16:32:51.000 [warn] Bug:     12  tor                                 0x000000010ac7e89f channel_queue_cell + 2447 (on Tor 0.2.9.3-alpha-dev 153ff4f559d9b7ee)
Oct 26 16:32:51.000 [warn] Bug:     13  tor                                 0x000000010ac9f3a3 channel_tls_handle_cell + 4099 (on Tor 0.2.9.3-alpha-dev 153ff4f559d9b7ee)
Oct 26 16:32:51.000 [warn] Bug:     14  tor                                 0x000000010af2af62 connection_or_process_cells_from_inbuf + 3170 (on Tor 0.2.9.3-alpha-dev 153ff4f559d9b7ee)
Oct 26 16:32:51.000 [warn] Bug:     15  tor                                 0x000000010af2881e connection_or_process_inbuf + 1454 (on Tor 0.2.9.3-alpha-dev 153ff4f559d9b7ee)
Oct 26 16:32:51.000 [warn] Bug:     16  tor                                 0x000000010aed2e9b connection_process_inbuf + 427 (on Tor 0.2.9.3-alpha-dev 153ff4f559d9b7ee)
Oct 26 16:32:51.000 [warn] Bug:     17  tor                                 0x000000010aeafc19 connection_handle_read_impl + 7033 (on Tor 0.2.9.3-alpha-dev 153ff4f559d9b7ee)
Oct 26 16:32:51.000 [warn] Bug:     18  tor                                 0x000000010aeae068 connection_handle_read + 40 (on Tor 0.2.9.3-alpha-dev 153ff4f559d9b7ee)
Oct 26 16:32:51.000 [warn] Bug:     19  tor                                 0x000000010b15eb2a conn_read_callback + 490 (on Tor 0.2.9.3-alpha-dev 153ff4f559d9b7ee)
Oct 26 16:32:51.000 [warn] Bug:     20  tor                                 0x000000010b6c71a6 event_base_loop + 1858 (on Tor 0.2.9.3-alpha-dev 153ff4f559d9b7ee)
Oct 26 16:32:51.000 [warn] Bug:     21  tor                                 0x000000010b181819 run_main_loop_once + 1433 (on Tor 0.2.9.3-alpha-dev 153ff4f559d9b7ee)
Oct 26 16:32:51.000 [warn] Bug:     22  tor                                 0x000000010b16bb62 run_main_loop_until_done + 34 (on Tor 0.2.9.3-alpha-dev 153ff4f559d9b7ee)
Oct 26 16:32:51.000 [warn] Bug:     23  tor                                 0x000000010b168f58 do_main_loop + 4504 (on Tor 0.2.9.3-alpha-dev 153ff4f559d9b7ee)
Oct 26 16:32:51.000 [warn] Bug:     24  tor                                 0x000000010b16f0ac tor_main + 1132 (on Tor 0.2.9.3-alpha-dev 153ff4f559d9b7ee)
Oct 26 16:32:51.000 [warn] Bug:     25  tor                                 0x000000010ac269d0 main + 48 (on Tor 0.2.9.3-alpha-dev 153ff4f559d9b7ee)
Oct 26 16:32:51.000 [warn] Bug:     26  libdyld.dylib                       0x00007fff9d8c65ad start + 1 (on Tor 0.2.9.3-alpha-dev 153ff4f559d9b7ee)
Oct 26 16:32:51.000 [warn] Bug:     27  ???                                 0x000000000000000f 0x0 + 15 (on Tor 0.2.9.3-alpha-dev 153ff4f559d9b7ee)

Child Tickets

Change History (8)

comment:1 Changed 3 years ago by teor

Keywords: TorCoreTeam201610 regression added

Thanks for this bug report.

This is a rare case:

  • tor creates an introduction circuit
  • some time later, the introduction point sends a NAK
  • tor extends through the previous intro point to a new intro point
  • the previous intro point is not in the consensus, either because it dropped out, or because it was never there to begin with.

This is the calling code:

    {
      const node_t *prev_node;
      prev_node = node_get_by_id(hop->prev->extend_info->identity_digest);
      circuit_pick_extend_handshake(&ec.cell_type,
                                    &ec.create_cell.cell_type,
                                    &ec.create_cell.handshake_type,
                                    prev_node,
                                    hop->extend_info);
    }

I think the correct way to fix this is to allow node_prev to be NULL, and skip the version checks in that case. We can't just do this for old-style hidden service, because it's also possible that nodes drop out of the consensus while the circuit is being built.

Bugfix on commit 10aa913 from #19163 in tor-0.2.9.3-alpha.

comment:2 Changed 3 years ago by teor

Actual Points: 0.3
Keywords: 029-backport added
Points: 0.3
Status: newneeds_review

See my branch bug20472-029, which also merges cleanly into master:

  • remove the bug warning
  • don't assume that nodes with unknown versions support EXTEND2 cells
  • rename the supports_ntor function to avoid confusion between EXTEND2 and ntor support

This passes make test-full on master and make check test-network-all 0.2.9, except for #20476 and #20477, which appear to be unrelated.

comment:3 Changed 3 years ago by nickm

Are there any versions on the network that don't support EXTEND2, though? If not, we might be better off just using EXTEND2 unconditionally in this case.

comment:4 in reply to:  3 Changed 3 years ago by teor

Status: needs_reviewneeds_revision

Replying to nickm:

Are there any versions on the network that don't support EXTEND2, though? If not, we might be better off just using EXTEND2 unconditionally in this case.

tor-spec.txt says:

   [Support for EXTEND2 was added in Tor 0.2.4.8-alpha.]

   Clients SHOULD use the EXTEND format whenever sending a TAP
   handshake, and MUST use it whenever the EXTEND cell will be handled
   by a node running a version of Tor too old to support EXTEND2.  In
   other cases, clients SHOULD use EXTEND2.

   When encoding a non-TAP handshake in an EXTEND cell, clients SHOULD
   use the format with 'client handshake type tag'.

And 0.2.4.8 is not in recommended versions, and has not been for some time, and the directory authorities reject descriptors less than 0.2.4.18.

So the answer is: there are no nodes that affirmatively claim to only support EXTEND, but some custom or versionless implementations might not support it, and might not tell us. And other nodes could say they don't support it using the new supported protocols lines in descriptors.

Oh, but some bridges might only support CREATE/EXTEND. I don't think this matters, because we use CREATE_FAST for the first hop anyway.

So I think you're right, we should simplify this logic and just use TAP/EXTEND/CREATE, NTOR/EXTEND2/CREATE2. And just assume versionless/unknown version/etc. nodes support EXTEND2. It's not worth having the extra code just for a few nodes that are really old.

The corresponding tor-spec.txt for CREATE is:

   In general, clients SHOULD use CREATE whenever they are using the TAP
   handshake, and CREATE2 otherwise.  Clients SHOULD NOT send the
   second format of CREATE cells (the one with the handshake type tag)
   to a server directly.

   Servers always reply to a successful CREATE with a CREATED, and to a
   successful CREATE2 with a CREATED2.  On failure, a server sends a
   DESTROY cell to tear down the circuit.

   [CREATE2 is handled by Tor 0.2.4.7-alpha and later.]

comment:5 Changed 3 years ago by teor

(It's a different case for supporting TAP keys, because we don't just need to know *whether* a node supports ntor, we also need its ntor key, which the current HS protocol just doesn't provide.)

comment:6 Changed 3 years ago by teor

Actual Points: 0.30.5
Status: needs_revisionneeds_review

Please see my branch bug20472-029-v2 on github.
It passes make check-spaces check test-network-all

If we can assume every node supports EXTEND2, circuit_pick_extend_handshake becomes very simple. (All we need to know is whether the next hop has an ntor key.)

I also refactored circuit_pick_create_handshake so it matched circuit_pick_extend_handshake, and renamed routerstatus_version_supports_ntor, because it was actually checking extend2 support.

These refactoring commits aren't essential, they can go in 0.3.0 if you want.
(That said, they are trivially correct.)

comment:7 Changed 3 years ago by dgoulet

Keywords: TorCoreTeam201611 added; TorCoreTeam201610 removed

comment:8 Changed 3 years ago by nickm

Resolution: fixed
Status: needs_reviewclosed

Looks good; merging to 029.

Note: See TracTickets for help on using tickets.