From 0d06193bf1ff4d1f79e1676cd8079704368f357b Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Thu, 21 Oct 2021 17:03:58 +0800 Subject: [PATCH 1/3] Only canonicalize executable path if it has relative directories Otherwise symbolic links may also accidentally be resolved which may lead to unexpected results in the case of 'coreutils', a binary that depends on the executable name being a symbolic link. This means a path like /bin/echo being canonicalized to /bin/coreutils will loose all information about the desired functionality. Test failures will occur if 'echo' is resolved that way and it's not trivial to find the cause of it in the provided error messages. --- crates/cargo-util/src/paths.rs | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/crates/cargo-util/src/paths.rs b/crates/cargo-util/src/paths.rs index d63611d7a55..66ff79d2132 100644 --- a/crates/cargo-util/src/paths.rs +++ b/crates/cargo-util/src/paths.rs @@ -125,7 +125,19 @@ pub fn resolve_executable(exec: &Path) -> Result { if candidate.is_file() { // PATH may have a component like "." in it, so we still need to // canonicalize. - return Ok(candidate.canonicalize()?); + // Only do so if there are relative path components, otherwise symlinks to 'echo' may be resolved to their + // root program like 'coreutils' which relies on the executable name for proper function. + let has_relative_path_components = candidate.components().any(|c| { + matches!( + c, + std::path::Component::ParentDir | std::path::Component::CurDir + ) + }); + return Ok(if has_relative_path_components { + candidate.canonicalize()? + } else { + candidate + }); } } From 4906ef236441ebbc4fdb53892b215ad458ed9931 Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Thu, 21 Oct 2021 18:19:04 +0800 Subject: [PATCH 2/3] Assure the binary name won't change after canonicalization, and keep looking if it does. --- crates/cargo-util/src/paths.rs | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/crates/cargo-util/src/paths.rs b/crates/cargo-util/src/paths.rs index 66ff79d2132..7dafca6548e 100644 --- a/crates/cargo-util/src/paths.rs +++ b/crates/cargo-util/src/paths.rs @@ -125,8 +125,7 @@ pub fn resolve_executable(exec: &Path) -> Result { if candidate.is_file() { // PATH may have a component like "." in it, so we still need to // canonicalize. - // Only do so if there are relative path components, otherwise symlinks to 'echo' may be resolved to their - // root program like 'coreutils' which relies on the executable name for proper function. + // Only do so if there are relative path components let has_relative_path_components = candidate.components().any(|c| { matches!( c, @@ -134,7 +133,22 @@ pub fn resolve_executable(exec: &Path) -> Result { ) }); return Ok(if has_relative_path_components { - candidate.canonicalize()? + // Assure symlinks to programs like 'echo' don't change the file-name after resolution. + // root program like 'coreutils' which relies on the executable name for proper function. + let file_name = candidate + .file_name() + .expect("executables have a file name") + .to_owned(); + let candidate = candidate + .canonicalize()? + .parent() + .expect("a parent is always available for tools called in test-suite") + .join(file_name) + .to_owned(); + if !candidate.is_file() { + continue; + } + candidate } else { candidate }); From cf8e464d6f9cff649c5c32be0ec1e610dc88157e Mon Sep 17 00:00:00 2001 From: Sebastian Thiel Date: Sat, 23 Oct 2021 08:53:31 +0800 Subject: [PATCH 3/3] Do not canonicalize the exe-candidate at all And here is why: https://github.com/rust-lang/cargo/pull/9991#issuecomment-949727679 --- crates/cargo-util/src/paths.rs | 32 ++------------------------------ 1 file changed, 2 insertions(+), 30 deletions(-) diff --git a/crates/cargo-util/src/paths.rs b/crates/cargo-util/src/paths.rs index 7dafca6548e..ee314eb69fd 100644 --- a/crates/cargo-util/src/paths.rs +++ b/crates/cargo-util/src/paths.rs @@ -123,41 +123,13 @@ pub fn resolve_executable(exec: &Path) -> Result { }); for candidate in candidates { if candidate.is_file() { - // PATH may have a component like "." in it, so we still need to - // canonicalize. - // Only do so if there are relative path components - let has_relative_path_components = candidate.components().any(|c| { - matches!( - c, - std::path::Component::ParentDir | std::path::Component::CurDir - ) - }); - return Ok(if has_relative_path_components { - // Assure symlinks to programs like 'echo' don't change the file-name after resolution. - // root program like 'coreutils' which relies on the executable name for proper function. - let file_name = candidate - .file_name() - .expect("executables have a file name") - .to_owned(); - let candidate = candidate - .canonicalize()? - .parent() - .expect("a parent is always available for tools called in test-suite") - .join(file_name) - .to_owned(); - if !candidate.is_file() { - continue; - } - candidate - } else { - candidate - }); + return Ok(candidate); } } anyhow::bail!("no executable for `{}` found in PATH", exec.display()) } else { - Ok(exec.canonicalize()?) + Ok(exec.into()) } }