diff --git a/imports/wasi_snapshot_preview1/fs.go b/imports/wasi_snapshot_preview1/fs.go index c87d7699..4f5f3fb6 100644 --- a/imports/wasi_snapshot_preview1/fs.go +++ b/imports/wasi_snapshot_preview1/fs.go @@ -858,16 +858,12 @@ func fdReaddirFn(_ context.Context, mod api.Module, params []uint64) syscall.Err return syscall.EINVAL } - // Validate the FD is a directory - f, errno := openedDir(fsc, fd) - if errno != 0 { - return errno - } - // Discard the bool value because we validated the fd already. - dir, errno := fsc.LookupReaddir(fd, f) + // Get or open the directory + dir, errno := openDir(fsc, fd) if errno != 0 { return errno } + // Validate the cookie and possibly sync the internal state to the one the cookie represents. if errno = dir.Rewind(cookie); errno != 0 { return errno @@ -1010,22 +1006,12 @@ func writeDirent(buf []byte, dNext uint64, ino uint64, dNamlen uint32, dType fs. le.PutUint32(buf[20:], uint32(filetype)) // d_type } -// openedDir returns the sys.FileEntry for the directory and 0 if the fd points to a readable directory. -func openedDir(fsc *sys.FSContext, fd int32) (*sys.FileEntry, syscall.Errno) { +// openDir lazy opens a sys.Readdir for the directory or returns an error. +func openDir(fsc *sys.FSContext, fd int32) (*sys.Readdir, syscall.Errno) { if f, ok := fsc.LookupFile(fd); !ok { return nil, syscall.EBADF - } else if isDir, errno := f.File.IsDir(); errno != 0 { - return nil, errno - } else if !isDir { - // fd_readdir docs don't indicate whether to return syscall.ENOTDIR or - // syscall.EBADF. It has been noticed that rust will crash on syscall.ENOTDIR, - // and POSIX C ref seems to not return this, so we don't either. - // - // See https://github.com/WebAssembly/WASI/blob/snapshot-01/phases/snapshot/docs.md#fd_readdir - // and https://en.wikibooks.org/wiki/C_Programming/POSIX_Reference/dirent.h - return nil, syscall.EBADF } else { - return f, 0 + return f.OpenDir(true) } } diff --git a/imports/wasi_snapshot_preview1/fs_test.go b/imports/wasi_snapshot_preview1/fs_test.go index c9d7c548..c6496c6f 100644 --- a/imports/wasi_snapshot_preview1/fs_test.go +++ b/imports/wasi_snapshot_preview1/fs_test.go @@ -2013,9 +2013,7 @@ func Test_fdReaddir(t *testing.T) { fsc := mod.(*wasm.ModuleInstance).Sys.FS() preopen := fsc.RootFS() - - fd, errno := fsc.OpenFile(preopen, "dir", os.O_RDONLY, 0) - require.EqualErrno(t, 0, errno) + fd := sys.FdPreopen + 1 tests := []struct { name string @@ -2069,7 +2067,7 @@ func Test_fdReaddir(t *testing.T) { initialDir: "dir", dir: func() { f, _ := fsc.LookupFile(fd) - rdd, _ := fsc.LookupReaddir(fd, f) + rdd, _ := f.OpenDir(true) _ = rdd.Advance() }, @@ -2084,7 +2082,7 @@ func Test_fdReaddir(t *testing.T) { initialDir: "dir", dir: func() { f, _ := fsc.LookupFile(fd) - rdd, _ := fsc.LookupReaddir(fd, f) + rdd, _ := f.OpenDir(true) _ = rdd.Advance() }, bufLen: 30, // length is longer than the second entry, but not long enough for a header. @@ -2099,7 +2097,7 @@ func Test_fdReaddir(t *testing.T) { initialDir: "dir", dir: func() { f, _ := fsc.LookupFile(fd) - rdd, _ := fsc.LookupReaddir(fd, f) + rdd, _ := f.OpenDir(true) _ = rdd.Advance() }, bufLen: 50, // length is longer than the second entry + enough for the header of third. @@ -2113,7 +2111,7 @@ func Test_fdReaddir(t *testing.T) { initialDir: "dir", dir: func() { f, _ := fsc.LookupFile(fd) - rdd, _ := fsc.LookupReaddir(fd, f) + rdd, _ := f.OpenDir(true) _ = rdd.Advance() }, bufLen: 53, // length is long enough for second and third. @@ -2127,7 +2125,7 @@ func Test_fdReaddir(t *testing.T) { initialDir: "dir", dir: func() { f, _ := fsc.LookupFile(fd) - rdd, _ := fsc.LookupReaddir(fd, f) + rdd, _ := f.OpenDir(true) rdd.Skip(2) }, bufLen: 27, // length is long enough for exactly third. @@ -2141,7 +2139,7 @@ func Test_fdReaddir(t *testing.T) { initialDir: "dir", dir: func() { f, _ := fsc.LookupFile(fd) - rdd, _ := fsc.LookupReaddir(fd, f) + rdd, _ := f.OpenDir(true) rdd.Skip(2) }, bufLen: 300, // length is long enough for third and more @@ -2155,7 +2153,7 @@ func Test_fdReaddir(t *testing.T) { initialDir: "dir", dir: func() { f, _ := fsc.LookupFile(fd) - rdd, _ := fsc.LookupReaddir(fd, f) + rdd, _ := f.OpenDir(true) rdd.Skip(5) }, bufLen: 300, // length is long enough for third and more @@ -2169,12 +2167,12 @@ func Test_fdReaddir(t *testing.T) { tc := tt t.Run(tc.name, func(t *testing.T) { defer log.Reset() - defer fsc.CloseReaddir(fd) - dir, errno := preopen.OpenFile(tc.initialDir, os.O_RDONLY, 0) + fd, errno := fsc.OpenFile(preopen, tc.initialDir, os.O_RDONLY, 0) require.EqualErrno(t, 0, errno) + defer fsc.CloseFile(fd) // nolint + file, _ := fsc.LookupFile(fd) - file.File = dir if tc.dir != nil { tc.dir() @@ -2202,7 +2200,7 @@ func Test_fdReaddir(t *testing.T) { require.Equal(t, tc.expectedMem, mem[:tc.expectedMemSize]) } - rdd, errno := fsc.LookupReaddir(fd, file) + rdd, errno := file.OpenDir(true) require.Equal(t, syscall.Errno(0), errno) require.Equal(t, tc.expectedCookie, rdd.Cookie()) }) @@ -2282,8 +2280,8 @@ func Test_fdReaddir_Errors(t *testing.T) { fileFD, errno := fsc.OpenFile(preopen, "animals.txt", os.O_RDONLY, 0) require.EqualErrno(t, 0, errno) - dirFD, errno := fsc.OpenFile(preopen, "dir", os.O_RDONLY, 0) - require.EqualErrno(t, 0, errno) + // Directories are stateful, so we open them during the test. + dirFD := fileFD + 1 tests := []struct { name string @@ -2380,13 +2378,10 @@ func Test_fdReaddir_Errors(t *testing.T) { defer log.Reset() // Reset the directory so that tests don't taint each other. - if file, ok := fsc.LookupFile(tc.fd); ok && tc.fd == dirFD { - dir, errno := preopen.OpenFile("dir", os.O_RDONLY, 0) + if tc.fd == dirFD { + dirFD, errno = fsc.OpenFile(preopen, "dir", os.O_RDONLY, 0) require.EqualErrno(t, 0, errno) - defer dir.Close() - - file.File = dir - fsc.CloseReaddir(tc.fd) + defer fsc.CloseFile(dirFD) // nolint } requireErrnoResult(t, tc.expectedErrno, mod, wasip1.FdReaddirName, diff --git a/imports/wasi_snapshot_preview1/wasi_bench_test.go b/imports/wasi_snapshot_preview1/wasi_bench_test.go index 8d735659..9302b9d8 100644 --- a/imports/wasi_snapshot_preview1/wasi_bench_test.go +++ b/imports/wasi_snapshot_preview1/wasi_bench_test.go @@ -219,7 +219,6 @@ func Benchmark_fdReaddir(b *testing.B) { if f.File, errno = fsc.RootFS().OpenFile(".", os.O_RDONLY, 0); errno != 0 { b.Fatal(errno) } - fsc.CloseReaddir(fd) // Make an initial call to build the state of an unread directory if bc.continued { diff --git a/internal/fsapi/dir.go b/internal/fsapi/dir.go index c28783b4..3ad75b94 100644 --- a/internal/fsapi/dir.go +++ b/internal/fsapi/dir.go @@ -9,20 +9,22 @@ import ( // Dirent is an entry read from a directory. // -// This is a portable variant of syscall.Dirent containing fields needed for -// WebAssembly ABI including WASI snapshot-01 and wasi-filesystem. Unlike -// fs.DirEntry, this may include the Ino. +// # Notes +// +// - This extends `dirent` defined in POSIX with some fields defined by +// Linux. See https://man7.org/linux/man-pages/man3/readdir.3.html and +// https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/dirent.h.html type Dirent struct { - // ^^ Dirent name matches syscall.Dirent - - // Name is the base name of the directory entry. - Name string - // Ino is the file serial number, or zero if not available. Ino uint64 + // Name is the base name of the directory entry. Empty is invalid. + Name string + // Type is fs.FileMode masked on fs.ModeType. For example, zero is a // regular file, fs.ModeDir is a directory and fs.ModeIrregular is unknown. + // + // Note: This is defined by Linux, not POSIX. Type fs.FileMode } diff --git a/internal/fsapi/file.go b/internal/fsapi/file.go index 2d6ad037..ba454acd 100644 --- a/internal/fsapi/file.go +++ b/internal/fsapi/file.go @@ -24,8 +24,11 @@ import ( // // # Notes // -// A writable filesystem abstraction is not yet implemented as of Go 1.20. See -// https://github.com/golang/go/issues/45757 +// - You must call Close to avoid file resource conflicts. For example, +// Windows cannot delete the underlying directory while a handle to it +// remains open. +// - A writable filesystem abstraction is not yet implemented as of Go 1.20. +// See https://github.com/golang/go/issues/45757 type File interface { // Ino returns the inode (Stat_t.Ino) of this file, zero if unknown or an // error there was an error retrieving it. @@ -216,15 +219,13 @@ type File interface { // // A zero syscall.Errno is success. The below are expected otherwise: // - syscall.ENOSYS: the implementation does not support this function. - // - syscall.ENOTDIR: the file was not a directory + // - syscall.EBADF: the file was closed or not a directory. + // - syscall.ENOENT: the directory could not be read (e.g. deleted). // // # Notes // // - This is like `Readdir` on os.File, but unlike `readdir` in POSIX. // See https://pubs.opengroup.org/onlinepubs/9699919799/functions/readdir.html - // - For portability reasons, no error is returned at the end of the - // directory, when the file is closed or removed while open. - // See https://github.com/ziglang/zig/blob/0.10.1/lib/std/fs.zig#L635-L637 Readdir(n int) (dirents []Dirent, errno syscall.Errno) // ^-- TODO: consider being more like POSIX, for example, returning a // closeable Dirent object that can iterate on demand. This would @@ -371,8 +372,7 @@ type File interface { // Close closes the underlying file. // - // A zero syscall.Errno is success. The below are expected otherwise: - // - syscall.ENOSYS: the implementation does not support this function. + // A zero syscall.Errno is returned if unimplemented or success. // // # Notes // diff --git a/internal/sys/fs.go b/internal/sys/fs.go index 760de604..1f1856bb 100644 --- a/internal/sys/fs.go +++ b/internal/sys/fs.go @@ -8,7 +8,6 @@ import ( "github.com/tetratelabs/wazero/internal/descriptor" "github.com/tetratelabs/wazero/internal/fsapi" - "github.com/tetratelabs/wazero/internal/platform" socketapi "github.com/tetratelabs/wazero/internal/sock" "github.com/tetratelabs/wazero/internal/sysfs" ) @@ -53,6 +52,35 @@ type FileEntry struct { // File is always non-nil. File fsapi.File + + // openDir is nil until OpenDir was called. + openDir *Readdir +} + +// OpenDir lazy creates a directory stream for this file. +// +// # Parameters +// +// When `addDotEntries` is true, navigational entries "." and ".." precede +// any other entries in the directory. Otherwise, they will be absent. +// +// # Errors +// +// # This returns the same errors as fsapi.File Readdir +// +// Notes: +// - In the future, this may be refactored to allow multiple open directory +// streams per file (likely in wasip>1). +func (f *FileEntry) OpenDir(addDotEntries bool) (dir *Readdir, errno syscall.Errno) { + if dir = f.openDir; dir != nil { + return dir, 0 + } else if dir, errno = newReaddirFromFileEntry(f, addDotEntries); errno != 0 { + // not a directory or error reading it. + return nil, errno + } else { + f.openDir = dir + return dir, 0 + } } const direntBufSize = 16 @@ -134,11 +162,14 @@ func (d *Readdir) init() syscall.Errno { } // newReaddirFromFileEntry is a constructor for Readdir that takes a FileEntry to initialize. -func newReaddirFromFileEntry(f *FileEntry) (*Readdir, syscall.Errno) { - // Generate the dotEntries only once and return it many times in the dirInit closure. - dotEntries, errno := synthesizeDotEntries(f) - if errno != 0 { - return nil, errno +func newReaddirFromFileEntry(f *FileEntry, addDotEntries bool) (*Readdir, syscall.Errno) { + var dotEntries []fsapi.Dirent + if addDotEntries { + // Generate the dotEntries only once and return it many times in the dirInit closure. + var errno syscall.Errno + if dotEntries, errno = synthesizeDotEntries(f); errno != 0 { + return nil, errno + } } dirInit := func() ([]fsapi.Dirent, syscall.Errno) { // Ensure we always rewind to the beginning when we re-init. @@ -259,21 +290,12 @@ type FSContext struct { // (or directories) and defaults to empty. // TODO: This is unguarded, so not goroutine-safe! openedFiles FileTable - - // readdirs is a map of numeric identifiers to Readdir structs - // and defaults to empty. - // TODO: This is unguarded, so not goroutine-safe! - readdirs ReaddirTable } // FileTable is a specialization of the descriptor.Table type used to map file // descriptors to file entries. type FileTable = descriptor.Table[int32, *FileEntry] -// ReaddirTable is a specialization of the descriptor.Table type used to map -// file descriptors to Readdir structs. -type ReaddirTable = descriptor.Table[int32, *Readdir] - // RootFS returns a possibly unimplemented root filesystem. Any files that // should be added to the table should be inserted via InsertFile. // @@ -287,6 +309,11 @@ func (c *FSContext) RootFS() fsapi.FS { } } +// LookupFile returns a file if it is in the table. +func (c *FSContext) LookupFile(fd int32) (*FileEntry, bool) { + return c.openedFiles.Lookup(fd) +} + // OpenFile opens the file into the table and returns its file descriptor. // The result must be closed by CloseFile or Close. func (c *FSContext) OpenFile(fs fsapi.FS, path string, flag int, perm fs.FileMode) (int32, syscall.Errno) { @@ -307,6 +334,34 @@ func (c *FSContext) OpenFile(fs fsapi.FS, path string, flag int, perm fs.FileMod } } +// Renumber assigns the file pointed by the descriptor `from` to `to`. +func (c *FSContext) Renumber(from, to int32) syscall.Errno { + fromFile, ok := c.openedFiles.Lookup(from) + if !ok || to < 0 { + return syscall.EBADF + } else if fromFile.IsPreopen { + return syscall.ENOTSUP + } + + // If toFile is already open, we close it to prevent windows lock issues. + // + // The doc is unclear and other implementations do nothing for already-opened To FDs. + // https://github.com/WebAssembly/WASI/blob/snapshot-01/phases/snapshot/docs.md#-fd_renumberfd-fd-to-fd---errno + // https://github.com/bytecodealliance/wasmtime/blob/main/crates/wasi-common/src/snapshots/preview_1.rs#L531-L546 + if toFile, ok := c.openedFiles.Lookup(to); ok { + if toFile.IsPreopen { + return syscall.ENOTSUP + } + _ = toFile.File.Close() + } + + c.openedFiles.Delete(from) + if !c.openedFiles.InsertAt(fromFile, to) { + return syscall.EBADF + } + return 0 +} + // SockAccept accepts a socketapi.TCPConn into the file table and returns // its file descriptor. func (c *FSContext) SockAccept(sockFD int32, nonblock bool) (int32, syscall.Errno) { @@ -336,77 +391,17 @@ func (c *FSContext) SockAccept(sockFD int32, nonblock bool) (int32, syscall.Errn } } -// LookupFile returns a file if it is in the table. -func (c *FSContext) LookupFile(fd int32) (*FileEntry, bool) { - return c.openedFiles.Lookup(fd) -} - -// LookupReaddir returns a Readdir struct or creates an empty one if it was not present. -// -// Note: this currently assumes that idx == fd, where fd is the file descriptor of the directory. -// CloseFile will delete this idx from the internal store. In the future, idx may be independent -// of a file fd, and the idx may have to be disposed with an explicit CloseReaddir. -func (c *FSContext) LookupReaddir(idx int32, f *FileEntry) (*Readdir, syscall.Errno) { - if item, _ := c.readdirs.Lookup(idx); item != nil { - return item, 0 - } else { - item, err := newReaddirFromFileEntry(f) - if err != 0 { - return nil, err - } - ok := c.readdirs.InsertAt(item, idx) - if !ok { - return nil, syscall.EINVAL - } - return item, 0 - } -} - -// CloseReaddir delete the Readdir struct at the given index -// -// Note: Currently only necessary in tests. In the future, the idx will have to be disposed explicitly, -// unless we maintain a map fd -> []idx, and we let CloseFile close all the idx in []idx. -func (c *FSContext) CloseReaddir(idx int32) { - c.readdirs.Delete(idx) -} - -// Renumber assigns the file pointed by the descriptor `from` to `to`. -func (c *FSContext) Renumber(from, to int32) syscall.Errno { - fromFile, ok := c.openedFiles.Lookup(from) - if !ok || to < 0 { - return syscall.EBADF - } else if fromFile.IsPreopen { - return syscall.ENOTSUP - } - - // If toFile is already open, we close it to prevent windows lock issues. - // - // The doc is unclear and other implementations do nothing for already-opened To FDs. - // https://github.com/WebAssembly/WASI/blob/snapshot-01/phases/snapshot/docs.md#-fd_renumberfd-fd-to-fd---errno - // https://github.com/bytecodealliance/wasmtime/blob/main/crates/wasi-common/src/snapshots/preview_1.rs#L531-L546 - if toFile, ok := c.openedFiles.Lookup(to); ok { - if toFile.IsPreopen { - return syscall.ENOTSUP - } - _ = toFile.File.Close() - } - - c.openedFiles.Delete(from) - if !c.openedFiles.InsertAt(fromFile, to) { - return syscall.EBADF - } - return 0 -} - // CloseFile returns any error closing the existing file. -func (c *FSContext) CloseFile(fd int32) syscall.Errno { +func (c *FSContext) CloseFile(fd int32) (errno syscall.Errno) { f, ok := c.openedFiles.Lookup(fd) if !ok { return syscall.EBADF } + if errno = f.File.Close(); errno != 0 { + return errno + } c.openedFiles.Delete(fd) - c.readdirs.Delete(fd) - return platform.UnwrapOSError(f.File.Close()) + return errno } // Close implements io.Closer @@ -418,10 +413,8 @@ func (c *FSContext) Close() (err error) { } return true }) - // A closed FSContext cannot be reused so clear the state instead of - // using Reset. + // A closed FSContext cannot be reused so clear the state. c.openedFiles = FileTable{} - c.readdirs = ReaddirTable{} return } diff --git a/internal/sysfs/dir.go b/internal/sysfs/dir.go index 8462e846..c3256f9c 100644 --- a/internal/sysfs/dir.go +++ b/internal/sysfs/dir.go @@ -13,12 +13,12 @@ func adjustReaddirErr(f fsapi.File, isClosed bool, err error) syscall.Errno { return 0 // e.g. Readdir on darwin returns io.EOF, but linux doesn't. } else if errno := platform.UnwrapOSError(err); errno != 0 { errno = dirError(f, isClosed, errno) - // Ignore errors when the file was closed or removed. + // Comply with errors allowed on fsapi.File Readdir switch errno { - case syscall.EIO, syscall.EBADF: // closed while open - return 0 - case syscall.ENOENT: // Linux error when removed while open - return 0 + case syscall.EINVAL: // os.File Readdir can return this + return syscall.EBADF + case syscall.ENOTDIR: // dirError can return this + return syscall.EBADF } return errno } diff --git a/internal/sysfs/dir_test.go b/internal/sysfs/dir_test.go index 937afac3..dfa8fe1a 100644 --- a/internal/sysfs/dir_test.go +++ b/internal/sysfs/dir_test.go @@ -4,7 +4,6 @@ import ( "io" "io/fs" "os" - "path" "runtime" "sort" "syscall" @@ -57,11 +56,12 @@ func TestReaddir(t *testing.T) { testReaddirAll(t, dotF, tc.expectIno) }) - // Don't err if something else closed the directory while reading. + // Err if the caller closed the directory while reading. This is + // different from something else deleting it. t.Run("closed dir", func(t *testing.T) { require.EqualErrno(t, 0, dotF.Close()) _, errno := dotF.Readdir(-1) - require.EqualErrno(t, 0, errno) + require.EqualErrno(t, syscall.EBADF, errno) }) fileF, errno := sysfs.OpenFSFile(tc.fs, "empty.txt", syscall.O_RDONLY, 0) @@ -70,7 +70,7 @@ func TestReaddir(t *testing.T) { t.Run("file", func(t *testing.T) { _, errno := fileF.Readdir(-1) - require.EqualErrno(t, syscall.ENOTDIR, errno) + require.EqualErrno(t, syscall.EBADF, errno) }) dirF, errno := sysfs.OpenFSFile(tc.fs, "dir", syscall.O_RDONLY, 0) @@ -127,30 +127,6 @@ func TestReaddir(t *testing.T) { }) }) } - - // Don't err if something else removed the directory while reading. - t.Run("removed while open", func(t *testing.T) { - dirF, errno := sysfs.OpenFSFile(dirFS, "dir", syscall.O_RDONLY, 0) - require.EqualErrno(t, 0, errno) - defer dirF.Close() - - dirents, errno := dirF.Readdir(1) - require.EqualErrno(t, 0, errno) - require.Equal(t, 1, len(dirents)) - - // Speculatively try to remove even if it won't likely work - // on windows. - err := os.RemoveAll(path.Join(tmpDir, "dir")) - if err != nil && runtime.GOOS == "windows" { - t.Skip() - } else { - require.NoError(t, err) - } - - _, errno = dirF.Readdir(1) - require.EqualErrno(t, 0, errno) - // don't validate the contents as due to caching it might be present. - }) } func testReaddirAll(t *testing.T, dotF fsapi.File, expectIno bool) { diff --git a/internal/sysfs/dirfs_test.go b/internal/sysfs/dirfs_test.go index 5e052ead..705ac0dc 100644 --- a/internal/sysfs/dirfs_test.go +++ b/internal/sysfs/dirfs_test.go @@ -36,7 +36,7 @@ func TestNewDirFS(t *testing.T) { d, errno := testFS.OpenFile(".", os.O_RDONLY, 0) require.EqualErrno(t, 0, errno) _, errno = d.Readdir(-1) - require.EqualErrno(t, syscall.ENOTDIR, errno) + require.EqualErrno(t, syscall.EBADF, errno) }) } @@ -802,10 +802,7 @@ func TestDirFS_Truncate(t *testing.T) { }) } -// 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) { +func TestDirFS_Readdir(t *testing.T) { root := t.TempDir() testFS := NewDirFS(root) @@ -813,21 +810,46 @@ func Test_fdReaddir_opened_file_written(t *testing.T) { errno := testFS.Mkdir(readDirTarget, 0o700) require.EqualErrno(t, 0, errno) - // Open the directory, before writing files! + // Open the empty directory dirFile, errno := testFS.OpenFile(readDirTarget, os.O_RDONLY, 0) require.EqualErrno(t, 0, errno) defer dirFile.Close() - // Then write a file to the directory. - f, err := os.Create(path.Join(root, readDirTarget, "my-file")) - require.NoError(t, err) - defer f.Close() + // Write files to the directory after it is open. + require.NoError(t, os.WriteFile(path.Join(root, readDirTarget, "1"), nil, 0o444)) + require.NoError(t, os.WriteFile(path.Join(root, readDirTarget, "2"), nil, 0o444)) - dirents, errno := dirFile.Readdir(-1) - require.EqualErrno(t, 0, errno) + // Test files are visible. This fails in windows unless the implementation + // re-opens the underlying file. + // https://github.com/ziglang/zig/blob/e3736baddb8ecff90f0594be9f604c7484ce9aa2/lib/std/fs/test.zig#L290-L317 + t.Run("Sees files written after open", func(t *testing.T) { + dirents, errno := dirFile.Readdir(1) + require.EqualErrno(t, 0, errno) - require.Equal(t, 1, len(dirents)) - require.Equal(t, "my-file", dirents[0].Name) + require.Equal(t, 1, len(dirents)) + n := dirents[0].Name + switch n { + case "1", "2": // order is inconsistent on scratch images. + default: + require.Equal(t, "1", n) + } + }) + + // Test there is no error reading the directory if it was deleted while + // iterating. See docs on Readdir for why in general, but specifically Zig + // tests enforce this. This test is Windows sensitive as well. + // + // https://github.com/ziglang/zig/blob/e3736baddb8ecff90f0594be9f604c7484ce9aa2/lib/std/fs/test.zig#L311C1-L311C1 + t.Run("syscall.ENOENT or no error, deleted while reading", func(t *testing.T) { + require.NoError(t, os.RemoveAll(path.Join(root, readDirTarget))) + + dirents, errno := dirFile.Readdir(-1) + if errno != 0 { + require.EqualErrno(t, syscall.ENOENT, errno) + } + + require.Equal(t, 0, len(dirents)) + }) } func TestDirFS_Link(t *testing.T) { diff --git a/internal/sysfs/file.go b/internal/sysfs/file.go index 913b9fd1..d6f975e6 100644 --- a/internal/sysfs/file.go +++ b/internal/sysfs/file.go @@ -137,7 +137,7 @@ func (f *fsFile) cachedStat() (fileType fs.FileMode, ino uint64, errno syscall.E return f.cachedSt.fileType, f.cachedSt.ino, 0 } -// Ino implements File.Ino +// Ino implements the same method as documented on fsapi.File func (f *fsFile) Ino() (uint64, syscall.Errno) { if _, ino, errno := f.cachedStat(); errno != 0 { return 0, errno @@ -146,17 +146,17 @@ func (f *fsFile) Ino() (uint64, syscall.Errno) { } } -// IsAppend implements File.IsAppend +// IsAppend implements the same method as documented on fsapi.File func (f *fsFile) IsAppend() bool { return false } -// SetAppend implements File.SetAppend +// SetAppend implements the same method as documented on fsapi.File func (f *fsFile) SetAppend(bool) (errno syscall.Errno) { return fileError(f, f.closed, syscall.ENOSYS) } -// IsDir implements File.IsDir +// IsDir implements the same method as documented on fsapi.File func (f *fsFile) IsDir() (bool, syscall.Errno) { if ft, _, errno := f.cachedStat(); errno != 0 { return false, errno @@ -166,7 +166,7 @@ func (f *fsFile) IsDir() (bool, syscall.Errno) { return false, 0 } -// Stat implements File.Stat +// Stat implements the same method as documented on fsapi.File func (f *fsFile) Stat() (st fsapi.Stat_t, errno syscall.Errno) { if f.closed { errno = syscall.EBADF @@ -195,7 +195,7 @@ func (f *fsFile) cacheStat(st fsapi.Stat_t) (fsapi.Stat_t, syscall.Errno) { return st, 0 } -// Read implements File.Read +// Read implements the same method as documented on fsapi.File func (f *fsFile) Read(buf []byte) (n int, errno syscall.Errno) { if n, errno = read(f.file, buf); errno != 0 { // Defer validation overhead until we've already had an error. @@ -204,7 +204,7 @@ func (f *fsFile) Read(buf []byte) (n int, errno syscall.Errno) { return } -// Pread implements File.Pread +// Pread implements the same method as documented on fsapi.File func (f *fsFile) Pread(buf []byte, off int64) (n int, errno syscall.Errno) { if ra, ok := f.file.(io.ReaderAt); ok { if n, errno = pread(ra, buf, off); errno != 0 { @@ -243,7 +243,7 @@ func (f *fsFile) Pread(buf []byte, off int64) (n int, errno syscall.Errno) { return } -// Seek implements File.Seek. +// Seek implements the same method as documented on fsapi.File func (f *fsFile) Seek(offset int64, whence int) (newOffset int64, errno syscall.Errno) { // If this is a directory, and we're attempting to seek to position zero, // we have to re-open the file to ensure the directory state is reset. @@ -267,16 +267,17 @@ func (f *fsFile) Seek(offset int64, whence int) (newOffset int64, errno syscall. return } -func (f *fsFile) reopen() syscall.Errno { - _ = f.close() - var err error - f.file, err = f.fs.Open(f.name) - return platform.UnwrapOSError(err) -} - // Readdir implements File.Readdir. Notably, this uses fs.ReadDirFile if // available. func (f *fsFile) Readdir(n int) (dirents []fsapi.Dirent, errno syscall.Errno) { + // Windows lets you Readdir after close, fs.File also may not implement + // close in a meaningful way. read our closed field to return consistent + // results. + if f.closed { + errno = syscall.EBADF + return + } + if of, ok := f.file.(*os.File); ok { // We can't use f.name here because it is the path up to the fsapi.FS, // not necessarily the real path. For this reason, Windows may not be @@ -300,12 +301,12 @@ func (f *fsFile) Readdir(n int) (dirents []fsapi.Dirent, errno syscall.Errno) { dirents = append(dirents, fsapi.Dirent{Name: e.Name(), Type: e.Type()}) } } else { - errno = syscall.ENOTDIR + errno = syscall.EBADF // not a directory } return } -// Write implements File.Write +// Write implements the same method as documented on fsapi.File. func (f *fsFile) Write(buf []byte) (n int, errno syscall.Errno) { if w, ok := f.file.(io.Writer); ok { if n, errno = write(w, buf); errno != 0 { @@ -318,7 +319,7 @@ func (f *fsFile) Write(buf []byte) (n int, errno syscall.Errno) { return } -// Pwrite implements File.Pwrite +// Pwrite implements the same method as documented on fsapi.File. func (f *fsFile) Pwrite(buf []byte, off int64) (n int, errno syscall.Errno) { if wa, ok := f.file.(io.WriterAt); ok { if n, errno = pwrite(wa, buf, off); errno != 0 { @@ -331,7 +332,7 @@ func (f *fsFile) Pwrite(buf []byte, off int64) (n int, errno syscall.Errno) { return } -// Close implements File.Close +// Close implements the same method as documented on fsapi.File. func (f *fsFile) Close() syscall.Errno { if f.closed { return 0 @@ -406,6 +407,21 @@ func seek(s io.Seeker, offset int64, whence int) (int64, syscall.Errno) { return newOffset, platform.UnwrapOSError(err) } +// reopenFile allows re-opening a file for reasons such as applying flags or +// directory iteration. +type reopenFile func() syscall.Errno + +// compile-time check to ensure fsFile.reopen implements reopenFile. +var _ reopenFile = (*fsFile)(nil).reopen + +// reopen implements the same method as documented on reopenFile. +func (f *fsFile) reopen() syscall.Errno { + _ = f.close() + var err error + f.file, err = f.fs.Open(f.name) + return platform.UnwrapOSError(err) +} + func readdir(f *os.File, path string, n int) (dirents []fsapi.Dirent, errno syscall.Errno) { fis, e := f.Readdir(n) if errno = platform.UnwrapOSError(e); errno != 0 { diff --git a/internal/sysfs/open_file.go b/internal/sysfs/open_file.go index 473f8ca6..ebe9e3d5 100644 --- a/internal/sysfs/open_file.go +++ b/internal/sysfs/open_file.go @@ -7,14 +7,9 @@ import ( "os" "syscall" - "github.com/tetratelabs/wazero/internal/fsapi" "github.com/tetratelabs/wazero/internal/platform" ) -func newOsFile(openPath string, openFlag int, openPerm fs.FileMode, f *os.File) fsapi.File { - return newDefaultOsFile(openPath, openFlag, openPerm, f) -} - // OpenFile is like os.OpenFile except it returns syscall.Errno. A zero // syscall.Errno is success. func openFile(path string, flag int, perm fs.FileMode) (*os.File, syscall.Errno) { diff --git a/internal/sysfs/open_file_js.go b/internal/sysfs/open_file_js.go index e473acbe..d84e2a37 100644 --- a/internal/sysfs/open_file_js.go +++ b/internal/sysfs/open_file_js.go @@ -8,10 +8,6 @@ import ( "github.com/tetratelabs/wazero/internal/platform" ) -func newOsFile(openPath string, openFlag int, openPerm fs.FileMode, f *os.File) File { - return newDefaultOsFile(openPath, openFlag, openPerm, f) -} - func openFile(path string, flag int, perm fs.FileMode) (*os.File, syscall.Errno) { flag &= ^(O_DIRECTORY | O_NOFOLLOW) // erase placeholders f, err := os.OpenFile(path, flag, perm) diff --git a/internal/sysfs/open_file_sun.go b/internal/sysfs/open_file_sun.go index e23b7185..576c8085 100644 --- a/internal/sysfs/open_file_sun.go +++ b/internal/sysfs/open_file_sun.go @@ -7,14 +7,9 @@ import ( "os" "syscall" - "github.com/tetratelabs/wazero/internal/fsapi" "github.com/tetratelabs/wazero/internal/platform" ) -func newOsFile(openPath string, openFlag int, openPerm fs.FileMode, f *os.File) fsapi.File { - return newDefaultOsFile(openPath, openFlag, openPerm, f) -} - func openFile(path string, flag int, perm fs.FileMode) (*os.File, syscall.Errno) { f, err := os.OpenFile(path, flag, perm) return f, platform.UnwrapOSError(err) diff --git a/internal/sysfs/open_file_windows.go b/internal/sysfs/open_file_windows.go index d9297d7e..5000ae85 100644 --- a/internal/sysfs/open_file_windows.go +++ b/internal/sysfs/open_file_windows.go @@ -11,12 +11,6 @@ import ( "github.com/tetratelabs/wazero/internal/platform" ) -func newOsFile(openPath string, openFlag int, openPerm fs.FileMode, f *os.File) fsapi.File { - return &windowsOsFile{ - osFile: osFile{path: openPath, flag: openFlag, perm: openPerm, file: f, fd: f.Fd()}, - } -} - func openFile(path string, flag int, perm fs.FileMode) (*os.File, syscall.Errno) { isDir := flag&fsapi.O_DIRECTORY > 0 flag &= ^(fsapi.O_DIRECTORY | fsapi.O_NOFOLLOW) // erase placeholders @@ -154,46 +148,3 @@ func open(path string, mode int, perm uint32) (fd syscall.Handle, err error) { h, e := syscall.CreateFile(pathp, access, sharemode, sa, createmode, attrs, 0) return h, e } - -// windowsOsFile overrides osFile to special case directory handling in Windows. -type windowsOsFile struct { - osFile - - dirInitialized bool -} - -// Readdir implements File.Readdir -func (f *windowsOsFile) Readdir(n int) (dirents []fsapi.Dirent, errno syscall.Errno) { - if errno = f.maybeInitDir(); errno != 0 { - return - } - - return f.osFile.Readdir(n) -} - -func (f *windowsOsFile) maybeInitDir() syscall.Errno { - if f.dirInitialized { - return 0 - } - - if isDir, errno := f.IsDir(); errno != 0 { - return errno - } else if !isDir { - return syscall.ENOTDIR - } - - // On Windows, once the directory is opened, changes to the directory are - // not visible on ReadDir on that already-opened file handle. - // - // To provide consistent behavior with other platforms, we re-open it. - if errno := f.osFile.Close(); errno != 0 { - return errno - } - newW, errno := openFile(f.path, f.flag, f.perm) - if errno != 0 { - return errno - } - f.osFile.file = newW - f.dirInitialized = true - return 0 -} diff --git a/internal/sysfs/osfile.go b/internal/sysfs/osfile.go index 95798f48..74436e41 100644 --- a/internal/sysfs/osfile.go +++ b/internal/sysfs/osfile.go @@ -4,6 +4,7 @@ import ( "io" "io/fs" "os" + "runtime" "syscall" "time" @@ -11,8 +12,12 @@ import ( "github.com/tetratelabs/wazero/internal/platform" ) -func newDefaultOsFile(openPath string, openFlag int, openPerm fs.FileMode, f *os.File) fsapi.File { - return &osFile{path: openPath, flag: openFlag, perm: openPerm, file: f, fd: f.Fd()} +func newOsFile(openPath string, openFlag int, openPerm fs.FileMode, f *os.File) fsapi.File { + // Windows cannot read files written to a directory after it was opened. + // This was noticed in #1087 in zig tests. Use a flag instead of a + // different type. + reopenDir := runtime.GOOS == "windows" + return &osFile{path: openPath, flag: openFlag, perm: openPerm, reopenDir: reopenDir, file: f, fd: f.Fd()} } // osFile is a file opened with this package, and uses os.File or syscalls to @@ -24,6 +29,9 @@ type osFile struct { file *os.File fd uintptr + // reopenDir is true if reopen should be called before Readdir. + reopenDir bool + // closed is true when closed was called. This ensures proper syscall.EBADF closed bool @@ -72,6 +80,9 @@ func (f *osFile) SetAppend(enable bool) (errno syscall.Errno) { return fileError(f, f.closed, f.reopen()) } +// compile-time check to ensure osFile.reopen implements reopenFile. +var _ reopenFile = (*fsFile)(nil).reopen + func (f *osFile) reopen() (errno syscall.Errno) { // Clear any create flag, as we are re-opening, not re-creating. f.flag &= ^syscall.O_CREAT @@ -183,6 +194,13 @@ func (f *osFile) PollRead(timeout *time.Duration) (ready bool, errno syscall.Err // Readdir implements File.Readdir. Notably, this uses "Readdir", not // "ReadDir", from os.File. func (f *osFile) Readdir(n int) (dirents []fsapi.Dirent, errno syscall.Errno) { + if f.reopenDir { // Lazy re-open the directory if needed. + f.reopenDir = false + if errno = adjustReaddirErr(f, f.closed, f.reopen()); errno != 0 { + return + } + } + if dirents, errno = readdir(f.file, f.path, n); errno != 0 { errno = adjustReaddirErr(f, f.closed, errno) }