new lint: unnecessary_path_exists

CHANGELOG.md

@@ -7449,6 +7449,7 @@ Released 2018-09-13
 [`unnecessary_operation`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_operation
 [`unnecessary_option_map_or_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_option_map_or_else
 [`unnecessary_owned_empty_strings`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_owned_empty_strings
+[`unnecessary_path_exists`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_path_exists
 [`unnecessary_result_map_or_else`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_result_map_or_else
 [`unnecessary_safety_comment`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_safety_comment
 [`unnecessary_safety_doc`]: https://rust-lang.github.io/rust-clippy/master/index.html#unnecessary_safety_doc

clippy_lints/src/declared_lints.rs

@@ -506,6 +506,7 @@ pub static LINTS: &[&::declare_clippy_lint::LintInfo] = &[
     crate::methods::UNNECESSARY_MAP_OR_INFO,
     crate::methods::UNNECESSARY_MIN_OR_MAX_INFO,
     crate::methods::UNNECESSARY_OPTION_MAP_OR_ELSE_INFO,
+    crate::methods::UNNECESSARY_PATH_EXISTS_INFO,
     crate::methods::UNNECESSARY_RESULT_MAP_OR_ELSE_INFO,
     crate::methods::UNNECESSARY_SORT_BY_INFO,
     crate::methods::UNNECESSARY_TO_OWNED_INFO,

clippy_lints/src/methods/mod.rs

@@ -141,6 +141,7 @@ mod unnecessary_literal_unwrap;
 mod unnecessary_map_or;
 mod unnecessary_map_or_else;
 mod unnecessary_min_or_max;
+mod unnecessary_path_exists;
 mod unnecessary_sort_by;
 mod unnecessary_to_owned;
 mod unnecessary_unwrap_unchecked;
@@ -4513,6 +4514,54 @@ declare_clippy_lint! {
     "making no use of the \"map closure\" when calling `.map_or_else(|| 2 * k, |n| n)`"
 }
 
+declare_clippy_lint! {
+    /// ### What it does
+    /// Checks for calls to `Path::exists` immediately before a filesystem
+    /// operation on the same path.
+    ///
+    /// ### Why is this bad?
+    /// Calling `exists()` and then performing a filesystem operation on the same
+    /// path is a classic Time-Of-Check to Time-Of-Use (TOCTOU) race condition.
+    /// Between the two calls another process can add, remove, or replace the
+    /// file, making the result of `exists()` stale. The filesystem operation
+    /// itself will indicate whether the path exists via its return value, making
+    /// the prior `exists()` check both redundant and dangerous.
+    ///
+    /// ### Example
+    /// ```rust,no_run
+    /// # use std::path::Path;
+    /// # fn example(path: &Path) {
+    /// if path.exists() {
+    ///     let metadata = path.metadata().unwrap();
+    ///     // use metadata ...
+    /// }
+    /// # }
+    /// ```
+    /// Use instead:
+    /// ```rust,no_run
+    /// # use std::path::Path;
+    /// # fn example(path: &Path) {
+    /// if let Ok(metadata) = path.metadata() {
+    ///     // use metadata ...
+    /// }
+    /// # }
+    /// ```
+    ///
+    /// ### Known problems
+    /// - Does not detect `std::fs` free functions used inside the block
+    ///   (e.g. `fs::read(path)`, `fs::File::open(path)`), only method calls on
+    ///   the path receiver itself.
+    /// - `Path::try_exists()` (stabilized in Rust 1.63) is only detected when
+    ///   used with the `?` operator (e.g. `if path.try_exists()? { ... }`);
+    ///   `.unwrap()`/`.unwrap_or(..)` and similar are not recognized.
+    /// - For the stored-bool variant (`let b = path.exists(); /* other stmts */;
+    ///   if b { ... }`), only detects when the `if` immediately follows the `let`.
+    #[clippy::version = "1.98.0"]
+    pub UNNECESSARY_PATH_EXISTS,
+    suspicious,
+    "calling `Path::exists` before a filesystem operation creates a TOCTOU race"
+}
+
 declare_clippy_lint! {
     /// ### What it does
     /// Checks for usage of `.map_or_else()` "map closure" for `Result` type.
@@ -5068,6 +5117,7 @@ impl_lint_pass!(Methods => [
     UNNECESSARY_MAP_OR,
     UNNECESSARY_MIN_OR_MAX,
     UNNECESSARY_OPTION_MAP_OR_ELSE,
+    UNNECESSARY_PATH_EXISTS,
     UNNECESSARY_RESULT_MAP_OR_ELSE,
     UNNECESSARY_SORT_BY,
     UNNECESSARY_TO_OWNED,
@@ -5421,6 +5471,9 @@ impl Methods {
                     }
                     path_ends_with_ext::check(cx, recv, arg, expr, self.msrv, &self.allowed_dotfiles);
                 },
+                (sym::exists | sym::try_exists, []) => {
+                    unnecessary_path_exists::check(cx, expr, recv);
+                },
                 (sym::expect, [_]) => {
                     match method_call(recv) {
                         Some((sym::ok, recv_inner, [], _, _)) => ok_expect::check(cx, expr, recv, recv_inner),

clippy_lints/src/methods/unnecessary_path_exists.rs

@@ -0,0 +1,211 @@
+use super::UNNECESSARY_PATH_EXISTS;
+use clippy_utils::diagnostics::span_lint_and_then;
+use clippy_utils::res::MaybeDef;
+use clippy_utils::{SpanlessEq, get_enclosing_block, get_parent_expr, higher, path_to_local_with_projections, sym};
+use rustc_hir::{BinOpKind, Expr, ExprKind, MatchSource, Node, PatKind, StmtKind};
+use rustc_lint::LateContext;
+use rustc_span::{Span, SyntaxContext};
+
+/// `expr` is a `.exists()` call on `recv`. Find out whether it's used either
+/// directly (or through a chain of `&&`) as an `if` condition, or stored in a
+/// `let` binding that's immediately checked by the following `if`, and if so
+/// look for a redundant filesystem operation in the `then` branch.
+pub(super) fn check<'tcx>(cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>, recv: &'tcx Expr<'tcx>) {
+    if is_path_method_call(cx, expr)
+        && let Some((then, ctxt)) = if_then_from_condition(cx, expr).or_else(|| if_then_from_stored_bool(cx, expr))
+        && let Some(fs_call_span) = find_fs_call(cx, then, recv, ctxt)
+    {
+        span_lint_and_then(
+            cx,
+            UNNECESSARY_PATH_EXISTS,
+            expr.span,
+            "unnecessary `Path::exists` before a filesystem operation on the same path",
+            |diag| {
+                diag.span_note(fs_call_span, "the filesystem operation is here");
+                diag.help(
+                    "the `exists()` check is redundant and creates a TOCTOU race condition; \
+                     consider removing it and handling the error from the filesystem operation directly",
+                );
+            },
+        );
+    }
+}
+
+/// If `current` is the operand of a `?` operator (i.e. `current?`), returns the
+/// `Match` expression that the desugaring produces, so callers can keep
+/// climbing from there. `EXPR?` lowers to
+/// `Match(Call(<lang item Try::branch>, [EXPR]), _, TryDesugar(call_hir_id))`,
+/// so this is recognized structurally via `MatchSource::TryDesugar`, not by
+/// name/string matching on the call.
+fn peel_try_desugar<'tcx>(cx: &LateContext<'tcx>, current: &'tcx Expr<'tcx>) -> Option<&'tcx Expr<'tcx>> {
+    let call_expr = get_parent_expr(cx, current)?;
+    let ExprKind::Call(_, [arg]) = call_expr.kind else {
+        return None;
+    };
+    if arg.hir_id != current.hir_id {
+        return None;
+    }
+    let match_expr = get_parent_expr(cx, call_expr)?;
+    if let ExprKind::Match(_, _, MatchSource::TryDesugar(scrutinee_id)) = match_expr.kind
+        && scrutinee_id == call_expr.hir_id
+    {
+        Some(match_expr)
+    } else {
+        None
+    }
+}
+
+/// Repeatedly applies [`peel_try_desugar`], returning the outermost expression
+/// once no more `?` layers can be peeled.
+fn peel_try_desugars<'tcx>(cx: &LateContext<'tcx>, mut current: &'tcx Expr<'tcx>) -> &'tcx Expr<'tcx> {
+    while let Some(match_expr) = peel_try_desugar(cx, current) {
+        current = match_expr;
+    }
+    current
+}
+
+/// Climbs through any enclosing `&&` chain (peeling a leading `?`, e.g. from
+/// `path.try_exists()?`, first) looking for an enclosing `if` whose condition
+/// is exactly the expression we climbed to.
+fn if_then_from_condition<'tcx>(
+    cx: &LateContext<'tcx>,
+    exists_expr: &'tcx Expr<'tcx>,
+) -> Option<(&'tcx Expr<'tcx>, SyntaxContext)> {
+    let mut current = peel_try_desugars(cx, exists_expr);
+    loop {
+        let parent = get_parent_expr(cx, current)?;
+        match parent.kind {
+            ExprKind::Binary(op, lhs, rhs)
+                if op.node == BinOpKind::And && (lhs.hir_id == current.hir_id || rhs.hir_id == current.hir_id) =>
+            {
+                current = parent;
+            },
+            _ => {
+                let higher::If { cond, then, .. } = higher::If::hir(parent)?;
+                return (cond.hir_id == current.hir_id && !parent.span.from_expansion())
+                    .then(|| (then, parent.span.ctxt()));
+            },
+        }
+    }
+}
+
+/// Handles `let b = path.exists(); if b { ... }` (or the `try_exists()?`
+/// equivalent), where the `if` immediately follows the `let` in the same
+/// block.
+fn if_then_from_stored_bool<'tcx>(
+    cx: &LateContext<'tcx>,
+    exists_expr: &'tcx Expr<'tcx>,
+) -> Option<(&'tcx Expr<'tcx>, SyntaxContext)> {
+    let outer = peel_try_desugars(cx, exists_expr);
+    let Node::LetStmt(local) = cx.tcx.parent_hir_node(outer.hir_id) else {
+        return None;
+    };
+    let PatKind::Binding(_, binding_id, _, _) = local.pat.kind else {
+        return None;
+    };
+
+    let block = get_enclosing_block(cx, local.hir_id)?;
+    if block.span.from_expansion() {
+        return None;
+    }
+    let idx = block
+        .stmts
+        .iter()
+        .position(|stmt| matches!(stmt.kind, StmtKind::Let(l) if l.hir_id == local.hir_id))?;
+    let next_expr = match block.stmts.get(idx + 1) {
+        Some(stmt) => match stmt.kind {
+            StmtKind::Expr(e) | StmtKind::Semi(e) => Some(e),
+            StmtKind::Let(_) | StmtKind::Item(_) => None,
+        },
+        None => block.expr,
+    }?;
+
+    let higher::If { cond, then, .. } = higher::If::hir(next_expr)?;
+    (path_to_local_with_projections(cond) == Some(binding_id)).then(|| (then, next_expr.span.ctxt()))
+}
+
+fn is_fs_method_name(name: rustc_span::Symbol) -> bool {
+    matches!(
+        name,
+        sym::canonicalize
+            | sym::is_dir
+            | sym::is_file
+            | sym::is_symlink
+            | sym::metadata
+            | sym::read_dir
+            | sym::read_link
+            | sym::symlink_metadata
+    )
+}
+
+/// Returns `true` if `expr` is a method call that resolves to a method defined
+/// on `std::path::Path` (handles any type that derefs to `Path`, e.g. `PathBuf`).
+fn is_path_method_call(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
+    cx.typeck_results()
+        .type_dependent_def_id(expr.hir_id)
+        .is_some_and(|def_id| {
+            let parent = cx.tcx.parent(def_id);
+            cx.tcx
+                .type_of(parent)
+                .instantiate_identity()
+                .skip_norm_wip()
+                .is_diag_item(cx, sym::Path)
+        })
+}
+
+/// Searches the `then` block of the `if` for the first filesystem method call
+/// on the same receiver as the `exists()` check.
+fn find_fs_call<'tcx>(
+    cx: &LateContext<'tcx>,
+    then: &'tcx Expr<'tcx>,
+    path_recv: &'tcx Expr<'tcx>,
+    ctxt: SyntaxContext,
+) -> Option<Span> {
+    let ExprKind::Block(block, _) = then.kind else {
+        return None;
+    };
+    for stmt in block.stmts {
+        let candidate = match stmt.kind {
+            StmtKind::Expr(e) | StmtKind::Semi(e) => Some(e),
+            StmtKind::Let(local) => local.init,
+            StmtKind::Item(_) => None,
+        };
+        if let Some(span) = candidate.and_then(|e| find_fs_call_in_expr(cx, e, path_recv, ctxt)) {
+            return Some(span);
+        }
+    }
+    block.expr.and_then(|e| find_fs_call_in_expr(cx, e, path_recv, ctxt))
+}
+
+/// Peels through method chains (e.g. `.metadata().unwrap()`) and the `?` operator
+/// desugaring to find a filesystem method call on `path_recv`.
+fn find_fs_call_in_expr<'tcx>(
+    cx: &LateContext<'tcx>,
+    expr: &'tcx Expr<'tcx>,
+    path_recv: &'tcx Expr<'tcx>,
+    ctxt: SyntaxContext,
+) -> Option<Span> {
+    match expr.kind {
+        ExprKind::MethodCall(method_seg, recv, _, _) => {
+            if is_fs_method_name(method_seg.ident.name)
+                && is_path_method_call(cx, expr)
+                && SpanlessEq::new(cx).eq_expr(ctxt, recv, path_recv)
+            {
+                return Some(expr.span);
+            }
+            // Peel through chains like `.metadata().unwrap()` or `.metadata().ok()`
+            find_fs_call_in_expr(cx, recv, path_recv, ctxt)
+        },
+        // The `?` operator desugars to:
+        //   Match(Call(TryTraitBranch, [inner_expr]), ..., TryDesugar)
+        // so we extract `inner_expr` and keep searching.
+        ExprKind::Match(scrutinee, _, MatchSource::TryDesugar(_)) => {
+            if let ExprKind::Call(_, [inner_expr]) = scrutinee.kind {
+                find_fs_call_in_expr(cx, inner_expr, path_recv, ctxt)
+            } else {
+                None
+            }
+        },
+        _ => None,
+    }
+}

