new lint: zombie_processes

CHANGELOG.md

@@ -6048,6 +6048,7 @@ Released 2018-09-13
 [`zero_repeat_side_effects`]: https://rust-lang.github.io/rust-clippy/master/index.html#zero_repeat_side_effects
 [`zero_sized_map_values`]: https://rust-lang.github.io/rust-clippy/master/index.html#zero_sized_map_values
 [`zero_width_space`]: https://rust-lang.github.io/rust-clippy/master/index.html#zero_width_space
+[`zombie_processes`]: https://rust-lang.github.io/rust-clippy/master/index.html#zombie_processes
 [`zst_offset`]: https://rust-lang.github.io/rust-clippy/master/index.html#zst_offset
 <!-- end autogenerated links to lint list -->
 <!-- begin autogenerated links to configuration documentation -->

clippy_dev/src/serve.rs

@@ -29,7 +29,7 @@ pub fn run(port: u16, lint: Option<String>) -> ! {
         }
         if let Some(url) = url.take() {
             thread::spawn(move || {
-                Command::new(PYTHON)
+                let mut child = Command::new(PYTHON)
                     .arg("-m")
                     .arg("http.server")
                     .arg(port.to_string())
@@ -40,6 +40,7 @@ pub fn run(port: u16, lint: Option<String>) -> ! {
                 thread::sleep(Duration::from_millis(500));
                 // Launch browser after first export.py has completed and http.server is up
                 let _result = opener::open(url);
+                child.wait().unwrap();
             });
         }
         thread::sleep(Duration::from_millis(1000));

clippy_lints/src/declared_lints.rs

@@ -768,4 +768,5 @@ pub static LINTS: &[&crate::LintInfo] = &[
     crate::zero_div_zero::ZERO_DIVIDED_BY_ZERO_INFO,
     crate::zero_repeat_side_effects::ZERO_REPEAT_SIDE_EFFECTS_INFO,
     crate::zero_sized_map_values::ZERO_SIZED_MAP_VALUES_INFO,
+    crate::zombie_processes::ZOMBIE_PROCESSES_INFO,
 ];

clippy_lints/src/lib.rs

@@ -386,6 +386,7 @@ mod write;
 mod zero_div_zero;
 mod zero_repeat_side_effects;
 mod zero_sized_map_values;
+mod zombie_processes;
 // end lints modules, do not remove this comment, it’s used in `update_lints`
 
 use clippy_config::{get_configuration_metadata, Conf};
@@ -933,5 +934,6 @@ pub fn register_lints(store: &mut rustc_lint::LintStore, conf: &'static Conf) {
     store.register_late_pass(|_| Box::new(set_contains_or_insert::SetContainsOrInsert));
     store.register_early_pass(|| Box::new(byte_char_slices::ByteCharSlice));
     store.register_early_pass(|| Box::new(cfg_not_test::CfgNotTest));
+    store.register_late_pass(|_| Box::new(zombie_processes::ZombieProcesses));
     // add lints here, do not remove this comment, it's used in `new_lint`
 }

clippy_lints/src/zombie_processes.rs

@@ -0,0 +1,334 @@
+use clippy_utils::diagnostics::span_lint_and_then;
+use clippy_utils::{fn_def_id, get_enclosing_block, match_any_def_paths, match_def_path, path_to_local_id, paths};
+use rustc_ast::Mutability;
+use rustc_errors::Applicability;
+use rustc_hir::intravisit::{walk_block, walk_expr, walk_local, Visitor};
+use rustc_hir::{Expr, ExprKind, HirId, LetStmt, Node, PatKind, Stmt, StmtKind};
+use rustc_lint::{LateContext, LateLintPass};
+use rustc_session::declare_lint_pass;
+use rustc_span::sym;
+use std::ops::ControlFlow;
+use ControlFlow::{Break, Continue};
+
+declare_clippy_lint! {
+    /// ### What it does
+    /// Looks for code that spawns a process but never calls `wait()` on the child.
+    ///
+    /// ### Why is this bad?
+    /// As explained in the [standard library documentation](https://doc.rust-lang.org/stable/std/process/struct.Child.html#warning),
+    /// calling `wait()` is necessary on Unix platforms to properly release all OS resources associated with the process.
+    /// Not doing so will effectively leak process IDs and/or other limited global resources,
+    /// which can eventually lead to resource exhaustion, so it's recommended to call `wait()` in long-running applications.
+    /// Such processes are called "zombie processes".
+    ///
+    /// ### Example
+    /// ```rust
+    /// use std::process::Command;
+    ///
+    /// let _child = Command::new("ls").spawn().expect("failed to execute child");
+    /// ```
+    /// Use instead:
+    /// ```rust
+    /// use std::process::Command;
+    ///
+    /// let mut child = Command::new("ls").spawn().expect("failed to execute child");
+    /// child.wait().expect("failed to wait on child");
+    /// ```
+    #[clippy::version = "1.74.0"]
+    pub ZOMBIE_PROCESSES,
+    suspicious,
+    "not waiting on a spawned child process"
+}
+declare_lint_pass!(ZombieProcesses => [ZOMBIE_PROCESSES]);
+
+impl<'tcx> LateLintPass<'tcx> for ZombieProcesses {
+    fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) {
+        if let ExprKind::Call(..) | ExprKind::MethodCall(..) = expr.kind
+            && let Some(child_adt) = cx.typeck_results().expr_ty(expr).ty_adt_def()
+            && match_def_path(cx, child_adt.did(), &paths::CHILD)
+        {
+            match cx.tcx.parent_hir_node(expr.hir_id) {
+                Node::LetStmt(local)
+                    if let PatKind::Binding(_, local_id, ..) = local.pat.kind
+                        && let Some(enclosing_block) = get_enclosing_block(cx, expr.hir_id) =>
+                {
+                    let mut vis = WaitFinder::WalkUpTo(cx, local_id);
+
+                    // If it does have a `wait()` call, we're done. Don't lint.
+                    if let Break(BreakReason::WaitFound) = walk_block(&mut vis, enclosing_block) {
+                        return;
+                    }
+
+                    // Don't emit a suggestion since the binding is used later
+                    check(cx, expr, false);
+                },
+                Node::LetStmt(&LetStmt { pat, .. }) if let PatKind::Wild = pat.kind => {
+                    // `let _ = child;`, also dropped immediately without `wait()`ing
+                    check(cx, expr, true);
+                },
+                Node::Stmt(&Stmt {
+                    kind: StmtKind::Semi(_),
+                    ..
+                }) => {
+                    // Immediately dropped. E.g. `std::process::Command::new("echo").spawn().unwrap();`
+                    check(cx, expr, true);
+                },
+                _ => {},
+            }
+        }
+    }
+}
+
+enum BreakReason {
+    WaitFound,
+    EarlyReturn,
+}
+
+/// A visitor responsible for finding a `wait()` call on a local variable.
+///
+/// Conditional `wait()` calls are assumed to not call wait:
+/// ```ignore
+/// let mut c = Command::new("").spawn().unwrap();
+/// if true {
+///     c.wait();
+/// }
+/// ```
+///
+/// Note that this visitor does NOT explicitly look for `wait()` calls directly, but rather does the
+/// inverse -- checking if all uses of the local are either:
+/// - a field access (`child.{stderr,stdin,stdout}`)
+/// - calling `id` or `kill`
+/// - no use at all (e.g. `let _x = child;`)
+/// - taking a shared reference (`&`), `wait()` can't go through that
+///
+/// None of these are sufficient to prevent zombie processes.
+/// Doing it like this means more FNs, but FNs are better than FPs.
+///
+/// `return` expressions, conditional or not, short-circuit the visitor because
+/// if a `wait()` call hadn't been found at that point, it might never reach one at a later point:
+/// ```ignore
+/// let mut c = Command::new("").spawn().unwrap();
+/// if true {
+///     return; // Break(BreakReason::EarlyReturn)
+/// }
+/// c.wait(); // this might not be reachable
+/// ```
+enum WaitFinder<'a, 'tcx> {
+    WalkUpTo(&'a LateContext<'tcx>, HirId),
+    Found(&'a LateContext<'tcx>, HirId),
+}
+
+impl<'a, 'tcx> Visitor<'tcx> for WaitFinder<'a, 'tcx> {
+    type Result = ControlFlow<BreakReason>;
+
+    fn visit_local(&mut self, l: &'tcx LetStmt<'tcx>) -> Self::Result {
+        if let Self::WalkUpTo(cx, local_id) = *self
+            && let PatKind::Binding(_, pat_id, ..) = l.pat.kind
+            && local_id == pat_id
+        {
+            *self = Self::Found(cx, local_id);
+        }
+
+        walk_local(self, l)
+    }
+
+    fn visit_expr(&mut self, ex: &'tcx Expr<'tcx>) -> Self::Result {
+        let Self::Found(cx, local_id) = *self else {
+            return walk_expr(self, ex);
+        };
+
+        if path_to_local_id(ex, local_id) {
+            match cx.tcx.parent_hir_node(ex.hir_id) {
+                Node::Stmt(Stmt {
+                    kind: StmtKind::Semi(_),
+                    ..
+                }) => {},
+                Node::Expr(expr) if let ExprKind::Field(..) = expr.kind => {},
+                Node::Expr(expr) if let ExprKind::AddrOf(_, Mutability::Not, _) = expr.kind => {},
+                Node::Expr(expr)
+                    if let Some(fn_did) = fn_def_id(cx, expr)
+                        && match_any_def_paths(cx, fn_did, &[&paths::CHILD_ID, &paths::CHILD_KILL]).is_some() => {},
+
+                // Conservatively assume that all other kinds of nodes call `.wait()` somehow.
+                _ => return Break(BreakReason::WaitFound),
+            }
+        } else {
+            match ex.kind {
+                ExprKind::Ret(..) => return Break(BreakReason::EarlyReturn),
+                ExprKind::If(cond, then, None) => {
+                    walk_expr(self, cond)?;
+
+                    // A `wait()` call in an if expression with no else is not enough:
+                    //
+                    // let c = spawn();
+                    // if true {
+                    //   c.wait();
+                    // }
+                    //
+                    // This might not call wait(). However, early returns are propagated,
+                    // because they might lead to a later wait() not being called.
+                    if let Break(BreakReason::EarlyReturn) = walk_expr(self, then) {
+                        return Break(BreakReason::EarlyReturn);
+                    }
+
+                    return Continue(());
+                },
+
+                ExprKind::If(cond, then, Some(else_)) => {
+                    walk_expr(self, cond)?;
+
+                    #[expect(clippy::unnested_or_patterns)]
+                    match (walk_expr(self, then), walk_expr(self, else_)) {
+                        (Continue(()), Continue(()))
+
+                        // `wait()` in one branch but nothing in the other does not count
+                        | (Continue(()), Break(BreakReason::WaitFound))
+                        | (Break(BreakReason::WaitFound), Continue(())) => {},
+
+                        // `wait()` in both branches is ok
+                        (Break(BreakReason::WaitFound), Break(BreakReason::WaitFound)) => {
+                            return Break(BreakReason::WaitFound);
+                        },
+
+                        // Propagate early returns in either branch
+                        (Break(BreakReason::EarlyReturn), _) | (_, Break(BreakReason::EarlyReturn)) => {
+                            return Break(BreakReason::EarlyReturn);
+                        },
+                    }
+
+                    return Continue(());
+                },
+                _ => {},
+            }
+        }
+
+        walk_expr(self, ex)
+    }
+}
+
+/// This function has shared logic between the different kinds of nodes that can trigger the lint.
+///
+/// In particular, `let <binding> = <expr that spawns child>;` requires some custom additional logic
+/// such as checking that the binding is not used in certain ways, which isn't necessary for
+/// `let _ = <expr that spawns child>;`.
+///
+/// This checks if the program doesn't unconditionally exit after the spawn expression.
+fn check<'tcx>(cx: &LateContext<'tcx>, spawn_expr: &'tcx Expr<'tcx>, emit_suggestion: bool) {
+    let Some(block) = get_enclosing_block(cx, spawn_expr.hir_id) else {
+        return;
+    };
+
+    let mut vis = ExitPointFinder {
+        cx,
+        state: ExitPointState::WalkUpTo(spawn_expr.hir_id),
+    };
+    if let Break(ExitCallFound) = vis.visit_block(block) {
+        // Visitor found an unconditional `exit()` call, so don't lint.
+        return;
+    }
+
+    span_lint_and_then(
+        cx,
+        ZOMBIE_PROCESSES,
+        spawn_expr.span,
+        "spawned process is never `wait()`ed on",
+        |diag| {
+            if emit_suggestion {
+                diag.span_suggestion(
+                    spawn_expr.span.shrink_to_hi(),
+                    "try",
+                    ".wait()",
+                    Applicability::MaybeIncorrect,
+                );
+            } else {
+                diag.note("consider calling `.wait()`");
+            }
+
+            diag.note("not doing so might leave behind zombie processes")
+                .note("see https://doc.rust-lang.org/stable/std/process/struct.Child.html#warning");
+        },
+    );
+}
+
+/// Checks if the given expression exits the process.
+fn is_exit_expression(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool {
+    fn_def_id(cx, expr).is_some_and(|fn_did| {
+        cx.tcx.is_diagnostic_item(sym::process_exit, fn_did) || match_def_path(cx, fn_did, &paths::ABORT)
+    })
+}
+
+#[derive(Debug)]
+enum ExitPointState {
+    /// Still walking up to the expression that initiated the visitor.
+    WalkUpTo(HirId),
+    /// We're inside of a control flow construct (e.g. `if`, `match`, `loop`)
+    /// Within this, we shouldn't accept any `exit()` calls in here, but we can leave all of these
+    /// constructs later and still continue looking for an `exit()` call afterwards. Example:
+    /// ```ignore
+    /// Command::new("").spawn().unwrap();
+    ///
+    /// if true {                // depth=1
+    ///     if true {            // depth=2
+    ///         match () {       // depth=3
+    ///             () => loop { // depth=4
+    ///
+    ///                 std::process::exit();
+    ///                 ^^^^^^^^^^^^^^^^^^^^^ conditional exit call, ignored
+    ///
+    ///             }           // depth=3
+    ///         }               // depth=2
+    ///     }                   // depth=1
+    /// }                       // depth=0
+    ///
+    /// std::process::exit();
+    /// ^^^^^^^^^^^^^^^^^^^^^ this exit call is accepted because we're now unconditionally calling it
+    /// ```
+    /// We can only get into this state from `NoExit`.
+    InControlFlow { depth: u32 },
+    /// No exit call found yet, but looking for one.
+    NoExit,
+}
+
+fn expr_enters_control_flow(expr: &Expr<'_>) -> bool {
+    matches!(expr.kind, ExprKind::If(..) | ExprKind::Match(..) | ExprKind::Loop(..))
+}
+
+struct ExitPointFinder<'a, 'tcx> {
+    state: ExitPointState,
+    cx: &'a LateContext<'tcx>,
+}
+
+struct ExitCallFound;
+
+impl<'a, 'tcx> Visitor<'tcx> for ExitPointFinder<'a, 'tcx> {
+    type Result = ControlFlow<ExitCallFound>;
+
+    fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) -> Self::Result {
+        match self.state {
+            ExitPointState::WalkUpTo(id) if expr.hir_id == id => {
+                self.state = ExitPointState::NoExit;
+                walk_expr(self, expr)
+            },
+            ExitPointState::NoExit if expr_enters_control_flow(expr) => {
+                self.state = ExitPointState::InControlFlow { depth: 1 };
+                walk_expr(self, expr)?;
+                if let ExitPointState::InControlFlow { .. } = self.state {
+                    self.state = ExitPointState::NoExit;
+                }
+                Continue(())
+            },
+            ExitPointState::NoExit if is_exit_expression(self.cx, expr) => Break(ExitCallFound),
+            ExitPointState::InControlFlow { ref mut depth } if expr_enters_control_flow(expr) => {
+                *depth += 1;
+                walk_expr(self, expr)?;
+                match self.state {
+                    ExitPointState::InControlFlow { depth: 1 } => self.state = ExitPointState::NoExit,
+                    ExitPointState::InControlFlow { ref mut depth } => *depth -= 1,
+                    _ => {},
+                }
+                Continue(())
+            },
+            _ => Continue(()),
+        }
+    }
+}

