[tor-bugs] #18616 [Tor]: Bug: Node search initiated by. Stack trace:

[Tor Bug Tracker & Wiki]

#18616: Bug: Node search initiated by. Stack trace:
-------------------------------------------------+-------------------------
 Reporter:  toralf                               |          Owner:
     Type:  defect                               |         Status:
 Priority:  Medium                               |  needs_revision
Component:  Tor                                  |      Milestone:  Tor:
 Severity:  Normal                               |  0.2.8.x-final
 Keywords:  regression, must-fix-before-028-rc,  |        Version:  Tor:
  TorCoreTeam201603                              |  0.2.8.1-alpha
Parent ID:                                       |     Resolution:
 Reviewer:                                       |  Actual Points:
                                                 |         Points:
                                                 |        Sponsor:
-------------------------------------------------+-------------------------

Comment (by teor):

 Replying to [comment:15 dgoulet]:
 > Replying to [comment:14 nickm]:
 > > Teor, Dgoulet: Do you think this is an alpha-blocker?
 >
 > TL;DR; imo, this need more work so we could wait after this alpha
 release.
 >
 > I think we need to "massage" a bit this patch. For instance,
 `check_whether_dirport_reachable` will now call `dir_server_mode` thus
 `router_should_be_directory_server` but then in
 `decide_to_advertise_dirport` we call both functions one after the other.

 Yes, I find this group of functions quite tangled. I am not sure how to
 simplify them though.

 > Furthermore, I'm not entirely sure about the addition of
 `dir_server_mode` to the dirport reachable check function. Actually, this
 whole function is confusing:
 >
 > {{{
 > /** Return 1 if we don't have a dirport configured, or if it's
 reachable. */
 > int
 > check_whether_dirport_reachable(void)
 > {
 >   const or_options_t *options = get_options();
 >   return !options->DirPort_set ||
 >          !dir_server_mode(options) ||
 >          options->AssumeReachable ||
 >          net_is_disabled() ||
 >          can_reach_dir_port;
 > }
 > }}}
 >
 > So OK, we return 1 if DirPort is _NOT_ configured but why are we using
 `can_reach_dir_port` without negating it?

 We treat the DirPort is "reachable" if we can't check it for any reason:
 {{{
          !options->DirPort_set ||
          !dir_server_mode(options) ||
          options->AssumeReachable ||
          net_is_disabled() ||
 }}}
 Or if we have checked and it's reachable:
 {{{
          can_reach_dir_port;
 }}}
 I think I added a comment to this effect in the branch. If not, we should.

 > Also, with this patch, `dir_server_mode` also checks the ORPort so is it
 OK to use it in there at all?

 Yes, it's not really possible to run a directory mirror without an ORPort.
 So checking the ORPort is OK. But maybe this could be part of the code
 simplification?

 Do you have any ideas here, dgoulet? I'm stuck, and focused on other
 things right now.

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