From 5bc726a1e78e956c1f25169ef3a99f52c0069da1 Mon Sep 17 00:00:00 2001 From: Zanie Blue Date: Tue, 8 Oct 2024 17:15:47 -0500 Subject: [PATCH] Avoid deleting a project environment directory if we cannot tell if a `pyvenv.cfg` file exists (#8012) I was exploring a fix to an [apparent bug](https://github.com/astral-sh/uv/actions/runs/11240101202/job/31248937280?pr=8010) but this was actually just a CI change https://github.com/astral-sh/uv/pull/8013 Regardless, I think this code is safer? --- crates/uv/src/commands/project/mod.rs | 46 ++++++++++++++++++--------- 1 file changed, 31 insertions(+), 15 deletions(-) diff --git a/crates/uv/src/commands/project/mod.rs b/crates/uv/src/commands/project/mod.rs index 699802b92416..7dd24276f0c5 100644 --- a/crates/uv/src/commands/project/mod.rs +++ b/crates/uv/src/commands/project/mod.rs @@ -525,24 +525,40 @@ pub(crate) async fn get_or_init_environment( let venv = workspace.venv(); // Avoid removing things that are not virtual environments - if venv.exists() && !venv.join("pyvenv.cfg").exists() { - return Err(ProjectError::InvalidProjectEnvironmentDir( - venv, - "it is not a compatible environment but cannot be recreated because it is not a virtual environment".to_string(), - )); - } + let should_remove = match (venv.try_exists(), venv.join("pyvenv.cfg").try_exists()) { + // It's a virtual environment we can remove it + (_, Ok(true)) => true, + // It doesn't exist at all, we should use it without deleting it to avoid TOCTOU bugs + (Ok(false), Ok(false)) => false, + // If it's not a virtual environment, bail + (Ok(true), Ok(false)) => { + return Err(ProjectError::InvalidProjectEnvironmentDir( + venv, + "it is not a compatible environment but cannot be recreated because it is not a virtual environment".to_string(), + )); + } + // Similarly, if we can't _tell_ if it exists we should bail + (_, Err(err)) | (Err(err), _) => { + return Err(ProjectError::InvalidProjectEnvironmentDir( + venv, + format!("it is not a compatible environment but cannot be recreated because uv cannot determine if it is a virtual environment: {err}"), + )); + } + }; // Remove the existing virtual environment if it doesn't meet the requirements. - match fs_err::remove_dir_all(&venv) { - Ok(()) => { - writeln!( - printer.stderr(), - "Removed virtual environment at: {}", - venv.user_display().cyan() - )?; + if should_remove { + match fs_err::remove_dir_all(&venv) { + Ok(()) => { + writeln!( + printer.stderr(), + "Removed virtual environment at: {}", + venv.user_display().cyan() + )?; + } + Err(e) if e.kind() == std::io::ErrorKind::NotFound => {} + Err(e) => return Err(e.into()), } - Err(e) if e.kind() == std::io::ErrorKind::NotFound => {} - Err(e) => return Err(e.into()), } writeln!(