sysfs: drops os.File special casing for fs.FS to pass wasi-testsuite (#…

…1174)

This adds a wazero adapter which passes wasi-testsuite 100pct on darwin,
linux and windows. While the main change was adding inodes to the wasi
`fd_readdir` dirents, there was a lot of incidental work needed.

Most of the work was troubleshooting in nature, around windows
specifically, but also wrapping of files. This backfills a lot of tests
and reworked how wrapping works, particularly around windows.

To accommodate this, we drop `os.File` special casing except for
`sysfs.DirFS`

Signed-off-by: Adrian Cole <[email protected]>

.github/wasi_testsuite_adapter.py

@@ -0,0 +1,47 @@
+# adapter for wazero until/unless https://github.com/WebAssembly/wasi-testsuite/pull/55
+
+import argparse
+import subprocess
+import sys
+import os
+import shlex
+
+# shlex.split() splits according to shell quoting rules
+WAZERO = shlex.split(os.getenv("TEST_RUNTIME_EXE", "wazero"))
+
+parser = argparse.ArgumentParser()
+parser.add_argument("--version", action="store_true")
+parser.add_argument("--test-file", action="store")
+parser.add_argument("--arg", action="append", default=[])
+parser.add_argument("--env", action="append", default=[])
+parser.add_argument("--dir", action="append", default=[])
+
+args = parser.parse_args()
+
+if args.version:
+    version = subprocess.run(
+        WAZERO + ["version"], capture_output=True, text=True
+    ).stdout.strip()
+    if version == "dev":
+        version = "0.0.0"
+    print("wazero", version)
+    sys.exit(0)
+
+TEST_FILE = args.test_file
+TEST_DIR = os.path.dirname(TEST_FILE)
+PROG_ARGS = []
+if args.arg:
+    PROG_ARGS = ["--"] + args.arg
+ENV_ARGS = [f"-env={e}" for e in args.env]
+cwd = os.getcwd()
+DIR_ARGS = [f"-mount={cwd}/{dir}:{dir}" for dir in args.dir]
+
+PROG = (
+    WAZERO
+    + ["run", "-hostlogging=filesystem"]
+    + ENV_ARGS
+    + DIR_ARGS
+    + [TEST_FILE]
+    + PROG_ARGS
+)
+sys.exit(subprocess.run(PROG, cwd=TEST_DIR).returncode)

.github/workflows/integration.yaml

@@ -279,10 +279,9 @@ jobs:
       - name: Checkout wasi-testsuite
         uses: actions/checkout@v3
         with:
-          # TODO: after https://github.com/WebAssembly/wasi-testsuite/pull/55 is merged and synced,
-          # Use the upstream repo and point to prod/testsuite-base branch (or specific commit in there).
-          repository: tetratelabs/wasi-testsuite
-          ref: wazero-python-adapter
+          repository: WebAssembly/wasi-testsuite
+          ref: prod/testsuite-base
+          path: wasi-testsuite
 
       - name: Initialize Python environment
         uses: actions/setup-python@v4
@@ -291,14 +290,15 @@ jobs:
           cache: pip
 
       - name: Install dependencies
-        working-directory: test-runner
+        working-directory: wasi-testsuite/test-runner
         run: |
           python3 -m pip install -r requirements.txt
 
       - name: Run all wasi-testsuite
+        working-directory: wasi-testsuite
         run: |
           python3 test-runner/wasi_test_runner.py \
             -t ./tests/assemblyscript/testsuite/ \
             ./tests/c/testsuite/ \
             ./tests/rust/testsuite/ \
-            -r ./adapters/wazero.py
+            -r ../.github/wasi_testsuite_adapter.py

RATIONALE.md

