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

Path normalization causes more accepted file paths #2249

Open
yagehu opened this issue Jun 13, 2024 · 2 comments
Open

Path normalization causes more accepted file paths #2249

yagehu opened this issue Jun 13, 2024 · 2 comments
Labels
bug Something isn't working

Comments

@yagehu
Copy link
Contributor

yagehu commented Jun 13, 2024

Describe the bug
Due to the path.Clean() call in the atPath() implementation, some paths that are rejected by other runtimes are OK in Wazero. For example, given a preopen directory fd and the path a/../a, path.Clean() would normalize away the .. and produce a as the final path. In runtimes such as Wasmtime and WAMR, since the first a/ does not exist, a call involving that path returns noent (44).

To Reproduce
This snippet will fail with Wasmtime, WAMR, WasmEdge, Wasmer. But Wazero creates the link a pointing to link successfully.

fn main() {
    unsafe {
        let base_fd = 3;

        wasi::path_symlink("link", base_fd, "a/../a").unwrap();
    }
}

Environment (please complete the relevant information):

  • Go version: 1.22.3
  • wazero Version: bf42c25
  • Host architecture: amd64
  • Runtime mode: compiler

Additional context
The only other runtime that has the same behavior is Node with uvwasi. That's because uvwasi uses libuv which doesn't have an openat(2) equivalent (yet!).

The wasi-filesystem spec describes a path resolution implementation that conflicts with the usage of path.Clean and similar path normalization strategies.

@yagehu yagehu added the bug Something isn't working label Jun 13, 2024
@evacchi
Copy link
Contributor

evacchi commented Jun 14, 2024

well, personally I am not against converging with other implementations, I suppose this used to be underspecified

@yagehu
Copy link
Contributor Author

yagehu commented Jun 14, 2024

well, personally I am not against converging with other implementations, I suppose this used to be underspecified

I can take a stab at it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants