Commit 1ec0ed45 authored by Nick Mathewson's avatar Nick Mathewson 🤹
Browse files

dirmgr: Note errors and inform the circmgr about them.

Some error types indicate that the guard has failed as a dircache.
We should treat these errors as signs to close the circuit, and to
mark the guard as having failed.
parent 8306a9cd
Loading
Loading
Loading
Loading
+11 −0
Original line number Diff line number Diff line
@@ -85,6 +85,8 @@ pub use config::{
use crate::preemptive::PreemptiveCircuitPredictor;
use usage::TargetCircUsage;

pub use tor_guardmgr::{ExternalFailure, GuardId};

/// A Result type as returned from this crate.
pub type Result<T> = std::result::Result<T, Error>;

@@ -430,6 +432,15 @@ impl<R: Runtime> CircMgr<R> {

        Ok(())
    }

    /// Record that a failure occurred on a circuit with a given guard, in a way
    /// that makes us unwilling to use that guard for future circuits.
    pub fn note_external_failure(&self, id: &GuardId, external_failure: ExternalFailure) {
        self.mgr
            .peek_builder()
            .guardmgr()
            .note_external_failure(id, external_failure);
    }
}

impl<R: Runtime> Drop for CircMgr<R> {
+26 −7
Original line number Diff line number Diff line
@@ -67,10 +67,13 @@ async fn fetch_single<R: Runtime>(
            fbs[..].into()
        }
    };
    let resource =
    let outcome =
        tor_dirclient::get_resource(request.as_requestable(), dirinfo, &dirmgr.runtime, circmgr)
            .await?;
            .await;

    dirmgr.note_request_outcome(&outcome);

    let resource = outcome?;
    Ok((request, resource))
}

@@ -190,20 +193,36 @@ async fn download_attempt<R: Runtime>(
    let missing = state.missing_docs();
    let fetched = fetch_multiple(Arc::clone(dirmgr), missing, parallelism).await?;
    for (client_req, dir_response) in fetched {
        let text =
            String::from_utf8(dir_response.into_output()).map_err(Error::BadUtf8FromDirectory)?;
        let source = dir_response.source().map(Clone::clone);
        let text = match String::from_utf8(dir_response.into_output())
            .map_err(Error::BadUtf8FromDirectory)
        {
            Ok(t) => t,
            Err(e) => {
                if let Some(source) = source {
                    dirmgr.note_cache_error(&source, &e);
                }
                continue;
            }
        };
        match dirmgr.expand_response_text(&client_req, text) {
            Ok(text) => {
                let outcome = state.add_from_download(&text, &client_req, Some(&dirmgr.store));
                match outcome {
                    Ok(b) => changed |= b,
                    // TODO: in this case we might want to stop using this source.
                    Err(e) => warn!("error while adding directory info: {}", e),
                    Err(e) => {
                        warn!("error while adding directory info: {}", e);
                        if let Some(source) = source {
                            dirmgr.note_cache_error(&source, &e);
                        }
                    }
                }
            }
            Err(e) => {
                // TODO: in this case we might want to stop using this source.
                warn!("Error when expanding directory text: {}", e);
                if let Some(source) = source {
                    dirmgr.note_cache_error(&source, &e);
                }
            }
        }
    }
+41 −0
Original line number Diff line number Diff line
@@ -134,6 +134,47 @@ impl Error {
    pub(crate) fn from_netdoc(source: DocSource, cause: tor_netdoc::Error) -> Error {
        Error::NetDocError { source, cause }
    }

    /// Return true if this error is serious enough that we should mark this
    /// cache as having failed.
    pub(crate) fn indicates_cache_failure(&self) -> bool {
        match self {
            // These indicate a problem from the cache.
            Error::Unwanted(_)
            | Error::UnrecognizedAuthorities
            | Error::BadUtf8FromDirectory(_)
            | Error::ConsensusDiffError(_)
            | Error::SignatureError(_)
            | Error::IOError(_) => true,

            // These errors cannot come from a directory cache.
            Error::NoDownloadSupport
            | Error::CacheCorruption(_)
            | Error::SqliteError(_)
            | Error::UnrecognizedSchema
            | Error::BadNetworkConfig(_)
            | Error::DirectoryNotPresent
            | Error::ManagerDropped
            | Error::CantAdvanceState
            | Error::StorageError(_)
            | Error::BadUtf8InCache(_)
            | Error::BadHexInCache(_)
            | Error::OfflineMode
            | Error::Spawn { .. }
            | Error::Bug(_) => false,

            // For this one, we delegate.
            Error::DirClientError(e) => e.should_retire_circ(),

            // TODO: This one is special.  It could mean that the directory
            // cache is serving us bad unparsable stuff, or it could mean that
            // for some reason we're unable to parse a real legit document.
            Error::NetDocError { .. } => true,

            // We can never see this kind of error from within the crate.
            Error::ExternalDirProvider { .. } => false,
        }
    }
}

impl From<rusqlite::Error> for Error {
+48 −0
Original line number Diff line number Diff line
@@ -935,6 +935,54 @@ impl<R: Runtime> DirMgr<R> {
        }
        Ok(text)
    }

    /// If there were errors from a peer in `outcome`, record those errors by
    /// marking the circuit (if any) as needing retirement, and noting the peer
    /// (if any) as having failed.
    fn note_request_outcome(&self, outcome: &tor_dirclient::Result<tor_dirclient::DirResponse>) {
        use tor_dirclient::Error::RequestFailed;
        // Extract an error and a source from this outcome, if there is one.
        let (err, source) = match outcome {
            Ok(req) => {
                if let (Some(e), Some(source)) = (req.error(), req.source()) {
                    (
                        RequestFailed {
                            error: e.clone(),
                            source: Some(source.clone()),
                        },
                        source,
                    )
                } else {
                    return;
                }
            }
            Err(
                error @ RequestFailed {
                    source: Some(source),
                    ..
                },
            ) => (error.clone(), source),
            _ => return,
        };

        self.note_cache_error(source, &err.into());
    }

    /// Record that a problem has occurred because of a failure in an answer from `source`.
    fn note_cache_error(&self, source: &tor_dirclient::SourceInfo, problem: &Error) {
        use tor_circmgr::{ExternalFailure, GuardId};

        if !problem.indicates_cache_failure() {
            return;
        }

        if let Some(circmgr) = &self.circmgr {
            info!("Marking {:?} as failed: {}", source, problem);
            let guard_id = GuardId::from_chan_target(source.cache_id());
            circmgr.note_external_failure(&guard_id, ExternalFailure::DirCache);
            circmgr.retire_circ(source.unique_circ_id());
        }
    }
}

/// A degree of readiness for a given directory state object.