Repository: tetratelabs/wazero
Pull request #1252 (2023-03-17 18:18:32)

path_filestat_get: interpret LOOKUP_SYMLINK_FOLLOW
This PR adds support for passing lookup flags to `path_filestat_get`. The motivation came from the work on https://github.com/golang/go/issues/58141, especially implementing `syscall.Lstat`.

Without this change, an alternative implementation is possible using `path_open` + `fd_filestat_get` + `fd_close` because wazero allows passing stats, however, this fails on OSX because `LOOKUP_SYMLINK_FOLLOW` is translated to `O_NOFOLLOW` which causes `open` to fail with `ELOOP` because wazero doesn't set `O_SYMLINK` (I can follow up with a fix for that in a different PR).

I updates the tests to use the `LOOKUP_SYMLINK_FOLLOW` flag since that was the historical behavior being tested; however, I haven't found a paved path for testing with symbolic links. The tests use `fstest.FS` which ends up creating an internal adapter between `fs.FS` and `sysfs.FS` and always returns `ENOSYS` on calls to `Lstat`.

It seems that we would want tests that actually work on a local file system (e.g. something in `testdata`) so we can exercise an actual call to `lstat`, I wanted to get feedback before making too many changes to the test suite. Let me know what you would recommend!

Signed-off-by: Achille Roussel <achille.roussel@gmail.com>


achille-roussel: The failing tests seem to be related to the use of `fs.FS` as well in `wasi_stdlib_test.go`, calls to `path_filestat_get` with `LOOKUP_SYMLINK_FOLLOW` now fail with `ENOSYS` when backed by a Go file system.
achille-roussel: I maybe found a compromise by implementing `Lstat` in terms of `Stat` in the `fs.FS` adapter; I left comments to explain the reasoning and added tests to exercise `path_filestat_get` with and without `LOOKUP_SYMLINK_FOLLOW`.

Let me know if you would like to see anything changed.

Patch:
diff --git a/imports/wasi_snapshot_preview1/fs.go b/imports/wasi_snapshot_preview1/fs.go
index a8603f5c..9f3fbfb1 100644
--- a/imports/wasi_snapshot_preview1/fs.go
+++ b/imports/wasi_snapshot_preview1/fs.go
@@ -1419,11 +1419,7 @@ func pathFilestatGetFn(_ context.Context, mod api.Module, params []uint64) Errno
 	fsc := mod.(*wasm.CallContext).Sys.FS()
 
 	fd := uint32(params[0])
-
-	// TODO: flags is a lookupflags and it only has one bit: symlink_follow
-	// https://github.com/WebAssembly/WASI/blob/snapshot-01/phases/snapshot/docs.md#lookupflags
-	_ /* flags */ = uint32(params[1])
-
+	flags := uint16(params[1])
 	path := uint32(params[2])
 	pathLen := uint32(params[3])
 
@@ -1432,9 +1428,23 @@ func pathFilestatGetFn(_ context.Context, mod api.Module, params []uint64) Errno
 		return errno
 	}
 
-	// Stat the file without allocating a file descriptor
+	// Stat the file without allocating a file descriptor.
+	//
+	// Note: `preopen` is a `sysfs.FS` interface, so passing the address of `st`
+	// causes the value to escape to the heap because the compiler doesn't know
+	// whether the pointer will be retained by the method.
+	//
+	// This could be optimized by modifying Stat/Lstat to return the `Stat_t`
+	// value instead of passing a pointer as output parameter.
 	var st platform.Stat_t
