[Author Prev][Author Next][Thread Prev][Thread Next][Author Index][Thread Index]

Re: [tor-bugs] #18318 [Tor]: Make sure keys and IP:Ports are unique in a consensus

#18318: Make sure keys and IP:Ports are unique in a consensus
-------------------------------+------------------------------------
 Reporter:  teor               |          Owner:  nickm
     Type:  defect             |         Status:  needs_revision
 Priority:  Very High          |      Milestone:  Tor: 0.2.7.x-final
Component:  Tor                |        Version:
 Severity:  Blocker            |     Resolution:
 Keywords:  TorCoreTeam201602  |  Actual Points:
Parent ID:  #17668             |         Points:
  Sponsor:                     |
-------------------------------+------------------------------------
Changes (by dgoulet):

 * status:  needs_review => needs_revision


Comment:

 >  There is now an `ed25519_voting_fixes` branch in torspec too.

 This branch diff with master is _empty_. Maybe you forgot to commit/push?
 :)

 Code review:

 * 3941fa64ccf10dda5ac224bb1160564581c0e213

  `routers_make_ed_keys_unique()`: the digestmap `by_ed_key` is not freed.

  `routers_make_ed_keys_unique()`: can't you use `SMARTLIST_DEL_CURRENT`
 directly in the first loop instead of flagging it then removing it?
 Actually, maybe I'm not understanding the whole picture here, but that's
 called in `dirserv_generate_networkstatus_vote_obj()` then few lines below
 `rl` is used with `dirserv_count_measured_bws();` and not the list with
 unique entries `routers`. Is it OK here to use a list of non unique
 entries? (same goes for `dirserv_compute_performance_thresholds`)

 * 03d484f2806e0df07d1813d3e69ec4840a42be60

  `if (! vrs->has_ed25519_listing)`, has extra space.

  Below, we just warn and we continue to try to create a valid consensus
 even though there is an obvious issue? If that bug is hit, will it create
 an invalid consensus?

 {{{
 +      if (ed_consensus > 0) {
 +        tor_assert(consensus_method >= MIN_METHOD_FOR_ED25519_ID_VOTING);
 +        if (ed_consensus <= total_authorities / 2) {
 +          log_warn(LD_BUG, "Not enough entries had ed_consensus set; how
 "
 +                   "can we have a consensus of %d?", ed_consensus );
 +        }
 +      }
 }}}

--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/18318#comment:9>
Tor Bug Tracker & Wiki <https://trac.torproject.org/>
The Tor Project: anonymity online
_______________________________________________
tor-bugs mailing list
tor-bugs@xxxxxxxxxxxxxxxxxxxx
https://lists.torproject.org/cgi-bin/mailman/listinfo/tor-bugs