-
Notifications
You must be signed in to change notification settings - Fork 1.7k
new lint: zombie_processes
#11476
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
new lint: zombie_processes
#11476
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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(()), | ||
| } | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.