Skip to content

Disallow mounting folders on the guest's root for WASIX modules #5475

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Arshia001
Copy link
Member

No description provided.

@Arshia001 Arshia001 requested a review from syrusakbary as a code owner March 12, 2025 11:34
@Arshia001 Arshia001 requested review from theduke and charmitro March 12, 2025 11:34
Copy link

promptless bot commented Mar 12, 2025

📝 Documentation updates detected! A separate PR for documentation updates has been made here: wasmerio/docs.wasmer.io#122

Comment on lines +491 to +495
if mount_path.as_path() == Path::new("/") {
bail!(
"The \"{package}\" package wants to mount a volume at \"/\", but that's not allowed",
);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a helper function for this? It is used in fs v2 and v3. Something like (not tested),

fn check_volume_path(
    mount_path: &PathBuf,
    package: &str,
) -> Result<(), Error> {
    if mount_path.as_path() == Path::new("/") {
        bail!(
            "The \"{package}\" package wants to mount a volume at \"/\", but that's not allowed",
        );
    }
    Ok(())
}

Copy link
Member Author

Choose a reason for hiding this comment

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

if only the code was that clean to begin with... it has been my experience that attempting to make clean implementations in code that's not clean by design usually leads to more complexity down the line, so when I see duplicate code, I also duplicate mine, to at least keep the code consistently unclean.

guest: if is_wasix {
MAPPED_CURRENT_DIR_DEFAULT_PATH.to_string()
} else {
"/".to_string()
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd love a const about "/", const ROOT_PATH: &str = "/"; or similar. What do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'd happily approve of someone else doing that XD / is well-known enough to warrant not giving it a const of its own though.

@charmitro
Copy link
Contributor

Also,

Tested-by: Charalampos Mitrodimas [email protected]

@syrusakbary
Copy link
Member

This PR might have some implications, that we don't want. I remember other programs using mapdir=.:/ to make Python work for example. We may want to use that directory instead (and not mount any other dir) if / is provided. Lets follow up in a sync

@Arshia001
Copy link
Member Author

Arshia001 commented Mar 12, 2025

@syrusakbary in fact, that's exactly what this PR is trying to prevent. Mounting things on / messes up a lot of assumptions in WASIX, including:

  • We put commands from packages in /bin and /usr/bin. If / is mounted, those will get overwritten and be inaccessible.
  • Packages mount volumes, which contain necessary files without which they won't run. Python is in fact a great example of this. If I were to just mount something on /, I'd lose all the data files that came with the python package, ending up with a broken application.
    • Same with PHP, if I mount a website's root on /, I lose all the openssl data files and HTTPS will be broken for example.
    • I do believe python was the motivation behind the custom behavior of mounting . on /home with --dir .. I think that's what you're thinking of as well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants