[tor-bugs] #23603 [Core Tor/Tor]: hs: Cleanup race between circuit close and free with the HS circuitmap

[Tor Bug Tracker & Wiki]

#23603: hs: Cleanup race between circuit close and free with the HS circuitmap
------------------------------+--------------------------------
     Reporter:  dgoulet       |      Owner:  (none)
         Type:  defect        |     Status:  new
     Priority:  High          |  Milestone:  Tor: 0.3.2.x-final
    Component:  Core Tor/Tor  |    Version:
     Severity:  Normal        |   Keywords:  tor-hs
Actual Points:                |  Parent ID:
       Points:                |   Reviewer:
      Sponsor:                |
------------------------------+--------------------------------
 Here is the scenario that has been encountered by asn. The following is in
 chronological order:

 1. [02:21:27] An intro point (IP) is picked and circuit A is launched.

 2. [02:23:47] Circuit A never completes and thus is '''marked for close'''
 which is NOT free thus still in our HS circuitmap.

 2. [02:23:48] A second later, the HS housekeeping kicks in (every 1 sec)
 and we notice that we are missing a circuit for this IP so we launch a new
 circuit B. What happens then is this new circuit B takes the place of
 circuit A in the HS circuitmap with the *same* hs_token (the token in this
 case is the IP authentication public key).

 At this point, we have circuit A marked for close and we have circuit B
 that was launched and replaced circuit A with the same hs_token which
 identify the IP. So far so good.

 3. [02:23:48] At the same second, we get a `circuit_build_failed()` which
 is only called from
 `circuit_about_to_free()`->`circuit_close_all_marked()` which means it is
 freeing circuit '''A''' leading to a `hs_circuitmap_remove_circuit()`
 which will use its `hs_token` to remove circuit B because it is the same
 token.

 We end up with no circuit for the intro point IP in our circuitmap.

 4. [02:23:49] A second after our circuit build failed which removed
 circuit B from the circuitmap, we cleanup the IP from our list through the
 house keeping and we launch a new circuit to a new intro point. The reason
 why we removed the IP is either because it has expire, no `node_t` could
 be found or we retried too many times on that circuit which the allowed
 number of retries is 3 right now. When we do remove an IP from our service
 list, we also try to close the circuit but because circuit B got removed
 from the circuitmap, we can't find it thus it is not closed.

 5. [02:24:54] Circuit B completes but we are unable to find the IP in our
 service list. And boom we get the warning:

 In summary, we end up with an intro circuit being established but not in
 our circuitmap leading to the failed lookup when we remove that IP from
 our service list for whatever reason.

 Seems like one thing we could do is actually remove a circuit from the HS
 circuitmap when we mark for close because at that point, the circuit is
 good as dead. We avoid this race between mark for close and actually being
 freed where in between a new circuit to the same destination with the same
 hs_token can be launched.

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/23603>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online