clippy_utils/src/paths.rs

@@ -4,6 +4,7 @@
 //! Whenever possible, please consider diagnostic items over hardcoded paths.
 //! See <https://github.com/rust-lang/rust-clippy/issues/5393> for more information.
 
+pub const ABORT: [&str; 3] = ["std", "process", "abort"];
 pub const APPLICABILITY: [&str; 2] = ["rustc_lint_defs", "Applicability"];
 pub const APPLICABILITY_VALUES: [[&str; 3]; 4] = [
     ["rustc_lint_defs", "Applicability", "Unspecified"],
@@ -23,6 +24,9 @@ pub const CORE_RESULT_OK_METHOD: [&str; 4] = ["core", "result", "Result", "ok"];
 pub const CSTRING_AS_C_STR: [&str; 5] = ["alloc", "ffi", "c_str", "CString", "as_c_str"];
 pub const EARLY_CONTEXT: [&str; 2] = ["rustc_lint", "EarlyContext"];
 pub const EARLY_LINT_PASS: [&str; 3] = ["rustc_lint", "passes", "EarlyLintPass"];
+pub const CHILD: [&str; 3] = ["std", "process", "Child"];
+pub const CHILD_ID: [&str; 4] = ["std", "process", "Child", "id"];
+pub const CHILD_KILL: [&str; 4] = ["std", "process", "Child", "kill"];
 pub const F32_EPSILON: [&str; 4] = ["core", "f32", "<impl f32>", "EPSILON"];
 pub const F64_EPSILON: [&str; 4] = ["core", "f64", "<impl f64>", "EPSILON"];
 pub const FILE_OPTIONS: [&str; 4] = ["std", "fs", "File", "options"];