clippy_utils/src/sym.rs

@@ -155,6 +155,7 @@ generate! {
     build_hasher,
     by_ref,
     bytes,
+    canonicalize,
     capacity,
     cargo_clippy: "cargo-clippy",
     cast,
@@ -222,6 +223,7 @@ generate! {
     eprint_macro,
     eprintln_macro,
     err,
+    exists,
     exp,
     expect_err,
     expn_data,
@@ -375,6 +377,7 @@ generate! {
     is_diag_item,
     is_diagnostic_item,
     is_digit,
+    is_dir,
     is_empty,
     is_err,
     is_file,
@@ -385,6 +388,7 @@ generate! {
     is_some,
     is_some_and,
     is_sorted_by_key,
+    is_symlink,
     isize_legacy_const_max,
     isize_legacy_const_min,
     isize_legacy_fn_max_value,
@@ -426,6 +430,7 @@ generate! {
     mem_replace,
     mem_size_of,
     mem_size_of_val,
+    metadata,
     min,
     min_by,
     min_by_key,
@@ -496,8 +501,10 @@ generate! {
     push_str,
     range_step,
     read,
+    read_dir,
     read_exact,
     read_line,
+    read_link,
     read_to_end,
     read_to_string,
     read_unaligned,
@@ -592,6 +599,7 @@ generate! {
     subsec_nanos,
     sum,
     symbol,
+    symlink_metadata,
     take,
     take_while,
     tcx,
@@ -618,6 +626,7 @@ generate! {
     trim_start,
     trim_start_matches,
     truncate,
+    try_exists,
     try_fold,
     try_for_each,
     try_from_fn,