diff --git a/crates/tor-guardmgr/src/filter.rs b/crates/tor-guardmgr/src/filter.rs index 2a4ef79db..be60dc80d 100644 --- a/crates/tor-guardmgr/src/filter.rs +++ b/crates/tor-guardmgr/src/filter.rs @@ -104,7 +104,11 @@ impl SingleFilter { } } - /// Modify `first_hop` so that it contains no elements not permitted by this filter. + /// Modify `first_hop` so that it contains no elements not permitted by this + /// filter. + /// + /// It is an internal error to call this function on a guard not already + /// passed by `self.permits()`. fn modify_hop( &self, mut first_hop: crate::FirstHop, @@ -115,6 +119,18 @@ impl SingleFilter { .orports .retain(|addr| patterns.iter().any(|pat| pat.matches_sockaddr(addr))); if first_hop.orports.is_empty() { + // TODO(nickm): The fact that this check needs to be checked + // happen indicates a likely problem in our code design. + // Right now, we have `modify_hop` and `permits` as separate + // methods because our GuardSet logic needs a way to check + // whether a guard will be permitted by a filter without + // actually altering that guard (since another filter might + // be used in the future that would allow the same guard). + // + // To mitigate the risk of hitting this error, we try to + // make sure that modify_hop is always called right after + // (or at least soon after) the filter is checked, with the + // same filter object. return Err(tor_error::internal!( "Tried to apply an address filter to an unsupported guard" ) diff --git a/crates/tor-guardmgr/src/lib.rs b/crates/tor-guardmgr/src/lib.rs index 50257f0ac..abc8280cb 100644 --- a/crates/tor-guardmgr/src/lib.rs +++ b/crates/tor-guardmgr/src/lib.rs @@ -1179,19 +1179,9 @@ impl GuardMgrInner { usage: &GuardUsage, now: Instant, ) -> Result<(sample::ListKind, FirstHop), PickGuardError> { - let (source, id) = self - .guards + self.guards .active_guards() - .pick_guard(usage, &self.params, now)?; - let guard = self - .guards - .active_guards() - .get(&id) - .expect("Selected guard that wasn't in our sample!?") - .get_external_rep(); - let guard = self.guards.active_guards().filter().modify_hop(guard)?; - - Ok((source, guard)) + .pick_guard_ext(usage, &self.params, now) } /// Helper: Select a fallback directory. diff --git a/crates/tor-guardmgr/src/sample.rs b/crates/tor-guardmgr/src/sample.rs index b7ddf0da8..64a307830 100644 --- a/crates/tor-guardmgr/src/sample.rs +++ b/crates/tor-guardmgr/src/sample.rs @@ -4,6 +4,7 @@ use crate::filter::GuardFilter; use crate::guard::{Guard, NewlyConfirmed, Reachable}; use crate::skew::SkewObservation; +use crate::FirstHop; use crate::{ ids::GuardId, ExternalActivity, GuardParams, GuardUsage, GuardUsageKind, PickGuardError, }; @@ -747,10 +748,30 @@ impl GuardSet { Some(false) } + /// Try to select a guard for a given `usage`. + /// + /// On success, returns the kind of guard that we got, and its filtered + /// representation in a form suitable for use as a first hop. + pub(crate) fn pick_guard_ext( + &self, + usage: &GuardUsage, + params: &GuardParams, + now: Instant, + ) -> Result<(ListKind, FirstHop), PickGuardError> { + let (list_kind, id) = self.pick_guard(usage, params, now)?; + let first_hop = self + .get(&id) + .expect("Somehow selected a guard we don't know!") + .get_external_rep(); + let first_hop = self.active_filter.modify_hop(first_hop)?; + + Ok((list_kind, first_hop)) + } + /// Try to select a guard for a given `usage`. /// /// On success, returns the kind of guard that we got, and its identity. - pub(crate) fn pick_guard( + fn pick_guard( &self, usage: &GuardUsage, params: &GuardParams,