From db4ac2e6964aa4a2d3f8e7aa986020ea883ed2a1 Mon Sep 17 00:00:00 2001 From: James Bornholt Date: Fri, 30 Jun 2023 16:21:22 +0000 Subject: [PATCH] Explicitly prevent mounting over an existing mount It's valid to mount multiple file systems to the same directory, but doing so requires the first one to be mounted read-write, which is why this wasn't a problem for us until #327. It seems libfuse2's version of fusermount explicitly checked this(?), but libfuse3 no longer rejects it. In principle this might be something we'd want to allow, but I think the less surprising/error-prone customer experience is to refuse to do it, so let's explicitly forbid it. Signed-off-by: James Bornholt --- Cargo.lock | 68 +++++++++++++++++++++++++++++++++++--- mountpoint-s3/Cargo.toml | 1 + mountpoint-s3/src/main.rs | 43 +++++++++++++++++++----- mountpoint-s3/tests/cli.rs | 4 +-- 4 files changed, 101 insertions(+), 15 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 331b65c2d..2693b08b9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1669,7 +1669,7 @@ checksum = "adcf93614601c8129ddf72e2d5633df827ba6551541c6d8c59520a371475be1f" dependencies = [ "hermit-abi 0.3.1", "io-lifetimes", - "rustix", + "rustix 0.37.20", "windows-sys 0.48.0", ] @@ -1780,6 +1780,12 @@ dependencies = [ "vcpkg", ] +[[package]] +name = "linux-raw-sys" +version = "0.1.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "f051f77a7c8e6957c0696eac88f26b0117e54f52d3fc682ab19397a8812846a4" + [[package]] name = "linux-raw-sys" version = "0.3.8" @@ -1911,6 +1917,7 @@ dependencies = [ "nix", "once_cell", "predicates 2.1.5", + "procfs", "proptest", "proptest-derive", "rand", @@ -2341,6 +2348,19 @@ dependencies = [ "unicode-ident", ] +[[package]] +name = "procfs" +version = "0.15.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "943ca7f9f29bab5844ecd8fdb3992c5969b6622bb9609b9502fef9b4310e3f1f" +dependencies = [ + "bitflags", + "byteorder", + "hex", + "lazy_static", + "rustix 0.36.14", +] + [[package]] name = "proptest" version = "1.2.0" @@ -2564,6 +2584,20 @@ version = "0.1.3" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "a45e222b29b1c72df440525c4158fa44fda5347a95f14dc47bb37fd7c1969b43" +[[package]] +name = "rustix" +version = "0.36.14" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "14e4d67015953998ad0eb82887a0eb0129e18a7e2f3b7b0f6c422fddcd503d62" +dependencies = [ + "bitflags", + "errno", + "io-lifetimes", + "libc", + "linux-raw-sys 0.1.4", + "windows-sys 0.45.0", +] + [[package]] name = "rustix" version = "0.37.20" @@ -2574,7 +2608,7 @@ dependencies = [ "errno", "io-lifetimes", "libc", - "linux-raw-sys", + "linux-raw-sys 0.3.8", "windows-sys 0.48.0", ] @@ -2919,7 +2953,7 @@ dependencies = [ "cfg-if", "fastrand", "redox_syscall", - "rustix", + "rustix 0.37.20", "windows-sys 0.48.0", ] @@ -3507,7 +3541,7 @@ version = "0.48.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "e686886bc078bc1b0b600cac0147aadb815089b6e4da64016cbd754b6342700f" dependencies = [ - "windows-targets", + "windows-targets 0.48.1", ] [[package]] @@ -3525,13 +3559,37 @@ dependencies = [ "windows_x86_64_msvc 0.42.2", ] +[[package]] +name = "windows-sys" +version = "0.45.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "75283be5efb2831d37ea142365f009c02ec203cd29a3ebecbc093d52315b66d0" +dependencies = [ + "windows-targets 0.42.2", +] + [[package]] name = "windows-sys" version = "0.48.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "677d2418bec65e3338edb076e806bc1ec15693c5d0104683f2efe857f61056a9" dependencies = [ - "windows-targets", + "windows-targets 0.48.1", +] + +[[package]] +name = "windows-targets" +version = "0.42.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8e5180c00cd44c9b1c88adb3693291f1cd93605ded80c250a75d472756b4d071" +dependencies = [ + "windows_aarch64_gnullvm 0.42.2", + "windows_aarch64_msvc 0.42.2", + "windows_i686_gnu 0.42.2", + "windows_i686_msvc 0.42.2", + "windows_x86_64_gnu 0.42.2", + "windows_x86_64_gnullvm 0.42.2", + "windows_x86_64_msvc 0.42.2", ] [[package]] diff --git a/mountpoint-s3/Cargo.toml b/mountpoint-s3/Cargo.toml index 7c0548969..9a6077312 100644 --- a/mountpoint-s3/Cargo.toml +++ b/mountpoint-s3/Cargo.toml @@ -31,6 +31,7 @@ time = { version = "0.3.17", features = ["macros", "formatting"] } const_format = "0.2.30" home = "0.5.4" serde_json = "1.0.95" +procfs = { version = "0.15.1", default-features = false } [dev-dependencies] assert_cmd = "2.0.6" diff --git a/mountpoint-s3/src/main.rs b/mountpoint-s3/src/main.rs index fb4a18c01..3cbcb3a52 100644 --- a/mountpoint-s3/src/main.rs +++ b/mountpoint-s3/src/main.rs @@ -251,14 +251,6 @@ impl CliArgs { fn main() -> anyhow::Result<()> { let args = CliArgs::parse(); - // validate mount point - if !args.mount_point.exists() || !args.mount_point.is_dir() { - return Err(anyhow!( - "Mount point {} does not exist or it is not a directory", - args.mount_point.display() - )); - } - if args.foreground { init_tracing_subscriber(args.foreground, args.log_directory.as_deref()) .context("failed to initialize logging")?; @@ -370,6 +362,8 @@ fn main() -> anyhow::Result<()> { fn mount(args: CliArgs) -> anyhow::Result { const DEFAULT_TARGET_THROUGHPUT: f64 = 10.0; + validate_mount_point(&args.mount_point)?; + let addressing_style = args.addressing_style(); let endpoint = args .endpoint_url @@ -571,6 +565,39 @@ fn get_maximum_network_throughput(ec2_instance_type: &str) -> anyhow::Result) -> anyhow::Result<()> { + let mount_point = path.as_ref(); + + if !mount_point.exists() { + return Err(anyhow!("mount point {} does not exist", mount_point.display())); + } + + if !mount_point.is_dir() { + return Err(anyhow!("mount point {} is not a directory", mount_point.display())); + } + + #[cfg(target_os = "linux")] + { + use procfs::process::Process; + + // This is a best-effort validation, so don't fail if we can't read /proc/self/mountinfo for + // some reason. + let mounts = match Process::myself().and_then(|me| me.mountinfo()) { + Ok(mounts) => mounts, + Err(e) => { + tracing::debug!("failed to read mountinfo, not checking for existing mounts: {e:?}"); + return Ok(()); + } + }; + + if mounts.iter().any(|mount| mount.mount_point == path.as_ref()) { + return Err(anyhow!("mount point {} is already mounted", path.as_ref().display())); + } + } + + Ok(()) +} + #[cfg(test)] mod tests { use super::*; diff --git a/mountpoint-s3/tests/cli.rs b/mountpoint-s3/tests/cli.rs index 8636a8e1d..23c2e83d2 100644 --- a/mountpoint-s3/tests/cli.rs +++ b/mountpoint-s3/tests/cli.rs @@ -12,7 +12,7 @@ fn mount_point_doesnt_exist() -> Result<(), Box> { let mut cmd = Command::cargo_bin("mount-s3")?; cmd.arg("test-bucket").arg("test/dir"); - let error_message = "Mount point test/dir does not exist or it is not a directory"; + let error_message = "mount point test/dir does not exist"; cmd.assert().failure().stderr(predicate::str::contains(error_message)); Ok(()) @@ -25,7 +25,7 @@ fn mount_point_isnt_dir() -> Result<(), Box> { cmd.arg("test-bucket").arg(file.path()); let error_message = format!( - "Mount point {} does not exist or it is not a directory", + "mount point {} does not exist", file.path().display() ); cmd.assert().failure().stderr(predicate::str::contains(error_message));