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

Improper resolution of .. components in symlinks #9272

Closed
neild opened this issue Sep 17, 2024 · 7 comments · Fixed by #9307
Closed

Improper resolution of .. components in symlinks #9272

neild opened this issue Sep 17, 2024 · 7 comments · Fixed by #9307
Labels
bug Incorrect behavior in the current implementation that needs fixing

Comments

@neild
Copy link

neild commented Sep 17, 2024

I believe I've run across a bug in wasmtime's handling of symlink targets which include "..". I've reproduced this using the Go program below which calls path_open on a path and reports the result. It's not the simplest test program, but it avoids dependencies and I have a Go compiler handy.

Simple case, no bug:

$ echo foo > file
$ ln -s file link
$ GOOS=wasip1 GOARCH=wasm go run main.go link
errno 0

The bug shows up when resolving a link with a target of "..":

$ echo foo > target
$ mkdir b
$ (cd b && ln -s .. up)
$ cat b/up/target
foo
$ GOOS=wasip1 GOARCH=wasm go run main.go b/up
errno 0
$ GOOS=wasip1 GOARCH=wasm go run main.go b/up/target
Not a directory

I'm not sure exactly what is going awry, but the "not a directory" error is clearly wrong. Note that we can open b/up (resolves to . as expected). The problem seems to be specific to links that end in ..: if we change the link target to ../a, everything works as expected:

$ mkdir a
$ echo bar > a/target
$ mkdir b
$ (cd b && ln -s ../a upa)
$ cat b/upa/target
bar
$ GOOS=wasip1 GOARCH=wasm go run main.go b/upa/target
errno 0

Test program:

package main

import (
        "fmt"
        "os"
        "syscall"
        "unsafe"
)

//go:wasmimport wasi_snapshot_preview1 path_open
//go:noescape
func path_open(rootFD int32, dirflags uint32, path unsafe.Pointer, pathLen uint32, oflags uint32, fsRightsBase uint64, fsRightsInheriting uint64, fsFlags uint32, fd unsafe.Pointer) uint32

const LOOKUP_SYMLINK_FOLLOW = 1

func main() {
        f, err := os.Open(".")
        if err != nil {
                panic(err)
        }

        filename := os.Args[1]
        var fd int32
        errno := path_open(
                int32(f.Fd()),
                syscall.LOOKUP_SYMLINK_FOLLOW,
                unsafe.Pointer(unsafe.StringData(filename)),
                uint32(len(filename)),
                0,
                syscall.RIGHT_FD_READ,
                syscall.RIGHT_FD_READ,
                0,
                unsafe.Pointer(&fd),
        )
        fmt.Println(syscall.Errno(errno))
}
@neild neild added the bug Incorrect behavior in the current implementation that needs fixing label Sep 17, 2024
@alexcrichton
Copy link
Member

Thanks for the report! Would you be able to gist exactly what wasmtime command is being used? You're using go run but it's not clear to me what exactly wasmtime is doing. Locally I can't seem to reproduce this with what I believe is an equivalent Rust program:

$ cat foo.rs
fn main() {
    println!(
        "{:?}",
        std::fs::File::open(std::env::args().nth(1).unwrap())
    );
}
$ rustc foo.rs --target wasm32-wasip1
$ mkdir a
$ echo bar > a/target
$ mkdir b
$ (cd b && ln -s ../a upa)
$ cat b/upa/target
bar
$ wasmtime --dir . foo.wasm b/upa/target
Ok(File { fd: 4 })
$ ls
a  b  foo.rs  foo.wasm
$ rm -rf a b
$ echo foo > target
$ mkdir b
$ (cd b && ln -s .. up)
$ cat b/up/target
foo
$ wasmtime --dir . foo.wasm b/up
Ok(File { fd: 4 })
$ wasmtime --dir . foo.wasm b/up/target
Ok(File { fd: 4 })
$ wasmtime --dir . foo.wasm nonexistent
Err(Os { code: 44, kind: NotFound, message: "No such file or directory" })

