[tor-bugs] #20638 [Core Tor/Tor]: Non-anonymous single-hop HS enabled tor doesn't detect already existing anonymous, HS at start-up

[Tor Bug Tracker & Wiki]

#20638: Non-anonymous single-hop HS enabled tor doesn't detect already existing
anonymous, HS at start-up
--------------------------+------------------------------------
 Reporter:  ahf           |          Owner:  teor
     Type:  defect        |         Status:  needs_revision
 Priority:  Medium        |      Milestone:  Tor: 0.2.9.x-final
Component:  Core Tor/Tor  |        Version:  Tor: 0.2.9.3-alpha
 Severity:  Normal        |     Resolution:
 Keywords:  tor-hs, sos   |  Actual Points:  1.0
Parent ID:                |         Points:  0.5
 Reviewer:                |        Sponsor:
--------------------------+------------------------------------
Changes (by asn):

 * status:  needs_review => needs_revision


Comment:

 Nice work teor. Patch seems to work for me. The poisoning detection now
 works without a need for SIGHUP.

 Some nitpicking around the patch:

 - I feel that the `else {` block in `rend_service_check_dir_and_add()` is
 a bit convoluted. For example, I think the `BUG(!s_list)` block can be
 turned into an assert, to assist with readability.

 - On the same note, in `rendservice.c` I now see three blocks of:
 {{{
   /* If no special service list is provided, then just use the global one.
 */
   /* ended up with more of this */
   if (!service_list) {
     if (BUG(!rend_service_list)) {
       return -1;
     }
     s_list = rend_service_list;
   } else {
     s_list = service_list;
   }
 }}}
   This is one more than we started with (also see comment:4). I wonder, if
 we can abstract that logic into a `get_hs_service_list()` function that
 returns the service list if found, otherwise it returns NULL.

 - Now some comment nitpicking. Double comment here:
 {{{
     if (service->directory != NULL) { /* Skip dupe for ephemeral services.
 */
       /* Skip dupe for ephemeral services. */
 }}}

   Also, what does this comment mean? I think it's one of those with a
 version string that will rot in the codebase:
 {{{
 /* Ignore service failures until 030 */
 }}}

 - Finally, I think it might be worthwhile to add a small paragraph to the
 commit msg of `59d525d29` to actually explain '''how''' the patch fixes
 the bug.

 I'm turning this into `needs_revision`, please feel free to fix whichever
 of the comments above you find reasonable.

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