Repository: tetratelabs/wazero
Pull request #919 (2022-12-13 06:51:03)

wasi: enforce fd_preXXX functions only work on pre-opened files
At the moment, the only pre-open support we have is the file system itself (root a.k.a. / or file-descriptor 3). We may in the future add the ability to pre-open sockets, but in any case, this is where we are today.

This change hardens logic around fd_preXXX functions, ensuring they only work on actual pre-opens. This also fixes the path returned in filestat as we sometimes returned a full path, when typically the basename is the only part that can be returned.


codefromthecrypt: added some RATIONALE thanks @mathetake for zig stuff

Patch:
diff --git a/RATIONALE.md b/RATIONALE.md
index f3fdf18e..bb28a304 100644
--- a/RATIONALE.md
+++ b/RATIONALE.md
@@ -534,7 +534,8 @@ Compilers that target wasm act differently with regard to the working
 directory. For example, while `GOOS=js` uses host functions to track the
 working directory, WASI host functions do not. wasi-libc, used by TinyGo,
 tracks working directory changes in compiled wasm instead: initially "/" until
-code calls `chdir`.
+code calls `chdir`. Zig assumes the first pre-opened file descriptor is the
+working directory.
 
 The only place wazero can standardize a layered concern is via a host function.
 Since WASI doesn't use host functions to track the working directory, we can't
@@ -548,6 +549,7 @@ use absolute paths in configuration.
 See
 * https://github.com/golang/go/blob/go1.19rc2/src/syscall/fs_js.go#L324
 * https://github.com/WebAssembly/wasi-libc/pull/214#issue-673090117
+* https://github.com/ziglang/zig/blob/53a9ee699a35a3d245ab6d1dac1f0687a4dcb42c/src/main.zig#L32
 
 ### Why ignore the error returned by io.Reader when n > 1?
 
@@ -579,22 +581,43 @@ operation will err if it needs to. This helps reduce the complexity of the code
 in wazero and also accommodates the scenario where the bytes read are enough to
 satisfy its processor.
 
-### FdPrestatDirName
+### Pre-opened files
 
-`FdPrestatDirName` is a WASI function to return the path of the pre-opened directory of a file descriptor.
-It has the following three parameters, and the third `pathLen` has ambiguous semantics.
+WASI includes `fd_prestat_get` and `fd_prestat_dir_name` functions used to
+learn any directory paths for file descriptors open at initialization time.
 
-- `fd` - a file descriptor
-- `path` - the offset for the result path
-- `pathLen` - In wazero, `FdPrestatDirName` writes the result path string to `path` offset for the exact length of `pathLen`.
+For example, `__wasilibc_register_preopened_fd` scans any file descriptors past
+STDERR (1) and invokes `fd_prestat_dir_name` to learn any path prefixes they
+correspond to. Zig's `preopensAlloc` does similar. These pre-open functions are
+not used again after initialization.
 
-Wasmer considers `pathLen` to be the maximum length instead of the exact length that should be written.
+wazero currently supports only one pre-opened file, "/" and so that is the name
+returned by `fd_prestat_dir_name` for file descriptor 3 (STDERR+1).
+
+See
+ * https://github.com/WebAssembly/wasi-libc/blob/a02298043ff551ce1157bc2ee7ab74c3bffe7144/libc-bottom-half/sources/preopens.c
+ * https://github.com/ziglang/zig/blob/9cb06f3b8bf9ea6b5e5307711bc97328762d6a1d/lib/std/fs/wasi.zig#L50-L53
+
+### fd_prestat_dir_name
+
+`fd_prestat_dir_name` is a WASI function to return the path of the pre-opened
+directory of a file descriptor. It has the following three parameters, and the
+third `path_len` has ambiguous semantics.
+
+* `fd`: a file descriptor
+* `path`: the offset for the result path
+* `path_len`: In wazero, `FdPrestatDirName` writes the result path string to
+  `path` offset for the exact length of `path_len`.
+
+Wasmer considers `path_len` to be the maximum length instead of the exact
+length  that should be written.
 See https://github.com/wasmerio/wasmer/blob/3463c51268ed551933392a4063bd4f8e7498b0f6/lib/wasi/src/syscalls/mod.rs#L764
 
 The semantics in wazero follows that of wasmtime.
 See https://github.com/bytecodealliance/wasmtime/blob/2ca01ae9478f199337cf743a6ab543e8c3f3b238/crates/wasi-common/src/snapshots/preview_1.rs#L578-L582
 
