|
| 1 | +use clippy_utils::diagnostics::span_lint_and_then; |
| 2 | +use clippy_utils::{fn_def_id, get_enclosing_block, match_any_def_paths, match_def_path, path_to_local_id, paths}; |
| 3 | +use rustc_ast::Mutability; |
| 4 | +use rustc_errors::Applicability; |
| 5 | +use rustc_hir::intravisit::{walk_block, walk_expr, walk_local, Visitor}; |
| 6 | +use rustc_hir::{Expr, ExprKind, HirId, LetStmt, Node, PatKind, Stmt, StmtKind}; |
| 7 | +use rustc_lint::{LateContext, LateLintPass}; |
| 8 | +use rustc_session::declare_lint_pass; |
| 9 | +use rustc_span::sym; |
| 10 | +use std::ops::ControlFlow; |
| 11 | +use ControlFlow::{Break, Continue}; |
| 12 | + |
| 13 | +declare_clippy_lint! { |
| 14 | + /// ### What it does |
| 15 | + /// Looks for code that spawns a process but never calls `wait()` on the child. |
| 16 | + /// |
| 17 | + /// ### Why is this bad? |
| 18 | + /// As explained in the [standard library documentation](https://doc.rust-lang.org/stable/std/process/struct.Child.html#warning), |
| 19 | + /// calling `wait()` is necessary on Unix platforms to properly release all OS resources associated with the process. |
| 20 | + /// Not doing so will effectively leak process IDs and/or other limited global resources, |
| 21 | + /// which can eventually lead to resource exhaustion, so it's recommended to call `wait()` in long-running applications. |
| 22 | + /// Such processes are called "zombie processes". |
| 23 | + /// |
| 24 | + /// ### Example |
| 25 | + /// ```rust |
| 26 | + /// use std::process::Command; |
| 27 | + /// |
| 28 | + /// let _child = Command::new("ls").spawn().expect("failed to execute child"); |
| 29 | + /// ``` |
| 30 | + /// Use instead: |
| 31 | + /// ```rust |
| 32 | + /// use std::process::Command; |
| 33 | + /// |
| 34 | + /// let mut child = Command::new("ls").spawn().expect("failed to execute child"); |
| 35 | + /// child.wait().expect("failed to wait on child"); |
| 36 | + /// ``` |
| 37 | + #[clippy::version = "1.74.0"] |
| 38 | + pub ZOMBIE_PROCESSES, |
| 39 | + suspicious, |
| 40 | + "not waiting on a spawned child process" |
| 41 | +} |
| 42 | +declare_lint_pass!(ZombieProcesses => [ZOMBIE_PROCESSES]); |
| 43 | + |
| 44 | +impl<'tcx> LateLintPass<'tcx> for ZombieProcesses { |
| 45 | + fn check_expr(&mut self, cx: &LateContext<'tcx>, expr: &'tcx Expr<'tcx>) { |
| 46 | + if let ExprKind::Call(..) | ExprKind::MethodCall(..) = expr.kind |
| 47 | + && let Some(child_adt) = cx.typeck_results().expr_ty(expr).ty_adt_def() |
| 48 | + && match_def_path(cx, child_adt.did(), &paths::CHILD) |
| 49 | + { |
| 50 | + match cx.tcx.parent_hir_node(expr.hir_id) { |
| 51 | + Node::LetStmt(local) |
| 52 | + if let PatKind::Binding(_, local_id, ..) = local.pat.kind |
| 53 | + && let Some(enclosing_block) = get_enclosing_block(cx, expr.hir_id) => |
| 54 | + { |
| 55 | + let mut vis = WaitFinder::WalkUpTo(cx, local_id); |
| 56 | + |
| 57 | + // If it does have a `wait()` call, we're done. Don't lint. |
| 58 | + if let Break(BreakReason::WaitFound) = walk_block(&mut vis, enclosing_block) { |
| 59 | + return; |
| 60 | + } |
| 61 | + |
| 62 | + // Don't emit a suggestion since the binding is used later |
| 63 | + check(cx, expr, false); |
| 64 | + }, |
| 65 | + Node::LetStmt(&LetStmt { pat, .. }) if let PatKind::Wild = pat.kind => { |
| 66 | + // `let _ = child;`, also dropped immediately without `wait()`ing |
| 67 | + check(cx, expr, true); |
| 68 | + }, |
| 69 | + Node::Stmt(&Stmt { |
| 70 | + kind: StmtKind::Semi(_), |
| 71 | + .. |
| 72 | + }) => { |
| 73 | + // Immediately dropped. E.g. `std::process::Command::new("echo").spawn().unwrap();` |
| 74 | + check(cx, expr, true); |
| 75 | + }, |
| 76 | + _ => {}, |
| 77 | + } |
| 78 | + } |
| 79 | + } |
| 80 | +} |
| 81 | + |
| 82 | +enum BreakReason { |
| 83 | + WaitFound, |
| 84 | + EarlyReturn, |
| 85 | +} |
| 86 | + |
| 87 | +/// A visitor responsible for finding a `wait()` call on a local variable. |
| 88 | +/// |
| 89 | +/// Conditional `wait()` calls are assumed to not call wait: |
| 90 | +/// ```ignore |
| 91 | +/// let mut c = Command::new("").spawn().unwrap(); |
| 92 | +/// if true { |
| 93 | +/// c.wait(); |
| 94 | +/// } |
| 95 | +/// ``` |
| 96 | +/// |
| 97 | +/// Note that this visitor does NOT explicitly look for `wait()` calls directly, but rather does the |
| 98 | +/// inverse -- checking if all uses of the local are either: |
| 99 | +/// - a field access (`child.{stderr,stdin,stdout}`) |
| 100 | +/// - calling `id` or `kill` |
| 101 | +/// - no use at all (e.g. `let _x = child;`) |
| 102 | +/// - taking a shared reference (`&`), `wait()` can't go through that |
| 103 | +/// |
| 104 | +/// None of these are sufficient to prevent zombie processes. |
| 105 | +/// Doing it like this means more FNs, but FNs are better than FPs. |
| 106 | +/// |
| 107 | +/// `return` expressions, conditional or not, short-circuit the visitor because |
| 108 | +/// if a `wait()` call hadn't been found at that point, it might never reach one at a later point: |
| 109 | +/// ```ignore |
| 110 | +/// let mut c = Command::new("").spawn().unwrap(); |
| 111 | +/// if true { |
| 112 | +/// return; // Break(BreakReason::EarlyReturn) |
| 113 | +/// } |
| 114 | +/// c.wait(); // this might not be reachable |
| 115 | +/// ``` |
| 116 | +enum WaitFinder<'a, 'tcx> { |
| 117 | + WalkUpTo(&'a LateContext<'tcx>, HirId), |
| 118 | + Found(&'a LateContext<'tcx>, HirId), |
| 119 | +} |
| 120 | + |
| 121 | +impl<'a, 'tcx> Visitor<'tcx> for WaitFinder<'a, 'tcx> { |
| 122 | + type Result = ControlFlow<BreakReason>; |
| 123 | + |
| 124 | + fn visit_local(&mut self, l: &'tcx LetStmt<'tcx>) -> Self::Result { |
| 125 | + if let Self::WalkUpTo(cx, local_id) = *self |
| 126 | + && let PatKind::Binding(_, pat_id, ..) = l.pat.kind |
| 127 | + && local_id == pat_id |
| 128 | + { |
| 129 | + *self = Self::Found(cx, local_id); |
| 130 | + } |
| 131 | + |
| 132 | + walk_local(self, l) |
| 133 | + } |
| 134 | + |
| 135 | + fn visit_expr(&mut self, ex: &'tcx Expr<'tcx>) -> Self::Result { |
| 136 | + let Self::Found(cx, local_id) = *self else { |
| 137 | + return walk_expr(self, ex); |
| 138 | + }; |
| 139 | + |
| 140 | + if path_to_local_id(ex, local_id) { |
| 141 | + match cx.tcx.parent_hir_node(ex.hir_id) { |
| 142 | + Node::Stmt(Stmt { |
| 143 | + kind: StmtKind::Semi(_), |
| 144 | + .. |
| 145 | + }) => {}, |
| 146 | + Node::Expr(expr) if let ExprKind::Field(..) = expr.kind => {}, |
| 147 | + Node::Expr(expr) if let ExprKind::AddrOf(_, Mutability::Not, _) = expr.kind => {}, |
| 148 | + Node::Expr(expr) |
| 149 | + if let Some(fn_did) = fn_def_id(cx, expr) |
| 150 | + && match_any_def_paths(cx, fn_did, &[&paths::CHILD_ID, &paths::CHILD_KILL]).is_some() => {}, |
| 151 | + |
| 152 | + // Conservatively assume that all other kinds of nodes call `.wait()` somehow. |
| 153 | + _ => return Break(BreakReason::WaitFound), |
| 154 | + } |
| 155 | + } else { |
| 156 | + match ex.kind { |
| 157 | + ExprKind::Ret(..) => return Break(BreakReason::EarlyReturn), |
| 158 | + ExprKind::If(cond, then, None) => { |
| 159 | + walk_expr(self, cond)?; |
| 160 | + |
| 161 | + // A `wait()` call in an if expression with no else is not enough: |
| 162 | + // |
| 163 | + // let c = spawn(); |
| 164 | + // if true { |
| 165 | + // c.wait(); |
| 166 | + // } |
| 167 | + // |
| 168 | + // This might not call wait(). However, early returns are propagated, |
| 169 | + // because they might lead to a later wait() not being called. |
| 170 | + if let Break(BreakReason::EarlyReturn) = walk_expr(self, then) { |
| 171 | + return Break(BreakReason::EarlyReturn); |
| 172 | + } |
| 173 | + |
| 174 | + return Continue(()); |
| 175 | + }, |
| 176 | + |
| 177 | + ExprKind::If(cond, then, Some(else_)) => { |
| 178 | + walk_expr(self, cond)?; |
| 179 | + |
| 180 | + #[expect(clippy::unnested_or_patterns)] |
| 181 | + match (walk_expr(self, then), walk_expr(self, else_)) { |
| 182 | + (Continue(()), Continue(())) |
| 183 | + |
| 184 | + // `wait()` in one branch but nothing in the other does not count |
| 185 | + | (Continue(()), Break(BreakReason::WaitFound)) |
| 186 | + | (Break(BreakReason::WaitFound), Continue(())) => {}, |
| 187 | + |
| 188 | + // `wait()` in both branches is ok |
| 189 | + (Break(BreakReason::WaitFound), Break(BreakReason::WaitFound)) => { |
| 190 | + return Break(BreakReason::WaitFound); |
| 191 | + }, |
| 192 | + |
| 193 | + // Propagate early returns in either branch |
| 194 | + (Break(BreakReason::EarlyReturn), _) | (_, Break(BreakReason::EarlyReturn)) => { |
| 195 | + return Break(BreakReason::EarlyReturn); |
| 196 | + }, |
| 197 | + } |
| 198 | + |
| 199 | + return Continue(()); |
| 200 | + }, |
| 201 | + _ => {}, |
| 202 | + } |
| 203 | + } |
| 204 | + |
| 205 | + walk_expr(self, ex) |
| 206 | + } |
| 207 | +} |
| 208 | + |
| 209 | +/// This function has shared logic between the different kinds of nodes that can trigger the lint. |
| 210 | +/// |
| 211 | +/// In particular, `let <binding> = <expr that spawns child>;` requires some custom additional logic |
| 212 | +/// such as checking that the binding is not used in certain ways, which isn't necessary for |
| 213 | +/// `let _ = <expr that spawns child>;`. |
| 214 | +/// |
| 215 | +/// This checks if the program doesn't unconditionally exit after the spawn expression. |
| 216 | +fn check<'tcx>(cx: &LateContext<'tcx>, spawn_expr: &'tcx Expr<'tcx>, emit_suggestion: bool) { |
| 217 | + let Some(block) = get_enclosing_block(cx, spawn_expr.hir_id) else { |
| 218 | + return; |
| 219 | + }; |
| 220 | + |
| 221 | + let mut vis = ExitPointFinder { |
| 222 | + cx, |
| 223 | + state: ExitPointState::WalkUpTo(spawn_expr.hir_id), |
| 224 | + }; |
| 225 | + if let Break(ExitCallFound) = vis.visit_block(block) { |
| 226 | + // Visitor found an unconditional `exit()` call, so don't lint. |
| 227 | + return; |
| 228 | + } |
| 229 | + |
| 230 | + span_lint_and_then( |
| 231 | + cx, |
| 232 | + ZOMBIE_PROCESSES, |
| 233 | + spawn_expr.span, |
| 234 | + "spawned process is never `wait()`ed on", |
| 235 | + |diag| { |
| 236 | + if emit_suggestion { |
| 237 | + diag.span_suggestion( |
| 238 | + spawn_expr.span.shrink_to_hi(), |
| 239 | + "try", |
| 240 | + ".wait()", |
| 241 | + Applicability::MaybeIncorrect, |
| 242 | + ); |
| 243 | + } else { |
| 244 | + diag.note("consider calling `.wait()`"); |
| 245 | + } |
| 246 | + |
| 247 | + diag.note("not doing so might leave behind zombie processes") |
| 248 | + .note("see https://doc.rust-lang.org/stable/std/process/struct.Child.html#warning"); |
| 249 | + }, |
| 250 | + ); |
| 251 | +} |
| 252 | + |
| 253 | +/// Checks if the given expression exits the process. |
| 254 | +fn is_exit_expression(cx: &LateContext<'_>, expr: &Expr<'_>) -> bool { |
| 255 | + fn_def_id(cx, expr).is_some_and(|fn_did| { |
| 256 | + cx.tcx.is_diagnostic_item(sym::process_exit, fn_did) || match_def_path(cx, fn_did, &paths::ABORT) |
| 257 | + }) |
| 258 | +} |
| 259 | + |
| 260 | +#[derive(Debug)] |
| 261 | +enum ExitPointState { |
| 262 | + /// Still walking up to the expression that initiated the visitor. |
| 263 | + WalkUpTo(HirId), |
| 264 | + /// We're inside of a control flow construct (e.g. `if`, `match`, `loop`) |
| 265 | + /// Within this, we shouldn't accept any `exit()` calls in here, but we can leave all of these |
| 266 | + /// constructs later and still continue looking for an `exit()` call afterwards. Example: |
| 267 | + /// ```ignore |
| 268 | + /// Command::new("").spawn().unwrap(); |
| 269 | + /// |
| 270 | + /// if true { // depth=1 |
| 271 | + /// if true { // depth=2 |
| 272 | + /// match () { // depth=3 |
| 273 | + /// () => loop { // depth=4 |
| 274 | + /// |
| 275 | + /// std::process::exit(); |
| 276 | + /// ^^^^^^^^^^^^^^^^^^^^^ conditional exit call, ignored |
| 277 | + /// |
| 278 | + /// } // depth=3 |
| 279 | + /// } // depth=2 |
| 280 | + /// } // depth=1 |
| 281 | + /// } // depth=0 |
| 282 | + /// |
| 283 | + /// std::process::exit(); |
| 284 | + /// ^^^^^^^^^^^^^^^^^^^^^ this exit call is accepted because we're now unconditionally calling it |
| 285 | + /// ``` |
| 286 | + /// We can only get into this state from `NoExit`. |
| 287 | + InControlFlow { depth: u32 }, |
| 288 | + /// No exit call found yet, but looking for one. |
| 289 | + NoExit, |
| 290 | +} |
| 291 | + |
| 292 | +fn expr_enters_control_flow(expr: &Expr<'_>) -> bool { |
| 293 | + matches!(expr.kind, ExprKind::If(..) | ExprKind::Match(..) | ExprKind::Loop(..)) |
| 294 | +} |
| 295 | + |
| 296 | +struct ExitPointFinder<'a, 'tcx> { |
| 297 | + state: ExitPointState, |
| 298 | + cx: &'a LateContext<'tcx>, |
| 299 | +} |
| 300 | + |
| 301 | +struct ExitCallFound; |
| 302 | + |
| 303 | +impl<'a, 'tcx> Visitor<'tcx> for ExitPointFinder<'a, 'tcx> { |
| 304 | + type Result = ControlFlow<ExitCallFound>; |
| 305 | + |
| 306 | + fn visit_expr(&mut self, expr: &'tcx Expr<'tcx>) -> Self::Result { |
| 307 | + match self.state { |
| 308 | + ExitPointState::WalkUpTo(id) if expr.hir_id == id => { |
| 309 | + self.state = ExitPointState::NoExit; |
| 310 | + walk_expr(self, expr) |
| 311 | + }, |
| 312 | + ExitPointState::NoExit if expr_enters_control_flow(expr) => { |
| 313 | + self.state = ExitPointState::InControlFlow { depth: 1 }; |
| 314 | + walk_expr(self, expr)?; |
| 315 | + if let ExitPointState::InControlFlow { .. } = self.state { |
| 316 | + self.state = ExitPointState::NoExit; |
| 317 | + } |
| 318 | + Continue(()) |
| 319 | + }, |
| 320 | + ExitPointState::NoExit if is_exit_expression(self.cx, expr) => Break(ExitCallFound), |
| 321 | + ExitPointState::InControlFlow { ref mut depth } if expr_enters_control_flow(expr) => { |
| 322 | + *depth += 1; |
| 323 | + walk_expr(self, expr)?; |
| 324 | + match self.state { |
| 325 | + ExitPointState::InControlFlow { depth: 1 } => self.state = ExitPointState::NoExit, |
| 326 | + ExitPointState::InControlFlow { ref mut depth } => *depth -= 1, |
| 327 | + _ => {}, |
| 328 | + } |
| 329 | + Continue(()) |
| 330 | + }, |
| 331 | + _ => Continue(()), |
| 332 | + } |
| 333 | + } |
| 334 | +} |
0 commit comments