From b3a62ea2dd80a4ae0acd795de3de83a953656f1f Mon Sep 17 00:00:00 2001 From: Alex Huszagh Date: Fri, 15 Jul 2022 19:48:53 -0500 Subject: [PATCH] Use non-canonical paths for mount paths. Symlinks and canonical paths can destroy assumptions about paths: say I have a file at `/tmp/path/to/file` I reference in my crate, and ask to mount `/tmp/path/to` on the container. Currently, on macOS, since `/tmp` is a symlink to `/private/tmp`, the code that expects `/tmp/path/to/file` will fail because in the container it will be mounted at `/private/tmp/path/to/file`. Also fix device_ns parsing with drive letters. Fixes a minor bug where the host root instead of the var names was passed as the environment variable. --- .changes/942.json | 15 ++++++ src/cli.rs | 27 +--------- src/docker/local.rs | 21 ++++++-- src/docker/remote.rs | 35 +++++++------ src/docker/shared.rs | 116 ++++++++++++++++++------------------------- src/file.rs | 86 ++++++++++++++++++++++++-------- 6 files changed, 169 insertions(+), 131 deletions(-) create mode 100644 .changes/942.json diff --git a/.changes/942.json b/.changes/942.json new file mode 100644 index 000000000..0144174da --- /dev/null +++ b/.changes/942.json @@ -0,0 +1,15 @@ +[ + { + "description": "use non-canonical paths for mount locations.", + "type": "changed", + "issues": [920] + }, + { + "description": "fixed DeviceNS drive parsing in creating WSL-style paths on windows.", + "type": "fixed" + }, + { + "description": "fixed the environment variable name for mounted volumes.", + "type": "fixed" + } +] diff --git a/src/cli.rs b/src/cli.rs index bc7743fbe..f5cf5986e 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -3,7 +3,7 @@ use std::path::{Path, PathBuf}; use crate::cargo::Subcommand; use crate::errors::Result; -use crate::file::PathExt; +use crate::file::{absolute_path, PathExt}; use crate::rustc::TargetList; use crate::shell::{self, MessageInfo}; use crate::Target; @@ -23,15 +23,6 @@ pub struct Args { pub color: Option, } -// Fix for issue #581. target_dir must be absolute. -fn absolute_path(path: PathBuf) -> Result { - Ok(if path.is_absolute() { - path - } else { - env::current_dir()?.join(path) - }) -} - pub fn is_subcommand_list(stdout: &str) -> bool { stdout.starts_with("Installed Commands:") } @@ -151,22 +142,8 @@ fn str_to_owned(arg: &str) -> Result { Ok(arg.to_owned()) } -#[allow(clippy::needless_return)] fn store_manifest_path(path: String) -> Result { - let p = Path::new(&path); - if p.is_absolute() { - #[cfg(target_os = "windows")] - { - return p.as_wslpath(); - } - #[cfg(not(target_os = "windows"))] - { - use crate::file::ToUtf8; - return p.to_utf8().map(ToOwned::to_owned); - } - } else { - p.as_posix() - } + Path::new(&path).as_posix_relative() } fn store_target_dir(_: String) -> Result { diff --git a/src/docker/local.rs b/src/docker/local.rs index 665bb385e..05e1b3d37 100644 --- a/src/docker/local.rs +++ b/src/docker/local.rs @@ -1,5 +1,6 @@ use std::io; -use std::process::ExitStatus; +use std::path::Path; +use std::process::{Command, ExitStatus}; use super::shared::*; use crate::errors::Result; @@ -8,6 +9,16 @@ use crate::file::{PathExt, ToUtf8}; use crate::shell::{MessageInfo, Stream}; use eyre::Context; +// NOTE: host path must be absolute +fn mount(docker: &mut Command, host_path: &Path, absolute_path: &Path, prefix: &str) -> Result<()> { + let mount_path = absolute_path.as_posix_absolute()?; + docker.args(&[ + "-v", + &format!("{}:{prefix}{}", host_path.to_utf8()?, mount_path), + ]); + Ok(()) +} + pub(crate) fn run( options: DockerOptions, paths: DockerPaths, @@ -39,7 +50,7 @@ pub(crate) fn run( &mut docker, &options, &paths, - |docker, val| mount(docker, val, ""), + |docker, host, absolute| mount(docker, host, absolute, ""), |_| {}, )?; @@ -81,7 +92,11 @@ pub(crate) fn run( if let Some(ref nix_store) = dirs.nix_store { docker.args(&[ "-v", - &format!("{}:{}:Z", nix_store.to_utf8()?, nix_store.as_posix()?), + &format!( + "{}:{}:Z", + nix_store.to_utf8()?, + nix_store.as_posix_absolute()? + ), ]); } diff --git a/src/docker/remote.rs b/src/docker/remote.rs index bdb2926e8..cfc5cf32c 100644 --- a/src/docker/remote.rs +++ b/src/docker/remote.rs @@ -168,7 +168,11 @@ fn create_volume_dir( // make our parent directory if needed subcommand_or_exit(engine, "exec")? .arg(container) - .args(&["sh", "-c", &format!("mkdir -p '{}'", dir.as_posix()?)]) + .args(&[ + "sh", + "-c", + &format!("mkdir -p '{}'", dir.as_posix_absolute()?), + ]) .run_and_get_status(msg_info, false) } @@ -183,7 +187,7 @@ fn copy_volume_files( subcommand_or_exit(engine, "cp")? .arg("-a") .arg(src.to_utf8()?) - .arg(format!("{container}:{}", dst.as_posix()?)) + .arg(format!("{container}:{}", dst.as_posix_absolute()?)) .run_and_get_status(msg_info, false) } @@ -214,7 +218,11 @@ fn container_path_exists( ) -> Result { Ok(subcommand_or_exit(engine, "exec")? .arg(container) - .args(&["bash", "-c", &format!("[[ -d '{}' ]]", path.as_posix()?)]) + .args(&[ + "bash", + "-c", + &format!("[[ -d '{}' ]]", path.as_posix_absolute()?), + ]) .run_and_get_status(msg_info, true)? .success()) } @@ -567,7 +575,7 @@ fn read_dir_fingerprint( let modified = file.metadata()?.modified()?; let millis = modified.duration_since(epoch)?.as_millis() as u64; let rounded = epoch + time::Duration::from_millis(millis); - let relpath = file.path().strip_prefix(home)?.as_posix()?; + let relpath = file.path().strip_prefix(home)?.as_posix_relative()?; map.insert(relpath, rounded); } } @@ -647,7 +655,7 @@ rm \"{PATH}\" // SAFETY: safe, single-threaded execution. let mut tempfile = unsafe { temp::TempFile::new()? }; for file in files { - writeln!(tempfile.file(), "{}", dst.join(file).as_posix()?)?; + writeln!(tempfile.file(), "{}", dst.join(file).as_posix_relative()?)?; } // need to avoid having hundreds of files on the command, so @@ -845,11 +853,6 @@ pub fn unique_container_identifier( Ok(format!("{toolchain_id}-{triple}-{name}-{project_hash}")) } -fn mount_path(val: &Path) -> Result { - let host_path = file::canonicalize(val)?; - canonicalize_mount_path(&host_path) -} - pub(crate) fn run( options: DockerOptions, paths: DockerPaths, @@ -921,7 +924,7 @@ pub(crate) fn run( &mut docker, &options, &paths, - |_, val| mount_path(val), + |_, _, _| Ok(()), |(src, dst)| volumes.push((src, dst)), ) .wrap_err("could not determine mount points")?; @@ -1100,7 +1103,7 @@ pub(crate) fn run( let mut final_args = vec![]; let mut iter = args.iter().cloned(); let mut has_target_dir = false; - let target_dir_string = target_dir.as_posix()?; + let target_dir_string = target_dir.as_posix_absolute()?; while let Some(arg) = iter.next() { if arg == "--target-dir" { has_target_dir = true; @@ -1156,7 +1159,11 @@ symlink_recurse \"${{prefix}}\" " )); for (src, dst) in to_symlink { - symlink.push(format!("ln -s \"{}\" \"{}\"", src.as_posix()?, dst)); + symlink.push(format!( + "ln -s \"{}\" \"{}\"", + src.as_posix_absolute()?, + dst + )); } subcommand_or_exit(engine, "exec")? .arg(&container) @@ -1185,7 +1192,7 @@ symlink_recurse \"${{prefix}}\" if !skip_artifacts && container_path_exists(engine, &container, &target_dir, msg_info)? { subcommand_or_exit(engine, "cp")? .arg("-a") - .arg(&format!("{container}:{}", target_dir.as_posix()?)) + .arg(&format!("{container}:{}", target_dir.as_posix_absolute()?)) .arg( &dirs .target diff --git a/src/docker/shared.rs b/src/docker/shared.rs index 1e572b095..bdb726307 100644 --- a/src/docker/shared.rs +++ b/src/docker/shared.rs @@ -12,15 +12,12 @@ use crate::cargo::{cargo_metadata_with_args, CargoMetadata}; use crate::config::{bool_from_envvar, Config}; use crate::errors::*; use crate::extensions::{CommandExt, SafeCommand}; -use crate::file::{self, write_file, ToUtf8}; +use crate::file::{self, write_file, PathExt, ToUtf8}; use crate::id; use crate::rustc::QualifiedToolchain; use crate::shell::{MessageInfo, Verbosity}; use crate::Target; -#[cfg(target_os = "windows")] -use crate::file::PathExt; - pub use super::custom::CROSS_CUSTOM_DOCKERFILE_IMAGE_PREFIX; pub const CROSS_IMAGE: &str = "ghcr.io/cross-rs"; @@ -287,6 +284,10 @@ impl Directories { } create_target_dir(target)?; + // get our mount paths prior to canonicalizing them + let cargo_mount_path = cargo.as_posix_absolute()?; + let xargo_mount_path = xargo.as_posix_absolute()?; + // now that we know the paths exist, canonicalize them. this avoids creating // directories after failed canonicalization into a shared directory. let cargo = file::canonicalize(&cargo)?; @@ -307,17 +308,15 @@ impl Directories { &metadata.workspace_root }); - // root is either workspace_root, or, if we're outside the workspace root, the current directory - // NOTE: host root has already found the mount path - let mount_root = canonicalize_mount_path(&host_root)?; + // on Windows, we can not mount the directory name directly. Instead, we use wslpath to convert the path to a linux compatible path. + // NOTE: on unix, host root has already found the mount path + let mount_root = host_root.as_posix_absolute()?; let mount_cwd = mount_finder.find_path(cwd, false)?; toolchain.set_sysroot(|p| mount_finder.find_mount_path(p)); // canonicalize these once to avoid syscalls - let cargo_mount_path = canonicalize_mount_path(&cargo)?; - let xargo_mount_path = canonicalize_mount_path(&xargo)?; - let sysroot_mount_path = canonicalize_mount_path(toolchain.get_sysroot())?; + let sysroot_mount_path = toolchain.get_sysroot().as_posix_absolute()?; Ok(Directories { cargo, @@ -501,16 +500,6 @@ fn add_cargo_configuration_envvars(docker: &mut Command) { } } -// NOTE: host path must be canonical -pub(crate) fn mount(docker: &mut Command, host_path: &Path, prefix: &str) -> Result { - let mount_path = canonicalize_mount_path(host_path)?; - docker.args(&[ - "-v", - &format!("{}:{prefix}{}", host_path.to_utf8()?, mount_path), - ]); - Ok(mount_path) -} - pub(crate) fn docker_envvars( docker: &mut Command, config: &Config, @@ -579,7 +568,7 @@ pub(crate) fn docker_mount( docker: &mut Command, options: &DockerOptions, paths: &DockerPaths, - mount_cb: impl Fn(&mut Command, &Path) -> Result, + mount_cb: impl Fn(&mut Command, &Path, &Path) -> Result<()>, mut store_cb: impl FnMut((String, String)), ) -> Result<()> { for ref var in options @@ -593,37 +582,44 @@ pub(crate) fn docker_mount( None => env::var(var), }; + // NOTE: we use canonical paths on the host, since it's unambiguous. + // however, for the mounted paths, we use the same path as was + // provided. this avoids canonicalizing symlinks which then causes + // the mounted path to differ from the path expected on the host. + // for example, if `/tmp` is a symlink to `/private/tmp`, canonicalizing + // it would lead to us mounting `/tmp/process` to `/private/tmp/process`, + // which would cause any code relying on `/tmp/process` to break. + if let Ok(val) = value { - let canonical_val = file::canonicalize(&val)?; - let host_path = paths.mount_finder.find_path(&canonical_val, true)?; - let mount_path = mount_cb(docker, host_path.as_ref())?; - docker.args(&["-e", &format!("{}={}", host_path, mount_path)]); + let canonical_path = file::canonicalize(&val)?; + let host_path = paths.mount_finder.find_path(&canonical_path, true)?; + let absolute_path = Path::new(&val).as_posix_absolute()?; + let mount_path = paths + .mount_finder + .find_path(Path::new(&absolute_path), true)?; + mount_cb(docker, host_path.as_ref(), mount_path.as_ref())?; + docker.args(&["-e", &format!("{}={}", var, mount_path)]); store_cb((val, mount_path)); } } for path in paths.workspace_dependencies() { + // NOTE: we use canonical paths here since cargo metadata + // always canonicalizes paths, so these should be relative + // to the mounted project directory. let canonical_path = file::canonicalize(path)?; let host_path = paths.mount_finder.find_path(&canonical_path, true)?; - let mount_path = mount_cb(docker, host_path.as_ref())?; + let absolute_path = Path::new(path).as_posix_absolute()?; + let mount_path = paths + .mount_finder + .find_path(Path::new(&absolute_path), true)?; + mount_cb(docker, host_path.as_ref(), mount_path.as_ref())?; store_cb((path.to_utf8()?.to_owned(), mount_path)); } Ok(()) } -pub(crate) fn canonicalize_mount_path(path: &Path) -> Result { - #[cfg(target_os = "windows")] - { - // On Windows, we can not mount the directory name directly. Instead, we use wslpath to convert the path to a linux compatible path. - path.as_wslpath() - } - #[cfg(not(target_os = "windows"))] - { - path.to_utf8().map(|p| p.to_owned()) - } -} - pub(crate) fn user_id() -> String { env::var("CROSS_CONTAINER_UID").unwrap_or_else(|_| id::user().to_string()) } @@ -685,7 +681,7 @@ pub(crate) fn docker_seccomp( #[cfg(target_os = "windows")] if matches!(engine_type, EngineType::Podman | EngineType::PodmanRemote) { // podman weirdly expects a WSL path here, and fails otherwise - path_string = path.as_wslpath()?; + path_string = path.as_posix_absolute()?; } path_string }; @@ -897,20 +893,15 @@ impl MountFinder { path.to_path_buf() } - #[allow(unused_variables, clippy::needless_return)] fn find_path(&self, path: &Path, host: bool) -> Result { - #[cfg(target_os = "windows")] - { - // On Windows, we can not mount the directory name directly. Instead, we use wslpath to convert the path to a linux compatible path. - if host { - return Ok(path.to_utf8()?.to_owned()); - } else { - return path.as_wslpath(); - } - } - #[cfg(not(target_os = "windows"))] - { - return Ok(self.find_mount_path(path).to_utf8()?.to_owned()); + if cfg!(target_os = "windows") && host { + // On Windows, we can not mount the directory name directly. + // Instead, we convert the path to a linux compatible path. + return path.to_utf8().map(ToOwned::to_owned); + } else if cfg!(target_os = "windows") { + path.as_posix_absolute() + } else { + self.find_mount_path(path).as_posix_absolute() } } } @@ -1102,20 +1093,9 @@ mod tests { Directories::create(mount_finder, metadata, &cwd, toolchain) } - fn path_to_posix(path: &Path) -> Result { - #[cfg(target_os = "windows")] - { - path.as_wslpath() - } - #[cfg(not(target_os = "windows"))] - { - path.as_posix() - } - } - #[track_caller] fn paths_equal(x: &Path, y: &Path) -> Result<()> { - assert_eq!(path_to_posix(x)?, path_to_posix(y)?); + assert_eq!(x.as_posix_absolute()?, y.as_posix_absolute()?); Ok(()) } @@ -1130,9 +1110,9 @@ mod tests { paths_equal(&directories.host_root, &metadata.workspace_root)?; assert_eq!( &directories.mount_root, - &path_to_posix(&metadata.workspace_root)? + &metadata.workspace_root.as_posix_absolute()? ); - assert_eq!(&directories.mount_cwd, &path_to_posix(&get_cwd()?)?); + assert_eq!(&directories.mount_cwd, &get_cwd()?.as_posix_absolute()?); reset_env(vars); Ok(()) @@ -1178,11 +1158,11 @@ mod tests { paths_equal(&directories.host_root, &mount_path(get_cwd()?))?; assert_eq!( &directories.mount_root, - &path_to_posix(&mount_path(get_cwd()?))? + &mount_path(get_cwd()?).as_posix_absolute()? ); assert_eq!( &directories.mount_cwd, - &path_to_posix(&mount_path(get_cwd()?))? + &mount_path(get_cwd()?).as_posix_absolute()? ); reset_env(vars); diff --git a/src/file.rs b/src/file.rs index bebf2050b..2e9cf7ca1 100644 --- a/src/file.rs +++ b/src/file.rs @@ -1,4 +1,5 @@ use std::borrow::Cow; +use std::env; use std::ffi::OsStr; use std::fs::{self, File}; use std::io::Read; @@ -26,9 +27,8 @@ impl ToUtf8 for Path { } pub trait PathExt { - fn as_posix(&self) -> Result; - #[cfg(target_family = "windows")] - fn as_wslpath(&self) -> Result; + fn as_posix_relative(&self) -> Result; + fn as_posix_absolute(&self) -> Result; } #[cfg(target_family = "windows")] @@ -44,6 +44,23 @@ fn fmt_disk(disk: u8) -> String { (disk as char).to_string() } +#[cfg(target_family = "windows")] +fn fmt_ns_disk(disk: &std::ffi::OsStr) -> Result { + let disk = disk.to_utf8()?; + Ok(match disk.len() { + // ns can be similar to `\\.\COM42`, or also `\\.\C:\` + 2 => { + let c = disk.chars().next().expect("cannot be empty"); + if c.is_ascii_alphabetic() && disk.ends_with(':') { + fmt_disk(c as u8) + } else { + disk.to_owned() + } + } + _ => disk.to_owned(), + }) +} + #[cfg(target_family = "windows")] fn fmt_unc(server: &std::ffi::OsStr, volume: &std::ffi::OsStr) -> Result { let server = server.to_utf8()?; @@ -61,7 +78,7 @@ fn fmt_unc(server: &std::ffi::OsStr, volume: &std::ffi::OsStr) -> Result } impl PathExt for Path { - fn as_posix(&self) -> Result { + fn as_posix_relative(&self) -> Result { if cfg!(target_os = "windows") { let push = |p: &mut String, c: &str| { if !p.is_empty() && p != "/" { @@ -89,11 +106,16 @@ impl PathExt for Path { } } - // this is similar to as_posix, but it handles drive separators - // and doesn't assume a relative path. + #[cfg(not(target_family = "windows"))] + fn as_posix_absolute(&self) -> Result { + absolute_path(self)?.to_utf8().map(ToOwned::to_owned) + } + + // this is similar to as_posix_relative, but it handles drive + // separators and will only work with absolute paths. #[cfg(target_family = "windows")] - fn as_wslpath(&self) -> Result { - let path = canonicalize(self)?; + fn as_posix_absolute(&self) -> Result { + let path = absolute_path(self)?; let push = |p: &mut String, c: &str, r: bool| { if !r { @@ -115,7 +137,7 @@ impl PathExt for Path { // a root_prefix since we force absolute paths. Prefix::VerbatimDisk(disk) => fmt_disk(disk), Prefix::UNC(server, volume) => fmt_unc(server, volume)?, - Prefix::DeviceNS(ns) => ns.to_utf8()?.to_owned(), + Prefix::DeviceNS(ns) => fmt_ns_disk(ns)?, Prefix::Disk(disk) => fmt_disk(disk), } } @@ -174,6 +196,15 @@ fn _canonicalize(path: &Path) -> Result { } } +// Fix for issue #581. target_dir must be absolute. +pub fn absolute_path(path: impl AsRef) -> Result { + Ok(if path.as_ref().is_absolute() { + path.as_ref().to_path_buf() + } else { + env::current_dir()?.join(path) + }) +} + /// Pretty format a file path. Also removes the path prefix from a command if wanted pub fn pretty_path(path: impl AsRef, strip: impl for<'a> Fn(&'a str) -> bool) -> String { let path = path.as_ref(); @@ -257,6 +288,7 @@ mod tests { }; } + #[track_caller] fn result_eq(x: Result, y: Result) { match (x, y) { (Ok(x), Ok(y)) => assert_eq!(x, y), @@ -265,32 +297,44 @@ mod tests { } #[test] - fn as_posix() { - result_eq(p!(".").join("..").as_posix(), Ok("./..".to_owned())); - result_eq(p!(".").join("/").as_posix(), Ok("/".to_owned())); - result_eq(p!("foo").join("bar").as_posix(), Ok("foo/bar".to_owned())); - result_eq(p!("/foo").join("bar").as_posix(), Ok("/foo/bar".to_owned())); + fn as_posix_relative() { + result_eq( + p!(".").join("..").as_posix_relative(), + Ok("./..".to_owned()), + ); + result_eq(p!(".").join("/").as_posix_relative(), Ok("/".to_owned())); + result_eq( + p!("foo").join("bar").as_posix_relative(), + Ok("foo/bar".to_owned()), + ); + result_eq( + p!("/foo").join("bar").as_posix_relative(), + Ok("/foo/bar".to_owned()), + ); } #[test] #[cfg(target_family = "windows")] fn as_posix_prefix() { assert_eq!(p!("C:").join(".."), p!("C:..")); - assert!(p!("C:").join("..").as_posix().is_err()); + assert!(p!("C:").join("..").as_posix_relative().is_err()); } #[test] #[cfg(target_family = "windows")] - fn as_wslpath() { - result_eq(p!(r"C:\").as_wslpath(), Ok("/mnt/c".to_owned())); - result_eq(p!(r"C:\Users").as_wslpath(), Ok("/mnt/c/Users".to_owned())); + fn as_posix_with_drive() { + result_eq(p!(r"C:\").as_posix_absolute(), Ok("/mnt/c".to_owned())); + result_eq( + p!(r"C:\Users").as_posix_absolute(), + Ok("/mnt/c/Users".to_owned()), + ); result_eq( - p!(r"\\localhost\c$\Users").as_wslpath(), + p!(r"\\localhost\c$\Users").as_posix_absolute(), Ok("/mnt/c/Users".to_owned()), ); - result_eq(p!(r"\\.\C:\").as_wslpath(), Ok("/mnt/c".to_owned())); + result_eq(p!(r"\\.\C:\").as_posix_absolute(), Ok("/mnt/c".to_owned())); result_eq( - p!(r"\\.\C:\Users").as_wslpath(), + p!(r"\\.\C:\Users").as_posix_absolute(), Ok("/mnt/c/Users".to_owned()), ); }