Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use non-canonical paths for mount paths. #942

Merged
merged 1 commit into from
Jul 16, 2022
Merged

Conversation

Alexhuszagh
Copy link
Contributor

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.

In addition, this fixes DeviceNS parsing with drive letters (this previously didn't matter since it never occurred, due to dunce canonicalization). This PR also fixes a minor bug where the host root instead of the var names was passed as the environment variable (introduced in #904).

Closes #920.

@Alexhuszagh Alexhuszagh requested a review from a team as a code owner July 12, 2022 03:43
@Alexhuszagh Alexhuszagh marked this pull request as draft July 12, 2022 03:43
@Alexhuszagh
Copy link
Contributor Author

I still have to test this extensively on macOS and with symlinks, just to ensure this works well (and also for Docker-in-Docker).

@Alexhuszagh
Copy link
Contributor Author

Alexhuszagh commented Jul 12, 2022

This mostly works: there's some issues with Docker-in-Docker it seems, but the core logic is sound enough. There's no way to use non-canonical paths for the current project directory (easily), since cargo metadata resolves any symlinks. That being said, everything else seems to work.

Specifically, this fails in workspaces for docker-in-docker: this is the mount point:

-v $PREFIX/merged/tmp/tmp.BHCKdn/external/external_lib:/tmp/tmp.BHCKdn/external/external_lib

And this is where it was supposed to be read:

failed to read `/var/lib/docker/overlay2/c7c0edabf2c42ce958ffef1d9eb8b59b903c0bdf2605682a8ca2cb54a2662ef0/merged/tmp/tmp.BHCKdn/external/external_lib/Cargo.toml`

Which means that the lack of the canonicalization seems to have undone this. We may have to canonicalize paths for docker-in-docker, or at least those used in workspaces (since this seems to be a workspace-specific issue, and might manifest in theory outside of docker-in-docker).

EDIT: This was a bug in my code and using mount_finder.

@Alexhuszagh Alexhuszagh marked this pull request as ready for review July 12, 2022 04:47
bors bot added a commit that referenced this pull request Jul 14, 2022
947: Resolve symlinks for cargo and xargo home. r=Emilgardis a=Alexhuszagh

Resolve symlinks for the xargo and cargo home (as well as the Nix store) prior to mounting, since they are mounted at a fixed location anyway. This is because podman mounts symlinks as root by default.

Closes #373.
Doesn't interfere with #920 and #942, although #942 might need work to ensure non-canonical paths are mounted with normal permissions.

Co-authored-by: Alex Huszagh <ahuszagh@gmail.com>
src/file.rs Show resolved Hide resolved
src/docker/shared.rs Outdated Show resolved Hide resolved
.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)]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops

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.
@Alexhuszagh
Copy link
Contributor Author

bors r=Emilgardis

@bors
Copy link
Contributor

bors bot commented Jul 16, 2022

Build succeeded:

@bors bors bot merged commit 59ec530 into cross-rs:main Jul 16, 2022
@Alexhuszagh Alexhuszagh deleted the canon_in_p branch July 16, 2022 22:20
@Emilgardis Emilgardis added this to the v0.3.0 milestone Aug 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't Canonicalize Paths
2 participants