-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Detect infinite recursion in uv run. #11386
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,5 @@ | ||
use std::borrow::Cow; | ||
use std::env::VarError; | ||
use std::ffi::OsString; | ||
use std::fmt::Write; | ||
use std::io::Read; | ||
|
@@ -91,7 +92,23 @@ pub(crate) async fn run( | |
env_file: Vec<PathBuf>, | ||
no_env_file: bool, | ||
preview: PreviewMode, | ||
max_recursion_depth: u32, | ||
) -> anyhow::Result<ExitStatus> { | ||
// Check if max recursion depth was exceeded. This most commonly happens | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This check could be pushed up higher in the call stack, but it seemed nice to have the logic for the check and the logic for setting/incrementing the counter in the same place. |
||
// for scripts with a shebang line like `#!/usr/bin/env -S uv run`, so try | ||
// to provide guidance for that case. | ||
let recursion_depth = read_recursion_depth_from_environment_variable()?; | ||
if recursion_depth > max_recursion_depth { | ||
bail!( | ||
r" | ||
`uv run` was recursively invoked {recursion_depth} times which exceeds the limit of {max_recursion_depth}. | ||
|
||
hint: If you are running a script with `{}` in the shebang, you may need to include the `{}` flag.", | ||
"uv run".green(), | ||
"--script".green(), | ||
); | ||
} | ||
|
||
// These cases seem quite complex because (in theory) they should change the "current package". | ||
// Let's ban them entirely for now. | ||
let mut requirements_from_stdin: bool = false; | ||
|
@@ -1034,6 +1051,12 @@ pub(crate) async fn run( | |
)?; | ||
process.env(EnvVars::PATH, new_path); | ||
|
||
// Increment recursion depth counter. | ||
process.env( | ||
EnvVars::UV_RUN_RECURSION_DEPTH, | ||
(recursion_depth + 1).to_string(), | ||
); | ||
|
||
// Ensure `VIRTUAL_ENV` is set. | ||
if interpreter.is_virtualenv() { | ||
process.env(EnvVars::VIRTUAL_ENV, interpreter.sys_prefix().as_os_str()); | ||
|
@@ -1464,3 +1487,24 @@ fn is_python_zipapp(target: &Path) -> bool { | |
} | ||
false | ||
} | ||
|
||
/// Read and parse recursion depth from the environment. | ||
/// | ||
/// Returns Ok(0) if `EnvVars::UV_RUN_RECURSION_DEPTH` is not set. | ||
/// | ||
/// Returns an error if `EnvVars::UV_RUN_RECURSION_DEPTH` is set to a value | ||
/// that cannot ber parsed as an integer. | ||
fn read_recursion_depth_from_environment_variable() -> anyhow::Result<u32> { | ||
let envvar = match std::env::var(EnvVars::UV_RUN_RECURSION_DEPTH) { | ||
Ok(val) => val, | ||
Err(VarError::NotPresent) => return Ok(0), | ||
Err(e) => { | ||
return Err(e) | ||
.with_context(|| format!("invalid value for {}", EnvVars::UV_RUN_RECURSION_DEPTH)) | ||
} | ||
}; | ||
|
||
envvar | ||
.parse::<u32>() | ||
.with_context(|| format!("invalid value for {}", EnvVars::UV_RUN_RECURSION_DEPTH)) | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -299,9 +299,16 @@ pub(crate) struct RunSettings { | |
pub(crate) settings: ResolverInstallerSettings, | ||
pub(crate) env_file: Vec<PathBuf>, | ||
pub(crate) no_env_file: bool, | ||
pub(crate) max_recursion_depth: u32, | ||
} | ||
|
||
impl RunSettings { | ||
// Default value for UV_RUN_MAX_RECURSION_DEPTH if unset. This is large | ||
// enough that it's unlikely a user actually needs this recursion depth, | ||
// but short enough that we detect recursion quickly enough to avoid OOMing | ||
// or hanging for a long time. | ||
const DEFAULT_MAX_RECURSION_DEPTH: u32 = 100; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At a depth of 100 this takes about 2.5 seconds to error on a relatively beefy laptop. The time to detection scales more or less linearly with this value, so I think the way to set this is basically to pick something large enough to be unlikely to cause problems, but low enough that we actually detect recursion in enough time to be useful. The 2.5s number above is from running in debug, so if a target error time of ~2s sounds ok, then this can probably be bumped up to 500-1000 in release. |
||
|
||
/// Resolve the [`RunSettings`] from the CLI and filesystem configuration. | ||
#[allow(clippy::needless_pass_by_value)] | ||
pub(crate) fn resolve(args: RunArgs, filesystem: Option<FilesystemOptions>) -> Self { | ||
|
@@ -344,6 +351,7 @@ impl RunSettings { | |
show_resolution, | ||
env_file, | ||
no_env_file, | ||
max_recursion_depth, | ||
} = args; | ||
|
||
let install_mirrors = filesystem | ||
|
@@ -403,6 +411,7 @@ impl RunSettings { | |
env_file, | ||
no_env_file, | ||
install_mirrors, | ||
max_recursion_depth: max_recursion_depth.unwrap_or(Self::DEFAULT_MAX_RECURSION_DEPTH), | ||
} | ||
} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can see a case for adding this to the CLI but... we could also just read it directly. Since you already did this, it seems okay to leave it (as a hidden option).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I made this configurable primarily to support testing (without this being configurable the test for this takes 2-3 seconds in debug), but this seemed like something that a user could theoretically want to control, and it isn't too hard to plumb this to where it needs to go.