Skip to content

Commit

Permalink
Explicitly prevent mounting over an existing mount
Browse files Browse the repository at this point in the history
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 <[email protected]>
  • Loading branch information
jamesbornholt committed Jun 30, 2023
1 parent d71d4b4 commit 486c145
Show file tree
Hide file tree
Showing 4 changed files with 105 additions and 19 deletions.
68 changes: 63 additions & 5 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions mountpoint-s3/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,9 @@ const_format = "0.2.30"
home = "0.5.4"
serde_json = "1.0.95"

[target.'cfg(target_os = "linux")'.dependencies]
procfs = { version = "0.15.1", default-features = false }

[dev-dependencies]
assert_cmd = "2.0.6"
assert_fs = "1.0.9"
Expand Down
43 changes: 35 additions & 8 deletions mountpoint-s3/src/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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")?;
Expand Down Expand Up @@ -370,6 +362,8 @@ fn main() -> anyhow::Result<()> {
fn mount(args: CliArgs) -> anyhow::Result<FuseSession> {
const DEFAULT_TARGET_THROUGHPUT: f64 = 10.0;

validate_mount_point(&args.mount_point)?;

let addressing_style = args.addressing_style();
let endpoint = args
.endpoint_url
Expand Down Expand Up @@ -571,6 +565,39 @@ fn get_maximum_network_throughput(ec2_instance_type: &str) -> anyhow::Result<f64
.ok_or_else(|| anyhow!("no throughput configuration for EC2 instance type {ec2_instance_type}"))
}

fn validate_mount_point(path: impl AsRef<Path>) -> 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::*;
Expand Down
10 changes: 4 additions & 6 deletions mountpoint-s3/tests/cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,22 +12,20 @@ fn mount_point_doesnt_exist() -> Result<(), Box<dyn std::error::Error>> {
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(())
}

#[test]
fn mount_point_isnt_dir() -> Result<(), Box<dyn std::error::Error>> {
let file = assert_fs::NamedTempFile::new("test/file.txt")?;
let file = assert_fs::NamedTempFile::new("file.txt")?;
fs::write(file.path(), b"hello")?;
let mut cmd = Command::cargo_bin("mount-s3")?;

cmd.arg("test-bucket").arg(file.path());
let error_message = format!(
"Mount point {} does not exist or it is not a directory",
file.path().display()
);
let error_message = format!("mount point {} is not a directory", file.path().display());
cmd.assert().failure().stderr(predicate::str::contains(error_message));

Ok(())
Expand Down

0 comments on commit 486c145

Please sign in to comment.