@@ -465,6 +465,24 @@ Unfortunately, (WASI Snapshot Preview 1)[https://github.com/WebAssembly/WASI/blo
 This section describes how Wazero interprets and implements the semantics of several WASI APIs that may be interpreted differently by different wasm runtimes.
 Those APIs may affect the portability of a WASI application.
 
+### Why don't we attempt to pass wasi-testsuite on user-defined `fs.FS`?
+
+While most cases work fine on an `os.File` based implementation, we won't
+promise wasi-testsuite compatibility on user defined wrappers of `os.DirFS`.
+The only option for real systems is to use our `sysfs.FS`.
+
+There are a lot of areas where windows behaves differently, despite the
+`os.File` abstraction. This goes well beyond file locking concerns (e.g.
+`EBUSY` errors on open files). For example, errors like `ACCESS_DENIED` aren't
+properly mapped to `EPERM`. There are trickier parts too. `FileInfo.Sys()`
+doesn't return enough information to build inodes needed for WASI. To rebuild
+them requires the full path to the underlying file, not just its directory
+name, and there's no way for us to get that information. At one point we tried,
+but in practice things became tangled and functionality such as read-only
+wrappers became untenable. Finally, there are version-specific behaviors which
+are difficult to maintain even in our own code. For example, go 1.20 opens
+files in a different way than versions before it.
+
 ### Why aren't WASI rules enforced?
 
 The [snapshot-01](https://github.com/WebAssembly/WASI/blob/snapshot-01/phases/snapshot/docs.md) version of WASI has a

imports/wasi_snapshot_preview1/fs.go

@@ -851,13 +851,14 @@ func fdReaddirFn(_ context.Context, mod api.Module, params []uint64) Errno {
 
 	// Check if we have maxDirEntries, and read more from the FS as needed.
 	if entryCount := len(dirents); entryCount < maxDirEntries {
+		// Note: platform.Readdir does not return io.EOF as it is
+		// inconsistently returned (e.g. darwin does, but linux doesn't).
 		l, err := platform.Readdir(rd, maxDirEntries-entryCount)
 		if errno = ToErrno(err); errno != ErrnoSuccess {
 			return errno
 		}
 
-		// Zero length read is possible on an empty directory or on io.EOF,
-		// which coerces to nil in WASI because there's no Errno for it.
+		// Zero length read is possible on an empty or exhausted directory.
 		if len(l) > 0 {
 			dir.CountRead += uint64(len(l))
 			dirents = append(dirents, l...)
@@ -893,18 +894,26 @@ func fdReaddirFn(_ context.Context, mod api.Module, params []uint64) Errno {
 	return ErrnoSuccess
 }
 
-// dotDirents returns "." and "..", where "." has a real stat because
-// wasi-testsuite does inode validation.
+// dotDirents returns "." and "..", where "." because wasi-testsuite does inode
+// validation.
 func dotDirents(f *sys.FileEntry) ([]*platform.Dirent, error) {
-	ino, ft, err := f.CachedStat()
+	dotIno, ft, err := f.CachedStat()
 	if err != nil {
 		return nil, err
 	} else if ft.Type() != fs.ModeDir {
 		return nil, syscall.ENOTDIR
 	}
+	dotDotIno := uint64(0)
+	if !f.IsPreopen && f.Name != "." {
+		var st platform.Stat_t
+		if err = f.FS.Stat(pathutil.Dir(f.Name), &st); err != nil {
+			return nil, err
+		}
+		dotDotIno = st.Ino
+	}
 	return []*platform.Dirent{
-		{Name: ".", Ino: ino, Type: fs.ModeDir},
-		{Name: "..", Type: fs.ModeDir},
+		{Name: ".", Ino: dotIno, Type: fs.ModeDir},
+		{Name: "..", Ino: dotDotIno, Type: fs.ModeDir},
 	}, nil
 }
 
@@ -1022,7 +1031,7 @@ func writeDirents(
 		e := dirents[i]
 		nameLen := uint32(len(e.Name))
 
-		writeDirent(buf[pos:], d_next, nameLen, e.IsDir())
+		writeDirent(buf[pos:], d_next, e.Ino, nameLen, e.IsDir())
 		pos += DirentSize
 
 		copy(buf[pos:], e.Name)
@@ -1037,16 +1046,16 @@ func writeDirents(
 	// Write a dirent without its name
 	dirent := make([]byte, DirentSize)
 	e := dirents[i]
-	writeDirent(dirent, d_next, uint32(len(e.Name)), e.IsDir())
+	writeDirent(dirent, d_next, e.Ino, uint32(len(e.Name)), e.IsDir())
 
 	// Potentially truncate it
 	copy(buf[pos:], dirent)
 }
 
 // writeDirent writes DirentSize bytes
-func writeDirent(buf []byte, dNext uint64, dNamlen uint32, dType bool) {
+func writeDirent(buf []byte, dNext uint64, ino uint64, dNamlen uint32, dType bool) {
 	le.PutUint64(buf, dNext)        // d_next
-	le.PutUint64(buf[8:], 0)        // no d_ino
+	le.PutUint64(buf[8:], ino)      // d_ino
 	le.PutUint32(buf[16:], dNamlen) // d_namlen
 
 	filetype := FILETYPE_REGULAR_FILE

imports/wasi_snapshot_preview1/fs_test.go

@@ -9,6 +9,7 @@ import (
 	"math"
 	"os"
 	"path"
+	"runtime"
 	"testing"
 	"time"
 
@@ -20,6 +21,7 @@ import (
 	"github.com/tetratelabs/wazero/internal/sys"
 	"github.com/tetratelabs/wazero/internal/sysfs"
 	"github.com/tetratelabs/wazero/internal/testing/require"
+	"github.com/tetratelabs/wazero/internal/u64"
 	. "github.com/tetratelabs/wazero/internal/wasi_snapshot_preview1"
 	"github.com/tetratelabs/wazero/internal/wasm"
 )
@@ -4451,10 +4453,69 @@ func requireOpenFile(t *testing.T, tmpDir string, pathName string, data []byte,
 	return mod, fd, log, r
 }
 
+// Test_fdReaddir_dotEntriesHaveRealInodes because wasi-testsuite requires it.
+func Test_fdReaddir_dotEntriesHaveRealInodes(t *testing.T) {
+	if runtime.GOOS == "windows" && !platform.IsGo120 {
+		t.Skip("windows before go 1.20 has trouble reading the inode information on directories.")
+	}
+
+	root := t.TempDir()
+	mod, r, _ := requireProxyModule(t, wazero.NewModuleConfig().
+		WithFSConfig(wazero.NewFSConfig().WithDirMount(root, "/")),
+	)
+	defer r.Close(testCtx)
+
+	mem := mod.Memory()
+
+	fsc := mod.(*wasm.CallContext).Sys.FS()
+	preopen := fsc.RootFS()
+
+	const readDirTarget = "dir"
+	mem.Write(0, []byte(readDirTarget))
+	requireErrnoResult(t, ErrnoSuccess, mod, PathCreateDirectoryName,
+		uint64(sys.FdPreopen), uint64(0), uint64(len(readDirTarget)))
+
+	// Open the directory, before writing files!
+	dirFd, err := fsc.OpenFile(preopen, readDirTarget, os.O_RDONLY, 0)
+	require.NoError(t, err)
+
+	// get the real inode of the current directory
+	var st platform.Stat_t
+	require.NoError(t, preopen.Stat(readDirTarget, &st))
+	dirents := []byte{1, 0, 0, 0, 0, 0, 0, 0}         // d_next = 1
+	dirents = append(dirents, u64.LeBytes(st.Ino)...) // d_ino
+	dirents = append(dirents, 1, 0, 0, 0)             // d_namlen = 1 character
+	dirents = append(dirents, 3, 0, 0, 0)             // d_type = directory
+	dirents = append(dirents, '.')                    // name
+
+	// get the real inode of the parent directory
+	require.NoError(t, preopen.Stat(".", &st))
+	dirents = append(dirents, 2, 0, 0, 0, 0, 0, 0, 0) // d_next = 2
+	dirents = append(dirents, u64.LeBytes(st.Ino)...) // d_ino
+	dirents = append(dirents, 2, 0, 0, 0)             // d_namlen = 2 characters
+	dirents = append(dirents, 3, 0, 0, 0)             // d_type = directory
+	dirents = append(dirents, '.', '.')               // name
+
+	// Try to list them!
+	resultBufused := uint32(0) // where to write the amount used out of bufLen
+	buf := uint32(8)           // where to start the dirents
+	requireErrnoResult(t, ErrnoSuccess, mod, FdReaddirName,
+		uint64(dirFd), uint64(buf), uint64(0x2000), 0, uint64(resultBufused))
+
+	used, _ := mem.ReadUint32Le(resultBufused)
+
+	results, _ := mem.Read(buf, used)
+	require.Equal(t, dirents, results)
+}
+
 // Test_fdReaddir_opened_file_written ensures that writing files to the already-opened directory
 // is visible. This is significant on Windows.
 // https://github.com/ziglang/zig/blob/2ccff5115454bab4898bae3de88f5619310bc5c1/lib/std/fs/test.zig#L156-L184
 func Test_fdReaddir_opened_file_written(t *testing.T) {
+	if runtime.GOOS == "windows" && !platform.IsGo120 {
+		t.Skip("windows before go 1.20 has trouble reading the inode information on directories.")
+	}
+
 	root := t.TempDir()
 	mod, r, _ := requireProxyModule(t, wazero.NewModuleConfig().
 		WithFSConfig(wazero.NewFSConfig().WithDirMount(root, "/")),
@@ -4480,7 +4541,32 @@ func Test_fdReaddir_opened_file_written(t *testing.T) {
 	require.NoError(t, err)
 	defer f.Close()
 
-	// Try list them!
+	// get the real inode of the current directory
+	var st platform.Stat_t
+	require.NoError(t, preopen.Stat(readDirTarget, &st))
+	dirents := []byte{1, 0, 0, 0, 0, 0, 0, 0}         // d_next = 1
+	dirents = append(dirents, u64.LeBytes(st.Ino)...) // d_ino
+	dirents = append(dirents, 1, 0, 0, 0)             // d_namlen = 1 character
+	dirents = append(dirents, 3, 0, 0, 0)             // d_type = directory
+	dirents = append(dirents, '.')                    // name
+
+	// get the real inode of the parent directory
+	require.NoError(t, preopen.Stat(".", &st))
+	dirents = append(dirents, 2, 0, 0, 0, 0, 0, 0, 0) // d_next = 2
+	dirents = append(dirents, u64.LeBytes(st.Ino)...) // d_ino
+	dirents = append(dirents, 2, 0, 0, 0)             // d_namlen = 2 characters
+	dirents = append(dirents, 3, 0, 0, 0)             // d_type = directory
+	dirents = append(dirents, '.', '.')               // name
+
+	// get the real inode of the file
+	require.NoError(t, platform.StatFile(f, &st))
+	dirents = append(dirents, 3, 0, 0, 0, 0, 0, 0, 0)  // d_next = 3
+	dirents = append(dirents, u64.LeBytes(st.Ino)...)  // d_ino
+	dirents = append(dirents, 5, 0, 0, 0)              // d_namlen = 5 characters
+	dirents = append(dirents, 4, 0, 0, 0)              // d_type = regular_file
+	dirents = append(dirents, 'a', 'f', 'i', 'l', 'e') // name
+
+	// Try to list them!
 	resultBufused := uint32(0) // where to write the amount used out of bufLen
 	buf := uint32(8)           // where to start the dirents
 	requireErrnoResult(t, ErrnoSuccess, mod, FdReaddirName,
@@ -4489,11 +4575,5 @@ func Test_fdReaddir_opened_file_written(t *testing.T) {
 	used, _ := mem.ReadUint32Le(resultBufused)
 
 	results, _ := mem.Read(buf, used)
-	require.Equal(t, append(append(direntDot, direntDotDot...),
-		3, 0, 0, 0, 0, 0, 0, 0, // d_next = 3
-		0, 0, 0, 0, 0, 0, 0, 0, // d_ino = 0
-		5, 0, 0, 0, // d_namlen = 4 character
-		4, 0, 0, 0, // d_type = regular_file
-		'a', 'f', 'i', 'l', 'e', // name
-	), results)
+	require.Equal(t, dirents, results)
 }