Commit 3ba9b470 authored by Nick Mathewson's avatar Nick Mathewson 🦞
Browse files

Rewrite and fix Guard::copy_status_from. 

The old version of this function was error-prone, and in fact had
errors: it was too easy to forget to add non-persistent fields, and
that's exactly what we forgot in a few cases
(`microdescriptor_missing`, `circ_history`, and
`suspicious_behavior_warned`).

The new version of this function consumes both of the incoming
Guards, and constructs every field explicitly so that we can't
forget to list any.

Closes #429.
parent b3e06b93

crates/tor-guardmgr/src/guard.rs

+28 −11
Original line number Original line Diff line number Diff line
@@ -294,19 +294,36 @@ impl Guard { 
    /// Copy all _non-persistent_ status from `other` to self.

    /// Copy all _non-persistent_ status from `other` to self.

    ///

    ///

    /// Requires that the two `Guard`s have the same ID.

    /// Requires that the two `Guard`s have the same ID.

    pub(crate) fn copy_status_from(&mut self, other: &Guard) {

    pub(crate) fn copy_status_from(self, other: Guard) -> Guard {

        debug_assert_eq!(self.id, other.id);

        debug_assert_eq!(self.id, other.id);





        // TODO: This pattern is easy to mess up; it's too easy to forget to add

        Guard {

        // some non-persistent field to this list.  We should use a better

            // All persistent fields are taken from `self`.

        // pattern here.

            id: self.id,

        self.last_tried_to_connect_at = other.last_tried_to_connect_at;

            orports: self.orports,

        self.retry_at = other.retry_at;

            added_at: self.added_at,

        self.retry_schedule = other.retry_schedule.clone();

            added_by: self.added_by,

        self.reachable = other.reachable;

            disabled: self.disabled,

        self.is_dir_cache = other.is_dir_cache;

            confirmed_at: self.confirmed_at,

        self.exploratory_circ_pending = other.exploratory_circ_pending;

            unlisted_since: self.unlisted_since,

        self.dir_status = other.dir_status.clone();

            unknown_fields: self.unknown_fields,



            // All non-persistent fields get taken from `other`.

            last_tried_to_connect_at: other.last_tried_to_connect_at,

            retry_at: other.retry_at,

            retry_schedule: other.retry_schedule,

            reachable: other.reachable,

            is_dir_cache: other.is_dir_cache,

            exploratory_circ_pending: other.exploratory_circ_pending,

            microdescriptor_missing: other.microdescriptor_missing,

            circ_history: other.circ_history,

            suspicious_behavior_warned: other.suspicious_behavior_warned,

            dir_status: other.dir_status,

            // Note that we _could_ remove either of the above blocks and add

            // `..self` or `..other`, but that would be risky: it would increase

            // the odds that we would forget to add some persistent or

            // non-persistent field to the right group in the future.

        }

    }

    }





    /// Change the reachability status for this guard.

    /// Change the reachability status for this guard.

crates/tor-guardmgr/src/lib.rs

+4 −4
Original line number Original line Diff line number Diff line
@@ -610,8 +610,8 @@ impl GuardSets { 




    /// Update all non-persistent state for the guards in this object with the

    /// Update all non-persistent state for the guards in this object with the

    /// state in `other`.

    /// state in `other`.

    fn copy_status_from(&mut self, other: &GuardSets) {

    fn copy_status_from(&mut self, other: GuardSets) {

        self.default.copy_status_from(&other.default);

        self.default.copy_status_from(other.default);

    }

    }

}

}
@@ -669,8 +669,8 @@ impl GuardMgrInner { 
    /// Replace the active guard state with `new_state`, preserving

    /// Replace the active guard state with `new_state`, preserving

    /// non-persistent state for any guards that are retained.

    /// non-persistent state for any guards that are retained.

    fn replace_guards_with(&mut self, mut new_guards: GuardSets, now: SystemTime) {

    fn replace_guards_with(&mut self, mut new_guards: GuardSets, now: SystemTime) {

        new_guards.copy_status_from(&self.guards);

        std::mem::swap(&mut self.guards, &mut new_guards);

        self.guards = new_guards;

        self.guards.copy_status_from(new_guards);

        self.update(now, None);

        self.update(now, None);

    }

    }

crates/tor-guardmgr/src/sample.rs

+13 −6
Original line number Original line Diff line number Diff line
@@ -181,12 +181,19 @@ impl GuardSet { 
    }

    }





    /// Copy non-persistent status from every guard shared with `other`.

    /// Copy non-persistent status from every guard shared with `other`.

    pub(crate) fn copy_status_from(&mut self, other: &GuardSet) {

    pub(crate) fn copy_status_from(&mut self, mut other: GuardSet) {

        for (id, guard) in &mut self.guards {

        let mut old_guards = HashMap::new();

            if let Some(other_guard) = other.get(id) {

        std::mem::swap(&mut old_guards, &mut self.guards);

                guard.copy_status_from(other_guard);

        self.guards = old_guards

            }

            .into_iter()

            .map(|(id, guard)| {

                if let Some(other_guard) = other.guards.remove(&id) {

                    (id, guard.copy_status_from(other_guard))

                } else {

                    (id, guard)

                }

                }

            })

            .collect();

    }

    }





    /// Return a serializable state object that can be stored to disk

    /// Return a serializable state object that can be stored to disk