-Their semantics match when `pathLen` == the length of `path`, so in practice this difference won't matter match.
+Their semantics match when `path_len` == the length of `path`, so in practice
+this difference won't matter match.
 
 ## sys.Walltime and Nanotime
 
diff --git a/imports/wasi_snapshot_preview1/fs.go b/imports/wasi_snapshot_preview1/fs.go
index d4be7e3d..8052ce2c 100644
--- a/imports/wasi_snapshot_preview1/fs.go
+++ b/imports/wasi_snapshot_preview1/fs.go
@@ -462,6 +462,11 @@ func fdPrestatGetFn(ctx context.Context, mod api.Module, stack []uint64) (presta
 	sysCtx := mod.(*wasm.CallContext).Sys
 	fd := uint32(stack[0])
 
+	// Currently, we only pre-open the root file descriptor.
+	if fd != internalsys.FdRoot {
+		return 0, ErrnoBadf
+	}
+
 	entry, ok := sysCtx.FS(ctx).OpenedFile(ctx, fd)
 	if !ok {
 		return 0, ErrnoBadf
@@ -469,7 +474,7 @@ func fdPrestatGetFn(ctx context.Context, mod api.Module, stack []uint64) (presta
 
 	// Upper 32-bits are zero because...
 	// * Zero-value 8-bit tag, and 3-byte zero-value padding
-	prestat = uint64(len(entry.Path) << 32)
+	prestat = uint64(len(entry.Name) << 32)
 	errno = ErrnoSuccess
 	return
 }
@@ -514,19 +519,22 @@ func fdPrestatDirNameFn(ctx context.Context, mod api.Module, params []uint64) Er
 	sysCtx := mod.(*wasm.CallContext).Sys
 	fd, path, pathLen := uint32(params[0]), uint32(params[1]), uint32(params[2])
 
+	// Currently, we only pre-open the root file descriptor.
+	if fd != internalsys.FdRoot {
+		return ErrnoBadf
+	}
+
 	f, ok := sysCtx.FS(ctx).OpenedFile(ctx, fd)
 	if !ok {
 		return ErrnoBadf
 	}
 
 	// Some runtimes may have another semantics. See /RATIONALE.md
-	if uint32(len(f.Path)) < pathLen {
+	if uint32(len(f.Name)) < pathLen {
 		return ErrnoNametoolong
 	}
 
-	// TODO: fdPrestatDirName may have to return ErrnoNotdir if the type of the
-	// prestat data of `fd` is not a PrestatDir.
-	if !mod.Memory().Write(ctx, path, []byte(f.Path)[:pathLen]) {
+	if !mod.Memory().Write(ctx, path, []byte(f.Name)[:pathLen]) {
 		return ErrnoFault
 	}
 	return ErrnoSuccess
@@ -1231,7 +1239,7 @@ func pathFilestatGetFn(ctx context.Context, mod api.Module, params []uint64) Err
 		return ErrnoNotdir // TODO: cache filetype instead of poking.
 	} else {
 		// TODO: consolidate "at" logic with path_open as same issues occur.
-		pathName = path.Join(dir.Path, pathName)
+		pathName = path.Join(dir.Name, pathName)
 	}
 
 	// Stat the file without allocating a file descriptor
diff --git a/internal/sys/fs.go b/internal/sys/fs.go
index e2c2460e..a0884d30 100644
--- a/internal/sys/fs.go
+++ b/internal/sys/fs.go
@@ -75,11 +75,13 @@ func (emptyRootDir) Sys() interface{}   { return nil }
 
 // FileEntry maps a path to an open file in a file system.
 type FileEntry struct {
-	// Path was the argument to FSContext.OpenFile
-	// TODO: we may need an additional field which is the full path.
-	Path string
+	// Name is the basename of the file, at the time it was opened. When the
+	// file is root "/" (fd = FdRoot), this is "/".
+	//
+	// Note: This must match fs.FileInfo.
+	Name string
 
-	// File is always non-nil, even when root "/" (fd=FdRoot)
+	// File is always non-nil, even when root "/" (fd = FdRoot).
 	File fs.File
 
 	// ReadDir is present when this File is a fs.ReadDirFile and `ReadDir`
@@ -159,7 +161,7 @@ func NewFSContext(root fs.FS) (fsc *FSContext, err error) {
 	return &FSContext{
 		fs: root,
 		openedFiles: map[uint32]*FileEntry{
-			FdRoot: {Path: "/", File: rootDir},
+			FdRoot: {Name: "/", File: rootDir},
 		},
 		lastFD: FdRoot,
 	}, nil
@@ -197,7 +199,7 @@ func (c *FSContext) OpenFile(_ context.Context, name string /* TODO: flags int,
 		_ = f.Close()
 		return 0, syscall.EBADF
 	}
-	c.openedFiles[newFD] = &FileEntry{Path: name, File: f}
+	c.openedFiles[newFD] = &FileEntry{Name: path.Base(name), File: f}
 	return newFD, nil
 }
 


Test patch:
diff --git a/experimental/fs_test.go b/experimental/fs_test.go
index caf4621a..6a49af14 100644
--- a/experimental/fs_test.go
+++ b/experimental/fs_test.go
@@ -28,7 +28,7 @@ func TestWithFS(t *testing.T) {
 
 	entry, ok := fsCtx.OpenedFile(ctx, 3)
 	require.True(t, ok)
-	require.Equal(t, "/", entry.Path)
+	require.Equal(t, "/", entry.Name)
 
 	// Override to nil context, ex to block file access
 	ctx, closer, err = experimental.WithFS(ctx, nil)
diff --git a/imports/wasi_snapshot_preview1/fs_test.go b/imports/wasi_snapshot_preview1/fs_test.go
index 462123a7..fc9c80b5 100644
--- a/imports/wasi_snapshot_preview1/fs_test.go
+++ b/imports/wasi_snapshot_preview1/fs_test.go
@@ -729,9 +729,9 @@ func Test_fdPread_Errors(t *testing.T) {
 }
 
 func Test_fdPrestatGet(t *testing.T) {
-	pathName := "/tmp"
-	mod, fd, log, r := requireOpenFile(t, pathName, nil)
+	mod, r, log := requireProxyModule(t, wazero.NewModuleConfig().WithFS(fstest.MapFS{}))
 	defer r.Close(testCtx)
+	fd := internalsys.FdRoot // only pre-opened directory currently supported.
 
 	resultPrestat := uint32(1) // arbitrary offset
 	expectedMemory := []byte{
@@ -739,7 +739,7 @@ func Test_fdPrestatGet(t *testing.T) {
 		0,       // 8-bit tag indicating `prestat_dir`, the only available tag
 		0, 0, 0, // 3-byte padding
 		// the result path length field after this
-		byte(len(pathName)), 0, 0, 0, // = in little endian encoding
+		1, 0, 0, 0, // = in little endian encoding
 		'?',
 	}
 
@@ -747,10 +747,10 @@ func Test_fdPrestatGet(t *testing.T) {
 
 	requireErrno(t, ErrnoSuccess, mod, fdPrestatGetName, uint64(fd), uint64(resultPrestat))
 	require.Equal(t, `
---> proxy.fd_prestat_get(fd=4,result.prestat=1)
-	--> wasi_snapshot_preview1.fd_prestat_get(fd=4,result.prestat=1)
-		==> wasi_snapshot_preview1.fdPrestatGet(fd=4)
-		<== (prestat=17179869184,ESUCCESS)
+--> proxy.fd_prestat_get(fd=3,result.prestat=1)
+	--> wasi_snapshot_preview1.fd_prestat_get(fd=3,result.prestat=1)
+		==> wasi_snapshot_preview1.fdPrestatGet(fd=3)
+		<== (prestat=4294967296,ESUCCESS)
 	<-- ESUCCESS
 <-- 0
 `, "\n"+log.String())
@@ -761,9 +761,9 @@ func Test_fdPrestatGet(t *testing.T) {
 }
 
 func Test_fdPrestatGet_Errors(t *testing.T) {
-	pathName := "/tmp"
-	mod, fd, log, r := requireOpenFile(t, pathName, nil)
+	mod, r, log := requireProxyModule(t, wazero.NewModuleConfig().WithFS(fstest.MapFS{}))
 	defer r.Close(testCtx)
+	fd := internalsys.FdRoot // only pre-opened directory currently supported.
 
 	memorySize := mod.Memory().Size(testCtx)
 	tests := []struct {
@@ -793,8 +793,8 @@ func Test_fdPrestatGet_Errors(t *testing.T) {
 			resultPrestat: memorySize,
 			expectedErrno: ErrnoFault,
 			expectedLog: `
---> proxy.fd_prestat_get(fd=4,result.prestat=65536)
-	--> wasi_snapshot_preview1.fd_prestat_get(fd=4,result.prestat=65536)
+--> proxy.fd_prestat_get(fd=3,result.prestat=65536)
+	--> wasi_snapshot_preview1.fd_prestat_get(fd=3,result.prestat=65536)
 	<-- EFAULT
 <-- 21
 `,
@@ -815,24 +815,22 @@ func Test_fdPrestatGet_Errors(t *testing.T) {
 }
 
 func Test_fdPrestatDirName(t *testing.T) {
-	pathName := "/tmp"
-	mod, fd, log, r := requireOpenFile(t, pathName, nil)
+	mod, r, log := requireProxyModule(t, wazero.NewModuleConfig().WithFS(fstest.MapFS{}))
 	defer r.Close(testCtx)
+	fd := internalsys.FdRoot // only pre-opened directory currently supported.
 
 	path := uint32(1)    // arbitrary offset
-	pathLen := uint32(3) // shorter than len("/tmp") to test the path is written for the length of pathLen
+	pathLen := uint32(0) // shorter than len("/") to prove truncation is ok
 	expectedMemory := []byte{
-		'?',
-		'/', 't', 'm',
-		'?', '?', '?',
+		'?', '?', '?', '?',
 	}
 
 	maskMemory(t, testCtx, mod, len(expectedMemory))
 
 	requireErrno(t, ErrnoSuccess, mod, fdPrestatDirNameName, uint64(fd), uint64(path), uint64(pathLen))
 	require.Equal(t, `
---> proxy.fd_prestat_dir_name(fd=4,path=1,path_len=3)
-	==> wasi_snapshot_preview1.fd_prestat_dir_name(fd=4,path=1,path_len=3)
+--> proxy.fd_prestat_dir_name(fd=3,path=1,path_len=0)
+	==> wasi_snapshot_preview1.fd_prestat_dir_name(fd=3,path=1,path_len=0)
 	<== ESUCCESS
 <-- 0
 `, "\n"+log.String())
@@ -843,13 +841,13 @@ func Test_fdPrestatDirName(t *testing.T) {
 }
 
 func Test_fdPrestatDirName_Errors(t *testing.T) {
-	pathName := "/tmp"
-	mod, fd, log, r := requireOpenFile(t, pathName, nil)
+	mod, r, log := requireProxyModule(t, wazero.NewModuleConfig().WithFS(fstest.MapFS{}))
 	defer r.Close(testCtx)
+	fd := internalsys.FdRoot // only pre-opened directory currently supported.
 
 	memorySize := mod.Memory().Size(testCtx)
 	validAddress := uint32(0) // Arbitrary valid address as arguments to fd_prestat_dir_name. We chose 0 here.
-	pathLen := uint32(len("/tmp"))
+	pathLen := uint32(len("/"))
 
 	tests := []struct {
 		name          string
@@ -866,8 +864,8 @@ func Test_fdPrestatDirName_Errors(t *testing.T) {
 			pathLen:       pathLen,
 			expectedErrno: ErrnoFault,
 			expectedLog: `
---> proxy.fd_prestat_dir_name(fd=4,path=65536,path_len=4)
-	==> wasi_snapshot_preview1.fd_prestat_dir_name(fd=4,path=65536,path_len=4)
+--> proxy.fd_prestat_dir_name(fd=3,path=65536,path_len=1)
+	==> wasi_snapshot_preview1.fd_prestat_dir_name(fd=3,path=65536,path_len=1)
 	<== EFAULT
 <-- 21
 `,
@@ -879,8 +877,8 @@ func Test_fdPrestatDirName_Errors(t *testing.T) {
 			pathLen:       pathLen,
 			expectedErrno: ErrnoFault,
 			expectedLog: `
---> proxy.fd_prestat_dir_name(fd=4,path=65533,path_len=4)
-	==> wasi_snapshot_preview1.fd_prestat_dir_name(fd=4,path=65533,path_len=4)
+--> proxy.fd_prestat_dir_name(fd=3,path=65536,path_len=1)
+	==> wasi_snapshot_preview1.fd_prestat_dir_name(fd=3,path=65536,path_len=1)
 	<== EFAULT
 <-- 21
 `,
@@ -892,8 +890,8 @@ func Test_fdPrestatDirName_Errors(t *testing.T) {
 			pathLen:       pathLen + 1,
 			expectedErrno: ErrnoNametoolong,
 			expectedLog: `
---> proxy.fd_prestat_dir_name(fd=4,path=0,path_len=5)
-	==> wasi_snapshot_preview1.fd_prestat_dir_name(fd=4,path=0,path_len=5)
+--> proxy.fd_prestat_dir_name(fd=3,path=0,path_len=2)
+	==> wasi_snapshot_preview1.fd_prestat_dir_name(fd=3,path=0,path_len=2)
 	<== ENAMETOOLONG
 <-- 37
 `,
@@ -905,8 +903,8 @@ func Test_fdPrestatDirName_Errors(t *testing.T) {
 			pathLen:       pathLen,
 			expectedErrno: ErrnoBadf,
 			expectedLog: `
---> proxy.fd_prestat_dir_name(fd=42,path=0,path_len=4)
-	==> wasi_snapshot_preview1.fd_prestat_dir_name(fd=42,path=0,path_len=4)
+--> proxy.fd_prestat_dir_name(fd=42,path=0,path_len=1)
+	==> wasi_snapshot_preview1.fd_prestat_dir_name(fd=42,path=0,path_len=1)
 	<== EBADF
 <-- 8
 `,
@@ -2654,7 +2652,7 @@ func Test_pathOpen(t *testing.T) {
 	fsc := mod.(*wasm.CallContext).Sys.FS(testCtx)
 	f, ok := fsc.OpenedFile(testCtx, expectedFD)
 	require.True(t, ok)
-	require.Equal(t, pathName, f.Path)
+	require.Equal(t, pathName, f.Name)
 }
 
 func Test_pathOpen_Errors(t *testing.T) {
diff --git a/internal/sys/fs_test.go b/internal/sys/fs_test.go
index ddc9b09e..789806c4 100644
--- a/internal/sys/fs_test.go
+++ b/internal/sys/fs_test.go
@@ -4,8 +4,10 @@ import (
 	"context"
 	"embed"
 	"errors"
+	"io"
 	"io/fs"
 	"os"
+	"path"
 	"testing"
 	"testing/fstest"
 
@@ -19,13 +21,6 @@ var testCtx = context.WithValue(context.Background(), struct{}{}, "arbitrary")
 //go:embed testdata
 var testdata embed.FS
 
-var testFS = fstest.MapFS{
-	"empty.txt":    {},
-	"test.txt":     {Data: []byte("animals\n")},
-	"sub":          {Mode: fs.ModeDir},
-	"sub/test.txt": {Data: []byte("greet sub dir\n")},
-}
-
 func TestNewFSContext(t *testing.T) {
 	embedFS, err := fs.Sub(testdata, "testdata")
 	require.NoError(t, err)
@@ -49,11 +44,7 @@ func TestNewFSContext(t *testing.T) {
 		},
 		{
 			name: "fstest.MapFS",
-			fs:   testFS,
-		},
-		{
-			name: "fstest.MapFS",
-			fs:   testFS,
+			fs:   fstest.MapFS{},
 		},
 	}
 
@@ -66,7 +57,7 @@ func TestNewFSContext(t *testing.T) {
 			defer fsc.Close(testCtx)
 
 			require.Equal(t, tc.fs, fsc.fs)
-			require.Equal(t, "/", fsc.openedFiles[FdRoot].Path)
+			require.Equal(t, "/", fsc.openedFiles[FdRoot].Name)
 			rootFile := fsc.openedFiles[FdRoot].File
 			require.NotNil(t, rootFile)
 
@@ -118,6 +109,65 @@ func TestEmptyFSContext(t *testing.T) {
 	})
 }
 
+func TestContext_File(t *testing.T) {
+	embedFS, err := fs.Sub(testdata, "testdata")
+	require.NoError(t, err)
+
+	fsc, err := NewFSContext(embedFS)
+	require.NoError(t, err)
+	defer fsc.Close(testCtx)
+
+	tests := []struct {
+		name     string
+		expected string
+	}{
+		{
+			name: "empty.txt",
+		},
+		{
+			name:     "test.txt",
+			expected: "animals\n",
+		},
+		{
+			name:     "sub/test.txt",
+			expected: "greet sub dir\n",
+		},
+		{
+			name:     "sub/sub/test.txt",
+			expected: "greet sub sub dir\n",
+		},
+	}
+
+	for _, tt := range tests {
+		tc := tt
+
+		t.Run(tc.name, func(b *testing.T) {
+			fd, err := fsc.OpenFile(testCtx, tc.name)
+			require.NoError(t, err)
+			defer fsc.CloseFile(testCtx, fd)
+
+			f, ok := fsc.OpenedFile(testCtx, fd)
+			require.True(t, ok)
+
+			stat, err := f.File.Stat()
+			require.NoError(t, err)
+
+			// Ensure the name is the basename and matches the stat name.
+			require.Equal(t, path.Base(tc.name), f.Name)
+			require.Equal(t, f.Name, stat.Name())
+
+			buf := make([]byte, stat.Size())
+			size, err := f.File.Read(buf)
+			if err != nil {
+				require.Equal(t, io.EOF, err)
+			}
+			require.Equal(t, stat.Size(), int64(size))
+
+			require.Equal(t, tc.expected, string(buf[:size]))
+		})
+	}
+}
+
 func TestContext_Close(t *testing.T) {
 	fsc, err := NewFSContext(testfs.FS{"foo": &testfs.File{}})
 	require.NoError(t, err)
diff --git a/internal/sys/testdata/sub/sub/test.txt b/internal/sys/testdata/sub/sub/test.txt
new file mode 100644
index 00000000..75a0f487
--- /dev/null
+++ b/internal/sys/testdata/sub/sub/test.txt
@@ -0,0 +1,1 @@
+greet sub sub dir