[tor-bugs] #31652 [Core Tor/Tor]: hs-v3: Service circuit retry limit should not close a valid circuit

[Tor Bug Tracker & Wiki]

#31652: hs-v3: Service circuit retry limit should not close a valid circuit
---------------------------------------------+-----------------------------
 Reporter:  dgoulet                          |          Owner:  neel
     Type:  defect                           |         Status:
                                             |  needs_revision
 Priority:  Medium                           |      Milestone:  Tor:
                                             |  0.4.2.x-final
Component:  Core Tor/Tor                     |        Version:
 Severity:  Normal                           |     Resolution:
 Keywords:  tor-hs, tor-circuit, 042-should  |  Actual Points:
Parent ID:  #30200                           |         Points:  0.1
 Reviewer:  asn                              |        Sponsor:
                                             |  Sponsor27-must
---------------------------------------------+-----------------------------
Changes (by asn):

 * status:  needs_review => needs_revision


Comment:

 Neel, I'm still confused here...

 Why does a function called `should_not_retry_intro_point()` allow the
 possibility of a retry when we have gone over the number of maximum
 retries? In my view, it should always return `false` in that case.

 In particular the following block:
 {{{
   /* If we have gone over the number of retried circuits, make sure we
 don't
    * already have an established circuit. */
   if (ip->circuit_retries > MAX_INTRO_POINT_CIRCUIT_RETRIES) {
     return !ip->circuit_established || hs_circ_service_get_intro_circ(ip);
   }
 }}}
 is confusing me even tho I've read it a few times. When I'm reading that
 function my logic would be "If we have gone over the number of retried
 circuits, we only allow retries if ...", but then why would we allow
 retries if we are past the max?  Also the comment mentions the established
 circuit clause, but not the `hs_circ_service_get_intro_circ(ip);` one. Can
 you please clarify that logic further? :/

 We seem to have left some comments in `cleanup_intro_points()` that are
 only relevant to `should_not_retry_intro_point()`.

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