From f593f4ca05f62d0eec063fb6f2f986da3c3be9aa Mon Sep 17 00:00:00 2001 From: Claudio Bley Date: Tue, 4 Jun 2024 13:46:52 +0200 Subject: [PATCH 1/4] Fix typo --- app/buck2_forkserver/src/run.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/buck2_forkserver/src/run.rs b/app/buck2_forkserver/src/run.rs index 3a206245fd2c3..9987c8234f943 100644 --- a/app/buck2_forkserver/src/run.rs +++ b/app/buck2_forkserver/src/run.rs @@ -341,7 +341,7 @@ impl KillProcess for DefaultKillProcess { /// Unify the the behavior of using a relative path for the executable between Unix and Windows. On /// UNIX, the path is understood to be relative to the cwd of the *spawned process*, whereas on -/// Windows, it's relative ot the cwd of the *spawning* process. +/// Windows, it's relative to the cwd of the *spawning* process. /// /// Here, we unify the two behaviors since we always run our subprocesses with a known cwd: we /// check if the executable actually exists relative to said cwd, and if it does, we use that. From 31eeae9d070e7ca8e5a8fa5f87a3ae74160768c6 Mon Sep 17 00:00:00 2001 From: Claudio Bley Date: Tue, 4 Jun 2024 13:48:22 +0200 Subject: [PATCH 2/4] Only absolutize exe if abs path refers to a file and is executable Fixes #670 --- app/buck2_forkserver/src/run.rs | 16 +++++++++++++++- 1 file changed, 15 insertions(+), 1 deletion(-) diff --git a/app/buck2_forkserver/src/run.rs b/app/buck2_forkserver/src/run.rs index 9987c8234f943..abbb60f755630 100644 --- a/app/buck2_forkserver/src/run.rs +++ b/app/buck2_forkserver/src/run.rs @@ -12,6 +12,7 @@ pub mod process_group; pub mod status_decoder; use std::borrow::Cow; +use std::fs; use std::path::Path; use std::pin::Pin; use std::process::Command; @@ -353,7 +354,20 @@ pub fn maybe_absolutize_exe<'a>( let abs = spawned_process_cwd.join(exe); if fs_util::try_exists(&abs).context("Error absolute-izing executable")? { - return Ok(abs.into_path_buf().into()); + let metadata = fs::metadata(&abs).context("Error getting metadata for path")?; + if metadata.is_file() { + #[cfg(unix)] + { + use std::os::unix::fs::PermissionsExt; + if metadata.permissions().mode() & 0o111 != 0 { + return Ok(abs.into_path_buf().into()); + } + } + #[cfg(not(unix))] + { + return Ok(abs.into_path_buf().into()); + } + } } Ok(exe.into()) From e34ab876be705af1182277259059f0afa3383422 Mon Sep 17 00:00:00 2001 From: Claudio Bley Date: Tue, 2 Jul 2024 10:37:10 +0200 Subject: [PATCH 3/4] Introduce `fs_util::metadata_if_exists` --- app/buck2_core/src/fs/fs_util.rs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/app/buck2_core/src/fs/fs_util.rs b/app/buck2_core/src/fs/fs_util.rs index bbd38d5c93731..89768d7c35319 100644 --- a/app/buck2_core/src/fs/fs_util.rs +++ b/app/buck2_core/src/fs/fs_util.rs @@ -442,6 +442,17 @@ pub fn remove_dir_all>(path: P) -> Result<(), IoError> { ) } +/// `None` if file does not exist. +pub fn metadata_if_exists>( + path: P, +) -> Result, IoError> { + let _guard = IoCounterKey::Stat.guard(); + make_error!( + if_exists(fs::metadata(path.as_ref().as_maybe_relativized())), + format!("metadata({})", path.as_ref().display()) + ) +} + /// `None` if file does not exist. pub fn symlink_metadata_if_exists>( path: P, From 80eb772932ff8d16126cffcde2c7572420b9dd6f Mon Sep 17 00:00:00 2001 From: Claudio Bley Date: Tue, 2 Jul 2024 10:37:46 +0200 Subject: [PATCH 4/4] Use `fs_util::metadata_if_exists` to avoid race condition --- app/buck2_forkserver/src/run.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/app/buck2_forkserver/src/run.rs b/app/buck2_forkserver/src/run.rs index abbb60f755630..ea201010179b4 100644 --- a/app/buck2_forkserver/src/run.rs +++ b/app/buck2_forkserver/src/run.rs @@ -12,7 +12,6 @@ pub mod process_group; pub mod status_decoder; use std::borrow::Cow; -use std::fs; use std::path::Path; use std::pin::Pin; use std::process::Command; @@ -353,8 +352,8 @@ pub fn maybe_absolutize_exe<'a>( let exe = exe.as_ref(); let abs = spawned_process_cwd.join(exe); - if fs_util::try_exists(&abs).context("Error absolute-izing executable")? { - let metadata = fs::metadata(&abs).context("Error getting metadata for path")?; + + if let Some(metadata) = fs_util::metadata_if_exists(&abs).context("Error getting metadata for path")? { if metadata.is_file() { #[cfg(unix)] {