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 envvar. Closes #920.
cross-rs · Jul 12, 2022 · 3f8f32e · 3f8f32e
1 parent d526484
commit 3f8f32e
Show file tree

Hide file tree

Showing 6 changed files with 150 additions and 134 deletions.
diff --git a/.changes/942.json 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
@@ -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<String>,
 }
 
-// Fix for issue #581. target_dir must be absolute.
-fn absolute_path(path: PathBuf) -> Result<PathBuf> {
-    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<String> {
     Ok(arg.to_owned())
 }
 
-#[allow(clippy::needless_return)]
 fn store_manifest_path(path: String) -> Result<String> {
-    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<String> {

diff --git 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,
@@ -28,7 +39,7 @@ pub(crate) fn run(
         &mut docker,
         &options,
         &paths,
-        |docker, val| mount(docker, val, ""),
+        |docker, host, absolute| mount(docker, host, absolute, ""),
         |_| {},
     )?;
 
@@ -57,7 +68,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
@@ -169,7 +169,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)
 }
 
@@ -184,7 +188,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)
 }
 
@@ -215,7 +219,11 @@ fn container_path_exists(
 ) -> Result<bool> {
     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())
 }
@@ -572,7 +580,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);
         }
     }
@@ -652,7 +660,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
@@ -847,11 +855,6 @@ pub fn unique_container_identifier(
     Ok(format!("{toolchain_id}-{triple}-{name}-{project_hash}"))
 }
 
-fn mount_path(val: &Path) -> Result<String> {
-    let host_path = file::canonicalize(val)?;
-    canonicalize_mount_path(&host_path)
-}
-
 pub(crate) fn run(
     options: DockerOptions,
     paths: DockerPaths,
@@ -919,7 +922,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")?;
@@ -1102,7 +1105,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;
@@ -1158,7 +1161,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)
@@ -1187,7 +1194,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
@@ -277,17 +277,9 @@ impl Directories {
         });
 
         // root is either workspace_root, or, if we're outside the workspace root, the current directory
-        let mount_root: String;
-        #[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.
-            mount_root = host_root.as_wslpath()?;
-        }
-        #[cfg(not(target_os = "windows"))]
-        {
-            // NOTE: host root has already found the mount path
-            mount_root = host_root.to_utf8()?.to_owned();
-        }
+        // 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)?;
         let sysroot = mount_finder.find_mount_path(sysroot);
 
@@ -440,16 +432,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<String> {
-    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,
@@ -509,7 +491,7 @@ pub(crate) fn docker_mount(
     docker: &mut Command,
     options: &DockerOptions,
     paths: &DockerPaths,
-    mount_cb: impl Fn(&mut Command, &Path) -> Result<String>,
+    mount_cb: impl Fn(&mut Command, &Path, &Path) -> Result<()>,
     mut store_cb: impl FnMut((String, String)),
 ) -> Result<()> {
     for ref var in options
@@ -523,37 +505,30 @@ pub(crate) fn docker_mount(
             None => env::var(var),
         };
 
+        // NOTE: we use canonical paths on the host, since it's unambiguous.
+        // to avoid changing paths on the mount, we use absolute paths.
+
         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)]);
-            store_cb((val, mount_path));
+            let canonical_path = file::canonicalize(&val)?;
+            let absolute_path = Path::new(&val).as_posix_absolute()?;
+            let host_path = paths.mount_finder.find_path(&canonical_path, true)?;
+            mount_cb(docker, host_path.as_ref(), absolute_path.as_ref())?;
+            docker.args(&["-e", &format!("{}={}", var, absolute_path)]);
+            store_cb((val, absolute_path));
         }
     }
 
     for path in paths.workspace_dependencies() {
         let canonical_path = file::canonicalize(path)?;
+        let absolute_path = Path::new(path).as_posix_absolute()?;
         let host_path = paths.mount_finder.find_path(&canonical_path, true)?;
-        let mount_path = mount_cb(docker, host_path.as_ref())?;
-        store_cb((path.to_utf8()?.to_owned(), mount_path));
+        mount_cb(docker, host_path.as_ref(), absolute_path.as_ref())?;
+        store_cb((path.to_utf8()?.to_owned(), absolute_path));
     }
 
     Ok(())
 }
 
-pub(crate) fn canonicalize_mount_path(path: &Path) -> Result<String> {
-    #[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())
 }
@@ -615,7 +590,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
         };
@@ -752,20 +727,15 @@ impl MountFinder {
         path.to_path_buf()
     }
 
-    #[allow(unused_variables, clippy::needless_return)]
     fn find_path(&self, path: &Path, host: bool) -> Result<String> {
-        #[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()
         }
     }
 }
@@ -946,20 +916,9 @@ mod tests {
             Directories::create(mount_finder, metadata, &cwd, &sysroot)
         }
 
-        fn path_to_posix(path: &Path) -> Result<String> {
-            #[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(())
         }
 
@@ -974,9 +933,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(())
@@ -1022,11 +981,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);