[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:
Priority: Very High | needs_revision
Component: Tor | Milestone:
Severity: Blocker | Version:
Keywords: TorCoreTeam201602, must-fix- | Resolution:
before-028-rc | Actual Points:
Parent ID: #17668 | Points:
Reviewer: | Sponsor:
-------------------------------------------------+-------------------------
Comment (by nickm):
Replying to [comment:9 dgoulet]:
> > There is now an `ed25519_voting_fixes` branch in torspec too.
>
> This branch diff with master is _empty_. Maybe you forgot to
commit/push? :)
As above actually see the torspec branch ed25519_voting_again.
> Code review:
>
> * 3941fa64ccf10dda5ac224bb1160564581c0e213
>
> `routers_make_ed_keys_unique()`: the digestmap `by_ed_key` is not
freed.
Calling this `DG1`. Fix in babd9df586312f
> `routers_make_ed_keys_unique()`: can't you use `SMARTLIST_DEL_CURRENT`
directly in the first loop instead of flagging it then removing it?
No because sometimes we flag ri and sometimes we flag ri2. Calling this
`DG2`. Adding comments for this in babd9df586312f
> 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`)
Oh, very interesting. Calling this `DG3`. Will work on a fix.
> * 03d484f2806e0df07d1813d3e69ec4840a42be60
>
> `if (! vrs->has_ed25519_listing)`, has extra space.
If you mean the one between the `!` and the `vrs`, I don't mind a single
space there.
> 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 );
> + }
> + }
> }}}
>
IMO this bug can't actually be hit. But it wouldn't mean that the
consensus was invalid; it would mean that the algorithm in dircollate.h
would be broken.
--
Ticket URL: <https://trac.torproject.org/projects/tor/ticket/18318#comment:13>
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