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

Explicitly prevent mounting over an existing mount #348

Merged
merged 1 commit into from
Jun 30, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
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 }
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this platform-specific?


[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
Copy link
Contributor

Choose a reason for hiding this comment

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

Are we okay moving this validation to the background process?

Copy link
Member Author

Choose a reason for hiding this comment

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

We should be -- it was the only validation we were doing in the foreground process previously, and we shouldn't be doing any work at all before the fork.

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes sense, but we need to update the tests.

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