-	if err := preopen.Stat(pathName, &st); err != nil {
+	var err error
+
+	if (flags & LOOKUP_SYMLINK_FOLLOW) == 0 {
+		err = preopen.Lstat(pathName, &st)
+	} else {
+		err = preopen.Stat(pathName, &st)
+	}
+	if err != nil {
 		return ToErrno(err)
 	}
 
diff --git a/internal/sysfs/adapter.go b/internal/sysfs/adapter.go
index 379add94..cb398d6c 100644
--- a/internal/sysfs/adapter.go
+++ b/internal/sysfs/adapter.go
@@ -61,6 +61,19 @@ func (a *adapter) Stat(path string, stat *platform.Stat_t) error {
 	return platform.StatFile(f, stat)
 }
 
+// Lstat implements FS.Lstat
+func (a *adapter) Lstat(path string, stat *platform.Stat_t) error {
+	// At this time, we make the assumption that fs.FS instances do not support
+	// symbolic links, therefore Lstat is the same as Stat. This is obviously
+	// not true but until fs.FS has a solid story for how to handle symlinks we
+	// are better off not making a decision that would be difficult to revert
+	// later on.
+	//
+	// For further discussions on the topic, see:
+	// https://github.com/golang/go/issues/49580
+	return a.Stat(path, stat)
+}
+
 func cleanPath(name string) string {
 	if len(name) == 0 {
 		return name


Test patch:
diff --git a/imports/wasi_snapshot_preview1/fs_test.go b/imports/wasi_snapshot_preview1/fs_test.go
index db5909cb..368a9077 100644
--- a/imports/wasi_snapshot_preview1/fs_test.go
+++ b/imports/wasi_snapshot_preview1/fs_test.go
@@ -3086,6 +3086,7 @@ func Test_pathFilestatGet(t *testing.T) {
 	tests := []struct {
 		name                        string
 		fd, pathLen, resultFilestat uint32
+		flags                       uint16
 		memory, expectedMemory      []byte
 		expectedErrno               Errno
 		expectedLog                 string
@@ -3210,6 +3211,136 @@ func Test_pathFilestatGet(t *testing.T) {
 			expectedLog: `
 ==> wasi_snapshot_preview1.path_filestat_get(fd=3,flags=,path=animals.txt)
 <== (filestat=,errno=EFAULT)
+`,
+		},
+		{
+			name:           "file under root (follow symlinks)",
+			fd:             sys.FdPreopen,
+			flags:          LOOKUP_SYMLINK_FOLLOW,
+			memory:         initialMemoryFile,
+			pathLen:        uint32(len(file)),
+			resultFilestat: uint32(len(file)) + 1,
+			expectedMemory: append(
+				initialMemoryFile,
+				0, 0, 0, 0, 0, 0, 0, 0, // dev
+				0, 0, 0, 0, 0, 0, 0, 0, // ino
+				4, 0, 0, 0, 0, 0, 0, 0, // filetype + padding
+				1, 0, 0, 0, 0, 0, 0, 0, // nlink
+				30, 0, 0, 0, 0, 0, 0, 0, // size
+				0x0, 0x82, 0x13, 0x80, 0x6b, 0x16, 0x24, 0x17, // atim
+				0x0, 0x82, 0x13, 0x80, 0x6b, 0x16, 0x24, 0x17, // mtim
+				0x0, 0x82, 0x13, 0x80, 0x6b, 0x16, 0x24, 0x17, // ctim
+			),
+			expectedLog: `
+==> wasi_snapshot_preview1.path_filestat_get(fd=3,flags=SYMLINK_FOLLOW,path=animals.txt)
+<== (filestat={filetype=REGULAR_FILE,size=30,mtim=1667482413000000000},errno=ESUCCESS)
+`,
+		},
+		{
+			name:           "file under dir (follow symlinks)",
+			fd:             sys.FdPreopen, // root
+			flags:          LOOKUP_SYMLINK_FOLLOW,
+			memory:         initialMemoryFileInDir,
+			pathLen:        uint32(len(fileInDir)),
+			resultFilestat: uint32(len(fileInDir)) + 1,
+			expectedMemory: append(
+				initialMemoryFileInDir,
+				0, 0, 0, 0, 0, 0, 0, 0, // dev
+				0, 0, 0, 0, 0, 0, 0, 0, // ino
+				4, 0, 0, 0, 0, 0, 0, 0, // filetype + padding
+				1, 0, 0, 0, 0, 0, 0, 0, // nlink
+				14, 0, 0, 0, 0, 0, 0, 0, // size
+				0x0, 0x0, 0xc2, 0xd3, 0x43, 0x6, 0x36, 0x17, // atim
+				0x0, 0x0, 0xc2, 0xd3, 0x43, 0x6, 0x36, 0x17, // mtim
+				0x0, 0x0, 0xc2, 0xd3, 0x43, 0x6, 0x36, 0x17, // ctim
+			),
+			expectedLog: `
+==> wasi_snapshot_preview1.path_filestat_get(fd=3,flags=SYMLINK_FOLLOW,path=sub/test.txt)
+<== (filestat={filetype=REGULAR_FILE,size=14,mtim=1672531200000000000},errno=ESUCCESS)
+`,
+		},
+		{
+			name:           "dir under root (follow symlinks)",
+			fd:             sys.FdPreopen,
+			flags:          LOOKUP_SYMLINK_FOLLOW,
+			memory:         initialMemoryDir,
+			pathLen:        uint32(len(dir)),
+			resultFilestat: uint32(len(dir)) + 1,
+			expectedMemory: append(
+				initialMemoryDir,
+				0, 0, 0, 0, 0, 0, 0, 0, // dev
+				0, 0, 0, 0, 0, 0, 0, 0, // ino
+				3, 0, 0, 0, 0, 0, 0, 0, // filetype + padding
+				1, 0, 0, 0, 0, 0, 0, 0, // nlink
+				0, 0, 0, 0, 0, 0, 0, 0, // size
+				0x0, 0x0, 0x1f, 0xa6, 0x70, 0xfc, 0xc5, 0x16, // atim
+				0x0, 0x0, 0x1f, 0xa6, 0x70, 0xfc, 0xc5, 0x16, // mtim
+				0x0, 0x0, 0x1f, 0xa6, 0x70, 0xfc, 0xc5, 0x16, // ctim
+			),
+			expectedLog: `
+==> wasi_snapshot_preview1.path_filestat_get(fd=3,flags=SYMLINK_FOLLOW,path=sub)
+<== (filestat={filetype=DIRECTORY,size=0,mtim=1640995200000000000},errno=ESUCCESS)
+`,
+		},
+		{
+			name:          "unopened FD (follow symlinks)",
+			fd:            math.MaxUint32,
+			flags:         LOOKUP_SYMLINK_FOLLOW,
+			expectedErrno: ErrnoBadf,
+			expectedLog: `
+==> wasi_snapshot_preview1.path_filestat_get(fd=-1,flags=SYMLINK_FOLLOW,path=)
+<== (filestat=,errno=EBADF)
+`,
+		},
+		{
+			name:           "Fd not a directory (follow symlinks)",
+			fd:             fileFD,
+			flags:          LOOKUP_SYMLINK_FOLLOW,
+			memory:         initialMemoryFile,
+			pathLen:        uint32(len(file)),
+			resultFilestat: 2,
+			expectedErrno:  ErrnoNotdir,
+			expectedLog: `
+==> wasi_snapshot_preview1.path_filestat_get(fd=4,flags=SYMLINK_FOLLOW,path=animals.txt)
+<== (filestat=,errno=ENOTDIR)
+`,
+		},
+		{
+			name:           "path under root doesn't exist (follow symlinks)",
+			fd:             sys.FdPreopen,
+			flags:          LOOKUP_SYMLINK_FOLLOW,
+			memory:         initialMemoryNotExists,
+			pathLen:        1,
+			resultFilestat: 2,
+			expectedErrno:  ErrnoNoent,
+			expectedLog: `
+==> wasi_snapshot_preview1.path_filestat_get(fd=3,flags=SYMLINK_FOLLOW,path=?)
+<== (filestat=,errno=ENOENT)
+`,
+		},
+		{
+			name:          "path is out of memory (follow symlinks)",
+			fd:            sys.FdPreopen,
+			flags:         LOOKUP_SYMLINK_FOLLOW,
+			memory:        initialMemoryFile,
+			pathLen:       memorySize,
+			expectedErrno: ErrnoFault,
+			expectedLog: `
+==> wasi_snapshot_preview1.path_filestat_get(fd=3,flags=SYMLINK_FOLLOW,path=OOM(1,65536))
+<== (filestat=,errno=EFAULT)
+`,
+		},
+		{
+			name:           "resultFilestat exceeds the maximum valid address by 1 (follow symlinks)",
+			fd:             sys.FdPreopen,
+			flags:          LOOKUP_SYMLINK_FOLLOW,
+			memory:         initialMemoryFile,
+			pathLen:        uint32(len(file)),
+			resultFilestat: memorySize - 64 + 1,
+			expectedErrno:  ErrnoFault,
+			expectedLog: `
+==> wasi_snapshot_preview1.path_filestat_get(fd=3,flags=SYMLINK_FOLLOW,path=animals.txt)
+<== (filestat=,errno=EFAULT)
 `,
 		},
 	}
@@ -3223,8 +3354,7 @@ func Test_pathFilestatGet(t *testing.T) {
 			maskMemory(t, mod, len(tc.expectedMemory))
 			mod.Memory().Write(0, tc.memory)
 
-			flags := uint32(0)
-			requireErrnoResult(t, tc.expectedErrno, mod, PathFilestatGetName, uint64(tc.fd), uint64(flags), uint64(1), uint64(tc.pathLen), uint64(tc.resultFilestat))
+			requireErrnoResult(t, tc.expectedErrno, mod, PathFilestatGetName, uint64(tc.fd), uint64(tc.flags), uint64(1), uint64(tc.pathLen), uint64(tc.resultFilestat))
 			require.Equal(t, tc.expectedLog, "\n"+log.String())
 
 			actual, ok := mod.Memory().Read(0, uint32(len(tc.expectedMemory)))
diff --git a/internal/sysfs/adapter_test.go b/internal/sysfs/adapter_test.go
index 6f9b7be3..85f7c575 100644
--- a/internal/sysfs/adapter_test.go
+++ b/internal/sysfs/adapter_test.go
@@ -4,6 +4,7 @@ import (
 	"errors"
 	"io/fs"
 	"os"
+	"path/filepath"
 	"runtime"
 	"syscall"
 	"testing"
@@ -116,8 +117,6 @@ func TestAdapt_Open_Read(t *testing.T) {
 	})
 }
 
-// TestAdapt_Lstat is unsupported because the Lstat() function is not implemented
-// on os.File.
 func TestAdapt_Lstat(t *testing.T) {
 	tmpDir := t.TempDir()
 	require.NoError(t, fstest.WriteTestFiles(tmpDir))
@@ -128,7 +127,7 @@ func TestAdapt_Lstat(t *testing.T) {
 		linkPath := joinPath(tmpDir, path+"-link")
 		require.NoError(t, os.Symlink(fullPath, linkPath))
 		var stat platform.Stat_t
-		require.EqualErrno(t, syscall.ENOSYS, testFS.Lstat(linkPath, &stat))
+		require.NoError(t, testFS.Lstat(filepath.Base(linkPath), &stat))
 	}
 }