Repository: tetratelabs/wazero
Pull request #1253 (2023-03-17 22:33:25)

fix file type of directory entries
Prior to this change, when reading directory entries, wazero would assume that all files are regular files except directories. This causes issues in client applications that work with symbolic links for example.

The issue was detected while working on passing the `testing/fstest` tests as part of the implementation of https://github.com/golang/go/issues/58141 because `fstest.TestFS` validates that the file type seen by `fs.DirEntry.Type()` and `fs.FileInfo.Mode().Type()` are the same.

Regarding testing this change, to reuse the existing test suite would likely require updating `internal/fstest.FS` to contain a symbolic link, but this fixture object is used in so many places that it may be difficult to land the fix. An alternative could be to make a local copy of `fstest.FS` and add the symbolic link there, but it defeats the documented intent of `fstest` to reduce drift between systems. Let me know how you would prefer to proceed!

Signed-off-by: Achille Roussel <achille.roussel@gmail.com>


Patch:
diff --git a/imports/wasi_snapshot_preview1/fs.go b/imports/wasi_snapshot_preview1/fs.go
index a8603f5c..43fe3af7 100644
--- a/imports/wasi_snapshot_preview1/fs.go
+++ b/imports/wasi_snapshot_preview1/fs.go
@@ -1022,7 +1022,7 @@ func writeDirents(
 		e := dirents[i]
 		nameLen := uint32(len(e.Name))
 
-		writeDirent(buf[pos:], d_next, e.Ino, nameLen, e.IsDir())
+		writeDirent(buf[pos:], d_next, e.Ino, nameLen, e.Type)
 		pos += DirentSize
 
 		copy(buf[pos:], e.Name)
@@ -1037,22 +1037,18 @@ func writeDirents(
 	// Write a dirent without its name
 	dirent := make([]byte, DirentSize)
 	e := dirents[i]
-	writeDirent(dirent, d_next, e.Ino, uint32(len(e.Name)), e.IsDir())
+	writeDirent(dirent, d_next, e.Ino, uint32(len(e.Name)), e.Type)
 
 	// Potentially truncate it
 	copy(buf[pos:], dirent)
 }
 
 // writeDirent writes DirentSize bytes
-func writeDirent(buf []byte, dNext uint64, ino uint64, dNamlen uint32, dType bool) {
+func writeDirent(buf []byte, dNext uint64, ino uint64, dNamlen uint32, dType fs.FileMode) {
 	le.PutUint64(buf, dNext)        // d_next
 	le.PutUint64(buf[8:], ino)      // d_ino
 	le.PutUint32(buf[16:], dNamlen) // d_namlen
-
-	filetype := FILETYPE_REGULAR_FILE
-	if dType {
-		filetype = FILETYPE_DIRECTORY
-	}
+	filetype := getWasiFiletype(dType)
 	le.PutUint32(buf[20:], uint32(filetype)) //  d_type
 }
 
@@ -1419,11 +1415,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 +1424,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..f320dc7f 100644
--- a/imports/wasi_snapshot_preview1/fs_test.go
+++ b/imports/wasi_snapshot_preview1/fs_test.go
@@ -1857,7 +1857,24 @@ var (
 		'a', 'b', '-', // name
 	}
 
-	dirents = append(append(append(append(direntDot, direntDotDot...), dirent1...), dirent2...), dirent3...)
+	// TODO: this entry is intended to test reading of a symbolic link entry,
+	// tho it requires modifying fstest.FS to contain this file.
+	// dirent4 = []byte{
+	// 	6, 0, 0, 0, 0, 0, 0, 0, // d_next = 6
+	// 	0, 0, 0, 0, 0, 0, 0, 0, // d_ino = 0
+	// 	2, 0, 0, 0, // d_namlen = 2 characters
+	// 	7, 0, 0, 0, // d_type = symbolic_link
+	// 	'l', 'n', // name
+	// }
+
+	dirents = bytes.Join([][]byte{
+		direntDot,
+		direntDotDot,
+		dirent1,
+		dirent2,
+		dirent3,
+		// dirent4,
+	}, nil)
 )
 
 func Test_fdReaddir(t *testing.T) {
@@ -3086,6 +3103,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 +3228,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 +3371,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))
 	}
 }