Opened 3 years ago

Closed 3 years ago

#20630 closed defect (fixed)

tor_fragile_assert hit in connection_ap_handshake_rewrite_and_attach

Reported by: arma Owned by:
Priority: Medium Milestone: Tor: 0.3.0.x-final
Component: Core Tor/Tor Version:
Severity: Normal Keywords: review-group-12
Cc: Actual Points:
Parent ID: Points:
Reviewer: Sponsor:

Description

Nov 10 16:45:46.971 [warn] tor_bug_occurred_(): Bug: src/or/connection_edge.c:1775: connection_ap_handshake_rewrite_and_attach: This line should not have been reached. (Future instances of this warning will be silenced.) (on Tor 0.3.0.0-alpha-dev 0980787f91cfc420)
Nov 10 16:45:46.972 [warn] Bug: Line unexpectedly reached at connection_ap_handshake_rewrite_and_attach at src/or/connection_edge.c:1775. Stack trace: (on Tor 0.3.0.0-alpha-dev 0980787f91cfc420)
Nov 10 16:45:46.972 [warn] Bug:     src/or/tor(log_backtrace+0x42) [0x7fbee4aed262] (on Tor 0.3.0.0-alpha-dev 0980787f91cfc420)
Nov 10 16:45:46.972 [warn] Bug:     src/or/tor(tor_bug_occurred_+0xb7) [0x7fbee4b05e67] (on Tor 0.3.0.0-alpha-dev 0980787f91cfc420)
Nov 10 16:45:46.972 [warn] Bug:     src/or/tor(connection_ap_handshake_rewrite_and_attach+0xe3a) [0x7fbee4a9cf8a] (on Tor 0.3.0.0-alpha-dev 0980787f91cfc420)
Nov 10 16:45:46.972 [warn] Bug:     src/or/tor(connection_edge_process_inbuf+0x1d0) [0x7fbee4a9d9c0] (on Tor 0.3.0.0-alpha-dev 0980787f91cfc420)
Nov 10 16:45:46.972 [warn] Bug:     src/or/tor(+0xfbbb8) [0x7fbee4a95bb8] (on Tor 0.3.0.0-alpha-dev 0980787f91cfc420)
Nov 10 16:45:46.972 [warn] Bug:     src/or/tor(+0x457b1) [0x7fbee49df7b1] (on Tor 0.3.0.0-alpha-dev 0980787f91cfc420)
Nov 10 16:45:46.972 [warn] Bug:     /usr/lib/x86_64-linux-gnu/libevent-2.0.so.5(event_base_loop+0x7fc) [0x7fbee40233dc] (on Tor 0.3.0.0-alpha-dev 0980787f91cfc420)
Nov 10 16:45:46.972 [warn] Bug:     src/or/tor(do_main_loop+0x234) [0x7fbee49e0834] (on Tor 0.3.0.0-alpha-dev 0980787f91cfc420)
Nov 10 16:45:46.972 [warn] Bug:     src/or/tor(tor_main+0x1c25) [0x7fbee49e3f85] (on Tor 0.3.0.0-alpha-dev 0980787f91cfc420)
Nov 10 16:45:46.972 [warn] Bug:     src/or/tor(main+0x19) [0x7fbee49dc119] (on Tor 0.3.0.0-alpha-dev 0980787f91cfc420)
Nov 10 16:45:46.972 [warn] Bug:     /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xf5) [0x7fbee2ff4b45] (on Tor 0.3.0.0-alpha-dev 0980787f91cfc420)
Nov 10 16:45:46.972 [warn] Bug:     src/or/tor(+0x42169) [0x7fbee49dc169] (on Tor 0.3.0.0-alpha-dev 0980787f91cfc420)

See #20494 for something related but apparently not the same.

Repeatably triggerable by "torify wget seul.org".

Child Tickets

Change History (7)

comment:1 Changed 3 years ago by arma

bug not present in maint-0.2.9

also triggerable by "tor-resolve seul.org"

comment:2 Changed 3 years ago by arma

This is the trouble:

-    } else if (socks->command == SOCKS_COMMAND_CONNECT) {
+    }
+
+    /* Now see if this is a connect request that we can reject immediately */
+    if (socks->command == SOCKS_COMMAND_CONNECT) {

which came in git commit f3e158ed.

In the old code, the flow was:

    if (socks->command == SOCKS_COMMAND_RESOLVE) {
       ...
    } else if (socks->command == SOCKS_COMMAND_CONNECT) {
      ...
    } else if (socks->command == SOCKS_COMMAND_RESOLVE_PTR) {
      ...
    } else {
      /* We should only be doing CONNECT or RESOLVE! */
      tor_fragile_assert();
    }

The new flow is

    if (socks->command == SOCKS_COMMAND_RESOLVE) {
      ...
    }

    if (socks->command == SOCKS_COMMAND_CONNECT) {
      ...
    } else if (socks->command == SOCKS_COMMAND_RESOLVE_PTR) {
      ...
    } else {
      /* We should only be doing CONNECT or RESOLVE! */
      tor_fragile_assert();
    }

comment:3 Changed 3 years ago by dgoulet

Milestone: Tor: 0.3.0.x-final

Oh wow, that commit was suppose to be only documentation... we should investigate what else that commit did changed in the code.

I see this:

+    /* Now see if this is a connect request that we can reject immediately */
+    if (socks->command == SOCKS_COMMAND_CONNECT) {
+    } else {
+      tor_assert_nonfatal_unreached_once();
+    if (retval < 1) {
+      /* We were either told "-1" (complete failure) or 0 (circuit in
+       * progress); we can't attach this stream yet. */
       return retval;
+    }
-    uint8_t new_circ_purpose;
...
+    /* What purpose do we need to launch this circuit with? */
+    uint8_t new_circ_purpose;

TBH, safe bet here would be to revert, clean that patch again for ONLY comments and extra commit(s) for any code change. That commit hasn't been released so not that problematic.

comment:4 Changed 3 years ago by nickm

Status: newneeds_review

Branch bug20630 in my public repository has a minimal fix here.

comment:5 Changed 3 years ago by nickm

Keywords: review-group-12 added

comment:6 Changed 3 years ago by arma

Status: needs_reviewmerge_ready

Patch seems to work!

I looked over the rest of f3e158ed and didn't see any more gotchas.

comment:7 Changed 3 years ago by nickm

Resolution: fixed
Status: merge_readyclosed

Merging!

Note: See TracTickets for help on using tickets.