lib/wasi: try quick fix for WasiFS::get_inode_at_path

[waynr]

Description

I've been working on getting wasix-libc's readdir function working but have
been receiving Errno::Noent responses from path_filestat_get (found in
lib/wasi/src/syscalls/wasi/path_filestat_get).

My understanding is that the path_filestat_get syscall takes a file
descriptor for the directory and a relative path for the basename of the file
intended to be stat'ed; at least, this is how it appears to be used in
wasix-libc's readdir implementation and how downstream libc users of
readdir more generally treat it.

With the change in this PR, get_inode_path_at no longer ignores the base fd
but instead uses it to find the file at the given relative path. When
get_inode_path_at is used in the context of path_filestate_get, this seems
to be the desired/correct behavior.

[theduke]

@waynr need to run cargo fmt --all.

[theduke]

@john-sharratt this looks correct to me, what are your concerns?

[john-sharratt]

My main concern is that I suspect you will find that this also works in your usecase....

pub(crate) fn get_inode_at_path(
    &self,
    inodes: &WasiInodes,
    base: WasiFd,
    path: &str,
    follow_symlinks: bool,
) -> Result<InodeGuard, Errno> {
    let start_inode = if self.is_wasix.load(Ordering::Acquire) {
        let (cur_inode, _) = self.get_current_dir(inodes, base)?;
        cur_inode
    } else {
        self.get_fd_inode(base)?
    };
    self.get_inode_at_path_inner(inodes, start_inode, path, 0, follow_symlinks)
}

Or at least it did with this test...

john@AlienWorld:/prog/wasmer/lib/cli$ cargo run --features compiler,cranelift,debug -- run --use sharrattj/coreutils sharrattj/bash
    Finished dev [unoptimized + debuginfo] target(s) in 0.23s
     Running `/prog/wasmer/target/debug/wasmer run --use sharrattj/coreutils sharrattj/bash`
bash.wasm-5.1# ls

Reason is that the logic around the base node being an absolute path isn't a valid code path as far as I can see (i.e. it is always false)

[waynr]

Reason is that the logic around the base node being an absolute path isn't a valid code path as far as I can see (i.e. it is always false)

Yeah, this occurred to me, happy to remove that part of the logic.

That being said, I don't see how the current master branch approach would ever be valid (except in the edge case where the base directory and the current working directory are the same) given the intent specified in the comment on this method:

    /// gets a host file from a base directory and a path
    /// this function ensures the fs remains sandboxed

Something else I don't understand is how is_wasix plays into this. Why should we ignore the given base directory for wasix but not for wasi?

FYI, I say that the base directory is (essentially) ignored in that first logic branch because that's the behavior I observe in master branch -- this function seems to always look for path in the current working directory even though readdir (via fd_filestat_get and fd_filestat_get_internal which call this function) passes in a file descriptor relative to which path should be interpreted.

Something else is off about the master branch code in my view. Let's consider the original logic:

        let start_inode = if !path.starts_with('/') && self.is_wasix.load(Ordering::Acquire) {
            let (cur_inode, _) = self.get_current_dir(inodes, base)?;
            cur_inode
        } else {
            self.get_fd_inode(base)?
        };

On the one hand we say if !path.starts_with('/') then we need the current directory. Presumably that's because if path is absolute then it's not relative to anything. Yet the other branch gives us a start_inode anyway. Does that make sense? It's saying, if path is absolute, then interpret it relative to base; if not, interpret it relative to the current directory. If it's absolute then we should never interpret it relative to either the current directory or the base directory, right?

[theduke]

@john-sharratt and @waynr had some discussions about this.

I'm not fully up to date there, but I think the new code should be agreeable, and we also have two new test cases to cover certain conditions.