[tor-bugs] #28970 [Core Tor/Tor]: tor_bug_occurred_(): Bug: ../src/or/hs_client.c:624: setup_intro_circ_auth_key: Non-fatal assertion

[Tor Bug Tracker & Wiki]

#28970: tor_bug_occurred_(): Bug: ../src/or/hs_client.c:624:
setup_intro_circ_auth_key: Non-fatal assertion
-------------------------------------------------+-------------------------
 Reporter:  torcrash                             |          Owner:  dgoulet
     Type:  defect                               |         Status:
                                                 |  assigned
 Priority:  High                                 |      Milestone:  Tor:
                                                 |  0.4.2.x-final
Component:  Core Tor/Tor                         |        Version:  Tor:
                                                 |  0.3.4.9
 Severity:  Critical                             |     Resolution:
 Keywords:  tor-client, tor-hs, postfreeze-ok,   |  Actual Points:  0.1
  040-unreached-must, network-team-roadmap-      |
  august, regression?, 041-unreached-must,       |
  042-should                                     |
Parent ID:  #29995                               |         Points:  5
 Reviewer:  asn                                  |        Sponsor:
                                                 |  Sponsor27-must
-------------------------------------------------+-------------------------
Changes (by dgoulet):

 * actualpoints:   => 0.1


Comment:

 Aaaaaaah!!!

 `hs_client_purge_state()` is what is called on NEWNYM which purges _all_
 descriptor from the client cache.

 Which means that for an INTRO circuit being established, once it opens, we
 lookup the descriptor to get the IP object but hey surprised, no more
 descriptor.

 And I bet you anything that the other case of this assert() in the ticket
 are because of `hs_cache_clean_as_client()` that cleans up the descriptor
 while an introduction is being made. Thin window for this race but a race
 nonetheless.

 I see three options:

 1. Assume that because of the cache clean up, we can end up in this
 situation and thus remove the `BUG()` with a fat comment and close the
 circuit instead of keeping it around.

 2. When cleaning up descriptors, keep them in the cache if a circuit is
 being established. They should be removed from the cache at the next
 iteration of the cleanup (mainloop event).

 3. When purging a descriptor from the cache, we also close all related
 intro circuits that are _not_ yet opened.

 I went with (1) which is probably what we want since this is a race that
 can happen and I wouldn't feel good about keeping out of date stuff in the
 cache or even worst, lying with the NEWNYM that the cache has been
 "purged-but-not-really-because-you-had-an-intro-circuit". (3) is also not
 so bad but (1) is very minimal.

 PR: https://github.com/torproject/tor/pull/1455
 Branch: `ticket28970_042_01`

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