[tor-bugs] #14847 [Tor]: Controller: add a command to fetch HS descriptor from HSdir(s)

[Tor Bug Tracker & Wiki]

#14847: Controller: add a command to fetch HS descriptor from HSdir(s)
-----------------------------+----------------------------------------
     Reporter:  dgoulet      |      Owner:  dgoulet
         Type:  enhancement  |     Status:  needs_revision
     Priority:  normal       |  Milestone:  Tor: 0.2.7.x-final
    Component:  Tor          |    Version:
   Resolution:               |   Keywords:  SponsorR tor-hs controller
Actual Points:               |  Parent ID:  #3521
       Points:               |
-----------------------------+----------------------------------------

Comment (by dgoulet):

 Big thanks Yawning for that thorough review!

 Most of it has been fixed and pushed here. Will probably update those
 branches once I have some answers to questions below.

 Spec: `ticket14847_11`
 Code: `bug14847_027_04`

 Here are some comments on the "C code philosophical part" and also
 technical. :)

 > * src/common/util.c:string_is_hex():

 This function is not used anymore so I removed the commit entirely.

 > * Behavioral question. If I issue 2 identical "HSFETCH" commands in
 rapid succession, how many events do I get? The spec implies that tor will
 send me duplicate events but that's not the case.

 Weird, one HSFETCH should trigger two events, HS_DESC and HS_DESC_CONTENT.
 Thus in your case 4 events. I just tested multiple times and I actually
 have the right amount. Maybe that's worth investigating more if you can
 reproduce that. Maybe if no more hsdirs are usable, we loose events?

 > * Use rend_valid_service_id() to check HS address validity.

 Fixed! Added a patch on top.

 > * Instead of removing out arg1, just so you can use SMARTLIST_FOREACH,
 for (int i = 1; i < smartlist_len(args); ++i) followed by
 smartlist_get(args, i) will suffice (and is consistent with the rest of
 the control code).

 Not sure I understand this one. I don't see this being used in control.c.
 Can you clarify the loop you have in mind for me to skip the first arg?

 > * (Style) You can just return 0; if getargs_helper() returns NULL since
 there is 0 cleanup to be done. See >above regarding moving variable
 declarations around

 Personally, I *try* to have only one return call site with goto labels for
 cleanup. In this case, I would prefer an `exit:` label instead of adding a
 return. I'm not strongly fighting against that tbh, I prefer consistent
 and maintainable code over stylish vanity thus here I would probably hand
 it over to the maintainer for a decision? :)

 > * (Style) if (hsaddress) { ... } else { tor_assert(desc_id); } instead
 of a branch containing an assert.

 I strongly advocate against `if/else if` statement that DOES NOT have a
 final `else`. Over time, that's only error prone but sometimes you don't
 have a choice thus a nice comment to explain why is important.

 In this case, I went for the "else tor_assert" because `desc_id` should be
 treated as a single value and not a "fallback". Assuming future code that
 might add an other variable, we'll end up splitting the `else` statement
 and breaking the git blame :(. It's so much more easier to read an if/else
 statement when the value being tested is actually the one being worked on
 (in the case of input parsing).

 > src/or/rendclient.c:rend_client_refetch_v2_renddesc(): No longer need to
 log a warning when you fail to compute the v2 rend descriptor ID?

 I'm not seeing any log warnings in that function about that. Maybe you had
 an other function in mind?

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