Add tests for using file operations on directories

[squeek502]

The inverse of #5684. Contributes towards #5653

[squeek502]

I didn't include a read-only test because I was unsure about how to handle the differing errors per-platform, as mentioned here:

On Linux, when opening a file with read-only permissions, there is no way to determine whether the file descriptor is a directory handle without an additional syscall.

That is, open on Linux only returns EISDIR when "pathname refers to a directory and the access requested involved writing".

Seems like it'd be good to test the read-only case, but I'm not sure if that should involve changing the implementation of openFile to behave consistently across platforms (i.e. do a stat on Linux and return error.IsDir) or if the test case should have a per-platform implementation and the differing IsDir behavior be documented. This might be worth opening a new issue for, though.

EDIT: Opened an issue for this: #5732

[squeek502]

CI failures:

• WASI seems to be hitting ENOTCAPABLE somewhere in one of the createFile/deleteFile/readFileAlloc/openFile calls here
  • EDIT: It's in readFileAlloc, specifically:

    zig/lib/std/fs/file.zig

    Line 367 in 130c7fd

    try self.reader().readNoEof(buf);

• MacOS is failing somewhere (presumably an error other than IsDir is being returned from one of the functions), but the CI doesn't give any information on the failure for some reason and I don't have a Mac to test/debug it with

[kubkon]

So I can immediately help with macOS (or even BSD perhaps?). In macOS, unlinkat called on a path that is a directory but the user not being a super-user, will yield EPERM == 1 instead of EISDIR (yeah, I know, weird OS). So the way to fix this is to catch EPERM on macOS, and check if path is a directory using fstatat. If that's the case, remap the error to EISDIR.

If you want, I could either supply a patch directly to your branch, or could work with you at getting it right. For reference, here's how we do it in wasi-common (source in Rust): wasi_common::unlink_file.

In terms of WASI failure, I'll need to spend some time to debug it, but I won't be able to do it until tomorrow. I'm happy to help you with this if you want to wait until tomorrow?

[kubkon]

Really cool you've added this testcase BTW! We need a lot more of those, so thanks a bunch!

[squeek502]

Info dump for unlink and EPERM/EISDIR:

POSIX says only EPERM should be used, never EISDIR:

[EPERM] The file named by path is a directory, and either the calling process does not have appropriate privileges, or the implementation prohibits using unlink() on directories.

Mac/iOS:

[EPERM] The named file is a directory and the effective user ID of the process is not the super-user.
[EPERM] The directory containing the file is marked sticky, and neither the containing directory nor the file to be removed are owned by the effective user ID.

NetBSD:

[EPERM] The named file is a directory and the effective user ID of the process is not the super-user, the file system containing the file does not permit the use of unlink() on a directory, or the directory containing the file is marked sticky, and neither the containing directory nor the file to be removed are owned by the effective user ID.

FreeBSD:

[EISDIR] The named file is a directory.
[EPERM] The named file is a directory.
[EPERM] The named file has its immutable, undeletable or append-only flag set, see the chflags(2) manual page for more information.
[EPERM] The parent directory of the named file has its immutable or append-only flag set.
[EPERM] The directory containing the file is marked sticky, and neither the containing directory nor the file to be removed are owned by the effective user ID.

Linux:

[EISDIR] pathname refers to a directory. (This is the non-POSIX value returned by Linux since 2.1.132.)
[EPERM] The system does not allow unlinking of directories, or unlinking of directories requires privileges that the calling process doesn't have. (This is the POSIX prescribed error return; as noted above, Linux returns EISDIR for this case.)
[EPERM] (Linux only) The filesystem does not allow unlinking of files.
[EPERM or EACCES] The directory containing pathname has the sticky bit (S_ISVTX) set and the process's effective UID is neither the UID of the file to be deleted nor that of the directory containing it, and the process is not privileged (Linux: does not have the CAP_FOWNER capability).
[EPERM] The file to be unlinked is marked immutable or append-only. (See ioctl_iflags(2).)

---

In terms of WASI failure, I'll need to spend some time to debug it, but I won't be able to do it until tomorrow. I'm happy to help you with this if you want to wait until tomorrow?

No worries, sounds good.

[squeek502]

@kubkon did you have a chance to look into the ENOTCAPABLE WASI error?

[kubkon]

@squeek502 just a heads up, I've found the source of the problem. In WASI, when you want to call wasi.fd_read on a file descriptor, the fd is required to hold a wasi.RIGHT_FD_READ right. However, since in the test case we're opening a directory (even though we're using Dir.openFile), we are successful in opening it and hence receive a valid WASI fd. Next, we use this fd and call wasi.fd_read on it. However, since fd is a directory, it cannot hold a wasi.RIGHT_FD_READ right, and therefore, wasmtime throws ENOTCAPABLE since the fd doesn't hold the required right to call wasi.fd_read.

This isn't a bug in Zig's libstd, rather a subtlety in WASI implementation/spec. I've now submitted an issue about this in wasmtime repo: wasmtime#1935.

Oh, and I should probably add that some errno remapping as was the case with unlinkat in this PR could also be a possibility here (ENOTCAPABLE to EISDIR if fd is a directory), but before we commit in that direction, I think it'd be good to see what's decided over at wasmtime itself. Therefore, for the time being, until there is any agreement reached re this in wasmtime, I'd suggest we skip the readFileAlloc assertion when targeting WASI. We could also leave a tracking issue in Zig and leave that in as a comment. Thoughts?

[squeek502]

This isn't a bug in Zig's libstd, rather a subtlety in WASI implementation/spec. I've now submitted an issue about this in wasmtime repo: wasmtime#1935.

Makes sense. When investigating earlier I found bytecodealliance/wasmtime@0302f1a which seems like a similar situation.

Therefore, for the time being, until there is any agreement reached re this in wasmtime, I'd suggest we skip the readFileAlloc assertion when targeting WASI. We could also leave a tracking issue in Zig and leave that in as a comment. Thoughts?

Seems reasonable.

[squeek502]

@andrewrk This should be good to go now, as long as you think the change in 14c3c47 is the right way to go.

[andrewrk]

We're probably going to need to adjust the API in the future so that the callsite can issue a deleteFile operation which is willing to tolerate the ambiguity of error.AccessDenied vs error.IsDir, in order to save a syscall. But that can be a separate issue - this is definitely an improvement over status quo. Thanks for working on better test coverage!