[tor-bugs] #18809 [Core Tor/Tor]: Handle linked connections better during bootstrap

[Tor Bug Tracker & Wiki]

#18809: Handle linked connections better during bootstrap
-------------------------------------------------+-------------------------
 Reporter:  teor                                 |          Owner:  arma
     Type:  defect                               |         Status:
 Priority:  Medium                               |  needs_revision
Component:  Core Tor/Tor                         |      Milestone:  Tor:
 Severity:  Normal                               |  0.2.8.x-final
 Keywords:  must-fix-before-028-rc,              |        Version:  Tor:
  TorCoreTeam201605, review-group-1              |  0.2.8.1-alpha
Parent ID:                                       |     Resolution:
 Reviewer:  andrea                               |  Actual Points:  medium
                                                 |         Points:  medium
                                                 |        Sponsor:
                                                 |  SponsorS-can
-------------------------------------------------+-------------------------
Changes (by teor):

 * status:  needs_review => needs_revision


Comment:

 Code review:

 ce8266d fix typos/etc before i go nuts on #18809
 function rename only

 91c5801 avoid following through on a consensus fetch if we have one
 already arriving
 Suffers from a race condition where two connections could attach at the
 same time. But this is unlikely and relatively benign: we'll just fetch
 the consensus twice.

 59da060 use the new function here too
 this is refactoring I should have done

 a7665df close other consensus fetches when we get a consensus
 There were two race conditions avoided by the previous code:
 * the old function closed the connection immediately to avoid wasting
 data. But this meant that if a connection was receiving data or undergoing
 other operations, there could be errors. I think calling
 connection_mark_for_close is better, even if it's less efficient.
 * connections can be initiated after
 connection_dir_list_by_purpose_and_resource() is called. These connections
 are never cleaned up. I think this is fixed by checking when we attach a
 stream in 91c5801.

 e230e80 get rid of the scattered checks to cancel a consensus fetch
 these look good, because we catch it in attach now

 bcae392 avoid another redundant check
 good, but I wonder if we'll want
 networkstatus_consensus_is_downloading_usable_flavor in future. probably
 not worth keeping it around

 c98fbd4 remove some more unused code
 probably not worth keeping it around, but I do wonder if we'll want these
 functions

 d5a9628 simplify more -- we only call these funcs when bootstrapping
 makes sense, but you'd better document that those functions should only
 ever be called when bootstrapping

 1f72653 fix a bug where relays would use the aggressive client
 bootstrapping retry number

 I think the correct fix for this is to check `server_mode(options)` in
 `consensus_max_download_tries()`, and `return
 options->TestingConsensusMaxDownloadTries` if it's true.

 Otherwise, this bug is just waiting to happen again.

 aa6341d stop looping once we know what the answer will be
 early exit FTW

 53aaed8 get rid of another no-longer-used function
 looks ok

 507883e rip out the unit tests for the functions i removed
 Please leave the parts of the unit tests that test existing functions, or
 remove those functions if they are unused:
 * connection_dir_list_by_purpose_and_resource
 * connection_dir_count_by_purpose_and_resource

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