That may mean that the difference boils down to what Rust and Go are doing, and so the next step here would be to narrow in on that and see what's going on. If you're able to upload the Go-generated wasm binary here that would also help too

@neild
Copy link
Author

neild commented Sep 20, 2024

The wasmtime command is from here: https://go.googlesource.com/go/+/refs/tags/go1.23.0/misc/wasm/go_wasip1_wasm_exec

wasmtime run --dir=/ --env PWD="$PWD" --env PATH="$PATH" -W max-wasm-stack=1048576 main.wasm b/up/target

Wasm binary I'm using:
main.wasm.gz

@alexcrichton
Copy link
Member

Hm I actually can't seem to reproduce with the binary that you provided. I get:

$ wasmtime run --dir=/ --env PWD="$PWD" --env PATH="$PATH" -W max-wasm-stack=1048576 main.wasm b/up/target
errno 0

which I think means that it's working?

I noticed above sometimes the symlink was called "up" and sometimes it was "upa". I mixed the two up and thought Rust was working and Go wasn't but then when I fixed the name they both worked. Mind double-checking on your end too?

@neild
Copy link
Author

neild commented Sep 20, 2024

Setup:

$ echo foo > target
$ mkdir b
$ (cd b && ln -s .. up)
$ cat b/up/target
foo

And running:

$ wasmtime --version
wasmtime 25.0.0 (0b195ef5d 2024-09-20)
$ wasmtime run --dir=/ --env PWD="$PWD" --env PATH="$PATH" -W max-wasm-stack=1048576 main.wasm b/up/target
Not a directory

This is on macOS. I had thought I'd observed this on macOS and Linux, but I just rechecked and I can't reproduce on Linux. So perhaps macOS-specific?

Or possibly even Linux version specific, if wasmtime uses openat2 with RESOLVE_BENEATH when available.

@alexcrichton
Copy link
Member

Aha the plot thickens! I was indeed testing on Linux. I'll work next week on digging more into macOS, as I suspect you're right this is macOS-specific behavior.

@sunfishcode
Copy link
Member

I can reproduce the problem using cap-std directly with this example:

use std::io::Read;

fn main() {
    let path = "b/up/target";
    let dir = cap_std::fs::Dir::open_ambient_dir(".", cap_std::ambient_authority()).unwrap();
    let mut file = dir.open(path).unwrap();
    let mut s = String::new();
    file.read_to_string(&mut s).unwrap();
    eprintln!("read: {:?}", s);
}

If I modify cap-std to avoid using openat2, then I can reproduce the "Not a directory" error even on Linux. I'll investigate further.

sunfishcode added a commit to bytecodealliance/cap-std that referenced this issue Sep 21, 2024
When `..` appears at the end of a symlink target, but is not at the end
of the full path target, don't mark the path as being expected to open
a directory.

This fixes the reduced testcase in bytecodealliance/wasmtime#9272.
sunfishcode added a commit to bytecodealliance/cap-std that referenced this issue Sep 24, 2024
* Fix handling of `..` paths in symlinks.

When `..` appears at the end of a symlink target, but is not at the end
of the full path target, don't mark the path as being expected to open
a directory.

This fixes the reduced testcase in bytecodealliance/wasmtime#9272.

* Update CI to macos-12.
sunfishcode added a commit to sunfishcode/wasmtime that referenced this issue Sep 24, 2024
cap-std 3.3.0 contains bytecodealliance/cap-std#366, which fixes bytecodealliance#9272,
which is about the handling of `..` in symlink destinations.
@sunfishcode
Copy link
Member

I've now submitted #9307 which should fix this issue.

github-merge-queue bot pushed a commit that referenced this issue Sep 25, 2024
* Update to cap-std 3.3.0.

cap-std 3.3.0 contains bytecodealliance/cap-std#366, which fixes #9272,
which is about the handling of `..` in symlink destinations.

* Add wildcard vet of pulley

---------

Co-authored-by: Alex Crichton <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Incorrect behavior in the current implementation that needs fixing
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants