diff --git a/crates/ruff/resources/test/fixtures/pylint/subprocess_popen_preexec_fn.py b/crates/ruff/resources/test/fixtures/pylint/subprocess_popen_preexec_fn.py new file mode 100644 index 0000000000..547223a7c2 --- /dev/null +++ b/crates/ruff/resources/test/fixtures/pylint/subprocess_popen_preexec_fn.py @@ -0,0 +1,18 @@ +import subprocess + + +def foo(): + pass + + +# Errors. +subprocess.Popen(preexec_fn=foo) +subprocess.Popen(["ls"], preexec_fn=foo) +subprocess.Popen(preexec_fn=lambda: print("Hello, world!")) +subprocess.Popen(["ls"], preexec_fn=lambda: print("Hello, world!")) + +# Non-errors. +subprocess.Popen() +subprocess.Popen(["ls"]) +subprocess.Popen(preexec_fn=None) # None is the default. +subprocess.Popen(["ls"], preexec_fn=None) # None is the default. diff --git a/crates/ruff/src/checkers/ast/mod.rs b/crates/ruff/src/checkers/ast/mod.rs index 50c411a8b9..b862b56b70 100644 --- a/crates/ruff/src/checkers/ast/mod.rs +++ b/crates/ruff/src/checkers/ast/mod.rs @@ -2961,6 +2961,9 @@ where self.diagnostics.push(diagnostic); } } + if self.enabled(Rule::SubprocessPopenPreexecFn) { + pylint::rules::subprocess_popen_preexec_fn(self, func, keywords); + } if self.any_enabled(&[ Rule::PytestRaisesWithoutException, Rule::PytestRaisesTooBroad, diff --git a/crates/ruff/src/codes.rs b/crates/ruff/src/codes.rs index 51d447ccd8..f9d1de032d 100644 --- a/crates/ruff/src/codes.rs +++ b/crates/ruff/src/codes.rs @@ -172,10 +172,10 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Pylint, "C0131") => (RuleGroup::Unspecified, rules::pylint::rules::TypeBivariance), (Pylint, "C0132") => (RuleGroup::Unspecified, rules::pylint::rules::TypeParamNameMismatch), (Pylint, "C0205") => (RuleGroup::Unspecified, rules::pylint::rules::SingleStringSlots), + (Pylint, "C0208") => (RuleGroup::Unspecified, rules::pylint::rules::IterationOverSet), (Pylint, "C0414") => (RuleGroup::Unspecified, rules::pylint::rules::UselessImportAlias), (Pylint, "C1901") => (RuleGroup::Nursery, rules::pylint::rules::CompareToEmptyString), (Pylint, "C3002") => (RuleGroup::Unspecified, rules::pylint::rules::UnnecessaryDirectLambdaCall), - (Pylint, "C0208") => (RuleGroup::Unspecified, rules::pylint::rules::IterationOverSet), (Pylint, "E0100") => (RuleGroup::Unspecified, rules::pylint::rules::YieldInInit), (Pylint, "E0101") => (RuleGroup::Unspecified, rules::pylint::rules::ReturnInInit), (Pylint, "E0116") => (RuleGroup::Unspecified, rules::pylint::rules::ContinueInFinally), @@ -221,6 +221,7 @@ pub fn code_to_rule(linter: Linter, code: &str) -> Option<(RuleGroup, Rule)> { (Pylint, "W0603") => (RuleGroup::Unspecified, rules::pylint::rules::GlobalStatement), (Pylint, "W0711") => (RuleGroup::Unspecified, rules::pylint::rules::BinaryOpException), (Pylint, "W1508") => (RuleGroup::Unspecified, rules::pylint::rules::InvalidEnvvarDefault), + (Pylint, "W1509") => (RuleGroup::Unspecified, rules::pylint::rules::SubprocessPopenPreexecFn), (Pylint, "W2901") => (RuleGroup::Unspecified, rules::pylint::rules::RedefinedLoopName), (Pylint, "W3301") => (RuleGroup::Unspecified, rules::pylint::rules::NestedMinMax), diff --git a/crates/ruff/src/rules/pylint/mod.rs b/crates/ruff/src/rules/pylint/mod.rs index c182a14b23..c1e07788a6 100644 --- a/crates/ruff/src/rules/pylint/mod.rs +++ b/crates/ruff/src/rules/pylint/mod.rs @@ -116,6 +116,10 @@ mod tests { Rule::RepeatedEqualityComparisonTarget, Path::new("repeated_equality_comparison_target.py") )] + #[test_case( + Rule::SubprocessPopenPreexecFn, + Path::new("subprocess_popen_preexec_fn.py") + )] fn rules(rule_code: Rule, path: &Path) -> Result<()> { let snapshot = format!("{}_{}", rule_code.noqa_code(), path.to_string_lossy()); let diagnostics = test_path( diff --git a/crates/ruff/src/rules/pylint/rules/mod.rs b/crates/ruff/src/rules/pylint/rules/mod.rs index 829c9e6916..a2911766a3 100644 --- a/crates/ruff/src/rules/pylint/rules/mod.rs +++ b/crates/ruff/src/rules/pylint/rules/mod.rs @@ -33,6 +33,7 @@ pub(crate) use repeated_equality_comparison_target::*; pub(crate) use repeated_isinstance_calls::*; pub(crate) use return_in_init::*; pub(crate) use single_string_slots::*; +pub(crate) use subprocess_popen_preexec_fn::*; pub(crate) use sys_exit_alias::*; pub(crate) use too_many_arguments::*; pub(crate) use too_many_branches::*; @@ -84,6 +85,7 @@ mod repeated_equality_comparison_target; mod repeated_isinstance_calls; mod return_in_init; mod single_string_slots; +mod subprocess_popen_preexec_fn; mod sys_exit_alias; mod too_many_arguments; mod too_many_branches; diff --git a/crates/ruff/src/rules/pylint/rules/subprocess_popen_preexec_fn.rs b/crates/ruff/src/rules/pylint/rules/subprocess_popen_preexec_fn.rs new file mode 100644 index 0000000000..22a2d6647a --- /dev/null +++ b/crates/ruff/src/rules/pylint/rules/subprocess_popen_preexec_fn.rs @@ -0,0 +1,67 @@ +use ruff_diagnostics::{Diagnostic, Violation}; +use ruff_macros::{derive_message_formats, violation}; +use ruff_python_ast::helpers::{find_keyword, is_const_none}; +use rustpython_parser::ast::{Expr, Keyword, Ranged}; + +use crate::checkers::ast::Checker; + +/// ## What it does +/// Checks for uses of `subprocess.Popen` with a `preexec_fn` argument. +/// +/// ## Why is this bad? +/// The `preexec_fn` argument is unsafe within threads as it can lead to +/// deadlocks. Furthermore, `preexec_fn` is [targeted for deprecation]. +/// +/// Instead, consider using task-specific arguments such as `env`, +/// `start_new_session`, and `process_group`. These are not prone to deadlocks +/// and are more explicit. +/// +/// ## Example +/// ```python +/// import os, subprocess +/// +/// subprocess.Popen(foo, preexec_fn=os.setsid) +/// subprocess.Popen(bar, preexec_fn=os.setpgid(0, 0)) +/// ``` +/// +/// Use instead: +/// ```python +/// import subprocess +/// +/// subprocess.Popen(foo, start_new_session=True) +/// subprocess.Popen(bar, process_group=0) # Introduced in Python 3.11 +/// ``` +/// +/// ## References +/// - [Python documentation: `subprocess.Popen`](https://docs.python.org/3/library/subprocess.html#popen-constructor) +/// - [Why `preexec_fn` in `subprocess.Popen` may lead to deadlock?](https://discuss.python.org/t/why-preexec-fn-in-subprocess-popen-may-lead-to-deadlock/16908/2) +/// +/// [targeted for deprecation]: https://github.com/python/cpython/issues/82616 +#[violation] +pub struct SubprocessPopenPreexecFn; + +impl Violation for SubprocessPopenPreexecFn { + #[derive_message_formats] + fn message(&self) -> String { + format!("`preexec_fn` argument is unsafe when using threads") + } +} + +/// PLW1509 +pub(crate) fn subprocess_popen_preexec_fn(checker: &mut Checker, func: &Expr, kwargs: &[Keyword]) { + if checker + .semantic() + .resolve_call_path(func) + .map_or(false, |call_path| { + matches!(call_path.as_slice(), ["subprocess", "Popen"]) + }) + { + if let Some(keyword) = + find_keyword(kwargs, "preexec_fn").filter(|keyword| !is_const_none(&keyword.value)) + { + checker + .diagnostics + .push(Diagnostic::new(SubprocessPopenPreexecFn, keyword.range())); + } + } +} diff --git a/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLW1509_subprocess_popen_preexec_fn.py.snap b/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLW1509_subprocess_popen_preexec_fn.py.snap new file mode 100644 index 0000000000..a212f70e14 --- /dev/null +++ b/crates/ruff/src/rules/pylint/snapshots/ruff__rules__pylint__tests__PLW1509_subprocess_popen_preexec_fn.py.snap @@ -0,0 +1,42 @@ +--- +source: crates/ruff/src/rules/pylint/mod.rs +--- +subprocess_popen_preexec_fn.py:9:18: PLW1509 `preexec_fn` argument is unsafe when using threads + | + 8 | # Errors. + 9 | subprocess.Popen(preexec_fn=foo) + | ^^^^^^^^^^^^^^ PLW1509 +10 | subprocess.Popen(["ls"], preexec_fn=foo) +11 | subprocess.Popen(preexec_fn=lambda: print("Hello, world!")) + | + +subprocess_popen_preexec_fn.py:10:26: PLW1509 `preexec_fn` argument is unsafe when using threads + | + 8 | # Errors. + 9 | subprocess.Popen(preexec_fn=foo) +10 | subprocess.Popen(["ls"], preexec_fn=foo) + | ^^^^^^^^^^^^^^ PLW1509 +11 | subprocess.Popen(preexec_fn=lambda: print("Hello, world!")) +12 | subprocess.Popen(["ls"], preexec_fn=lambda: print("Hello, world!")) + | + +subprocess_popen_preexec_fn.py:11:18: PLW1509 `preexec_fn` argument is unsafe when using threads + | + 9 | subprocess.Popen(preexec_fn=foo) +10 | subprocess.Popen(["ls"], preexec_fn=foo) +11 | subprocess.Popen(preexec_fn=lambda: print("Hello, world!")) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLW1509 +12 | subprocess.Popen(["ls"], preexec_fn=lambda: print("Hello, world!")) + | + +subprocess_popen_preexec_fn.py:12:26: PLW1509 `preexec_fn` argument is unsafe when using threads + | +10 | subprocess.Popen(["ls"], preexec_fn=foo) +11 | subprocess.Popen(preexec_fn=lambda: print("Hello, world!")) +12 | subprocess.Popen(["ls"], preexec_fn=lambda: print("Hello, world!")) + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ PLW1509 +13 | +14 | # Non-errors. + | + + diff --git a/ruff.schema.json b/ruff.schema.json index 722af19b89..5eae79f20e 100644 --- a/ruff.schema.json +++ b/ruff.schema.json @@ -2258,6 +2258,7 @@ "PLW15", "PLW150", "PLW1508", + "PLW1509", "PLW2", "PLW29", "PLW290",