Repository: tetratelabs/wazero
Pull request #1472 (2023-05-16 02:07:50)

Removes AccessMode from platform.File
In the beginning, we checked by type if a command like read or write might succeed. Temporarily, we switched to exposing the AccessMode, which could be incorrect or drift. Now, we use the underlying error instead. Removing `platform.AccessMode` was a TODO left until the code was consolidated enough to address it coherently.


Patch:
diff --git a/imports/assemblyscript/assemblyscript.go b/imports/assemblyscript/assemblyscript.go
index 96ad8655..7c96bdc8 100644
--- a/imports/assemblyscript/assemblyscript.go
+++ b/imports/assemblyscript/assemblyscript.go
@@ -31,7 +31,6 @@ import (
 	"io"
 	"strconv"
 	"strings"
-	"syscall"
 	"unicode/utf16"
 
 	"github.com/tetratelabs/wazero"
@@ -165,12 +164,11 @@ func abortWithMessage(ctx context.Context, mod api.Module, stack []uint64) {
 	columnNumber := uint32(stack[3])
 
 	// Don't panic if there was a problem reading the message
-	stderr := stdioFile(fsc, internalsys.FdStderr)
-	if stderr != nil {
+	if stderr, ok := fsc.LookupFile(internalsys.FdStderr); ok {
 		if msg, msgOk := readAssemblyScriptString(mem, message); msgOk && stderr != nil {
 			if fn, fnOk := readAssemblyScriptString(mem, fileName); fnOk {
 				s := fmt.Sprintf("%s at %s:%d:%d\n", msg, fn, lineNumber, columnNumber)
-				_, _ = stderr.Write([]byte(s))
+				_, _ = stderr.File.Write([]byte(s))
 			}
 		}
 	}
@@ -202,7 +200,9 @@ var traceStdout = &wasm.HostFunc{
 	Code: wasm.Code{
 		GoFunc: api.GoModuleFunc(func(_ context.Context, mod api.Module, stack []uint64) {
 			fsc := mod.(*wasm.ModuleInstance).Sys.FS()
-			traceTo(mod, stack, stdioFile(fsc, internalsys.FdStdout))
+			if stdout, ok := fsc.LookupFile(internalsys.FdStdout); ok {
+				traceTo(mod, stack, stdout.File)
+			}
 		}),
 	},
 }
@@ -210,7 +210,9 @@ var traceStdout = &wasm.HostFunc{
 // traceStderr implements trace to the configured Stderr.
 var traceStderr = traceStdout.WithGoModuleFunc(func(_ context.Context, mod api.Module, stack []uint64) {
 	fsc := mod.(*wasm.ModuleInstance).Sys.FS()
-	traceTo(mod, stack, stdioFile(fsc, internalsys.FdStderr))
+	if stderr, ok := fsc.LookupFile(internalsys.FdStderr); ok {
+		traceTo(mod, stack, stderr.File)
+	}
 })
 
 // traceTo implements the function "trace" in AssemblyScript. e.g.
@@ -224,9 +226,6 @@ var traceStderr = traceStdout.WithGoModuleFunc(func(_ context.Context, mod api.M
 //
 // See https://github.com/AssemblyScript/assemblyscript/blob/fa14b3b03bd4607efa52aaff3132bea0c03a7989/std/assembly/wasi/index.ts#L61
 func traceTo(mod api.Module, params []uint64, file platform.File) {
-	if file == nil {
-		return // closed
-	}
 	message := uint32(params[0])
 	nArgs := uint32(params[1])
 	arg0 := api.DecodeF64(params[2])
@@ -323,13 +322,3 @@ func decodeUTF16(b []byte) string {
 
 	return string(utf16.Decode(u16s))
 }
-
-func stdioFile(fsc *internalsys.FSContext, fd int32) platform.File {
-	if f, ok := fsc.LookupFile(fd); !ok {
-		return nil
-	} else if f.File.AccessMode() == syscall.O_RDONLY {
-		return nil
-	} else {
-		return f.File
-	}
-}
diff --git a/imports/wasi_snapshot_preview1/fs.go b/imports/wasi_snapshot_preview1/fs.go
index a7cd46d0..8520d663 100644
--- a/imports/wasi_snapshot_preview1/fs.go
+++ b/imports/wasi_snapshot_preview1/fs.go
@@ -753,8 +753,6 @@ func fdReadOrPread(mod api.Module, params []uint64, isPread bool) syscall.Errno
 	var reader func(buf []byte) (n int, errno syscall.Errno)
 	if f, ok := fsc.LookupFile(fd); !ok {
 		return syscall.EBADF
-	} else if f.File.AccessMode() == syscall.O_WRONLY {
-		return syscall.EBADF
 	} else if isPread {
 		offset := int64(params[3])
 		reader = (&preader{f: f.File, offset: offset}).Read
@@ -787,7 +785,9 @@ func fdReadOrPread(mod api.Module, params []uint64, isPread bool) syscall.Errno
 		n, errno := reader(b)
 		nread += uint32(n)
 
-		if errno != 0 {
+		if errno == syscall.ENOSYS {
+			return syscall.EBADF // e.g. unimplemented for read
+		} else if errno != 0 {
 			return errno
 		} else if n < int(l) {
 			break // stop when we read less than capacity.
@@ -1328,8 +1328,6 @@ func fdWriteOrPwrite(mod api.Module, params []uint64, isPwrite bool) syscall.Err
 	var writer func(buf []byte) (n int, errno syscall.Errno)
 	if f, ok := fsc.LookupFile(fd); !ok {
 		return syscall.EBADF
-	} else if f.File.AccessMode() == syscall.O_RDONLY {
-		return syscall.EBADF
 	} else if isPwrite {
 		offset := int64(params[3])
 		writer = (&pwriter{f: f.File, offset: offset}).Write
@@ -1356,7 +1354,9 @@ func fdWriteOrPwrite(mod api.Module, params []uint64, isPwrite bool) syscall.Err
 		}
 		n, errno := writer(b)
 		nwritten += uint32(n)
-		if errno != 0 {
+		if errno == syscall.ENOSYS {
+			return syscall.EBADF // e.g. unimplemented for write
+		} else if errno != 0 {
 			return errno
 		}
 	}
diff --git a/imports/wasi_snapshot_preview1/poll.go b/imports/wasi_snapshot_preview1/poll.go
index c3097277..1d8ec7f9 100644
--- a/imports/wasi_snapshot_preview1/poll.go
+++ b/imports/wasi_snapshot_preview1/poll.go
@@ -234,10 +234,8 @@ func processFDEventRead(fsc *internalsys.FSContext, fd int32) wasip1.Errno {
 
 // processFDEventWrite returns ErrnoNotsup if the file exists and ErrnoBadf otherwise.
 func processFDEventWrite(fsc *internalsys.FSContext, fd int32) wasip1.Errno {
-	if f, ok := fsc.LookupFile(fd); ok {
-		if f.File.AccessMode() != syscall.O_RDONLY {
-			return wasip1.ErrnoNotsup
-		}
+	if _, ok := fsc.LookupFile(fd); ok {
+		return wasip1.ErrnoNotsup
 	}
 	return wasip1.ErrnoBadf
 }
diff --git a/internal/gojs/fs.go b/internal/gojs/fs.go
index ef429f70..cc7c66a4 100644
--- a/internal/gojs/fs.go
+++ b/internal/gojs/fs.go
@@ -288,13 +288,14 @@ func syscallWrite(mod api.Module, fd int32, offset interface{}, buf []byte) (n i
 	fsc := mod.(*wasm.ModuleInstance).Sys.FS()
 	if f, ok := fsc.LookupFile(fd); !ok {
 		errno = syscall.EBADF
-	} else if f.File.AccessMode() == syscall.O_RDONLY {
-		errno = syscall.EBADF
 	} else if offset != nil {
 		n, errno = f.File.Pwrite(buf, toInt64(offset))
 	} else {
 		n, errno = f.File.Write(buf)
 	}
+	if errno == syscall.ENOSYS {
+		errno = syscall.EBADF // e.g. unimplemented for write
+	}
 	return
 }
 
diff --git a/internal/gojs/runtime.go b/internal/gojs/runtime.go
index 195687f8..dc6c3a0b 100644
--- a/internal/gojs/runtime.go
+++ b/internal/gojs/runtime.go
@@ -42,9 +42,17 @@ func wasmWrite(_ context.Context, mod api.Module, stack goarch.Stack) {
 	p := stack.ParamBytes(mod.Memory(), 1 /*, 2 */)
 
 	fsc := mod.(*wasm.ModuleInstance).Sys.FS()
-	if f, ok := fsc.LookupFile(fd); ok && f.File.AccessMode() != syscall.O_RDONLY {
-		if _, err := f.File.Write(p); err != 0 {
-			panic(fmt.Errorf("error writing p: %w", err))
+	if f, ok := fsc.LookupFile(fd); ok {
+		_, errno := f.File.Write(p)
+		switch errno {
+		case 0:
+			return // success
+		case syscall.ENOSYS:
+			return // e.g. unimplemented for write
+		case syscall.EBADF:
+			return // e.g. not opened for write
+		default:
+			panic(fmt.Errorf("error writing p: %w", errno))
 		}
 	} else {
 		panic(fmt.Errorf("fd %d invalid", fd))
diff --git a/internal/platform/dir.go b/internal/platform/dir.go
index 30e0b2a4..994c951b 100644
--- a/internal/platform/dir.go
+++ b/internal/platform/dir.go
@@ -56,11 +56,6 @@ func adjustReaddirErr(f File, isClosed bool, err error) syscall.Errno {
 // DirFile is embeddable to reduce the amount of functions to implement a file.
 type DirFile struct{}
 
-// AccessMode implements File.AccessMode
-func (DirFile) AccessMode() int {
-	return syscall.O_RDONLY
-}
-
 // IsAppend implements File.IsAppend
 func (DirFile) IsAppend() bool {
 	return false
diff --git a/internal/platform/file.go b/internal/platform/file.go
index 2ca644c2..4a0e9cb0 100644
--- a/internal/platform/file.go
+++ b/internal/platform/file.go
@@ -42,15 +42,6 @@ type File interface {
 	//   - Some implementations implement this with a cached call to Stat.
 	Ino() (uint64, syscall.Errno)
 
-	// AccessMode returns the access mode the file was opened with.
-	//
-	// This returns exclusively one of the following:
-	//   - syscall.O_RDONLY: read-only, e.g. os.Stdin
-	//   - syscall.O_WRONLY: write-only, e.g. os.Stdout
-	//   - syscall.O_RDWR: read-write, e.g. os.CreateTemp
-	AccessMode() int
-	// ^-- TODO: see if we can remove this
-
 	// IsNonblock returns true if SetNonblock was successfully enabled on this
 	// file.
 	//
@@ -507,18 +498,18 @@ func NewStdioFile(stdin bool, f fs.File) (File, error) {
 	} else {
 		mode = st.Mode()
 	}
-	var accessMode int
+	var flag int
 	if stdin {
-		accessMode = syscall.O_RDONLY
+		flag = syscall.O_RDONLY
 	} else {
-		accessMode = syscall.O_WRONLY
+		flag = syscall.O_WRONLY
 	}
 	var file File
 	if of, ok := f.(*os.File); ok {
 		// This is ok because functions that need path aren't used by stdioFile
-		file = newOsFile("", accessMode, 0, of)
+		file = newOsFile("", flag, 0, of)
 	} else {
-		file = &fsFile{accessMode: accessMode, file: f}
+		file = &fsFile{file: f}
 	}
 	return &stdioFile{File: file, st: Stat_t{Mode: mode, Nlink: 1}}, nil
 }
@@ -548,12 +539,7 @@ func OpenFSFile(fs fs.FS, path string, flag int, perm fs.FileMode) (File, syscal
 	}
 	// Don't return an os.File because the path is not absolute. osFile needs
 	// the path to be real and certain fs.File impls are subrooted.
-	return &fsFile{
-		fs:         fs,
-		name:       path,
-		accessMode: flag & (syscall.O_RDONLY | syscall.O_WRONLY | syscall.O_RDWR),
-		file:       f,
-	}, 0
+	return &fsFile{fs: fs, name: path, file: f}, 0
 }
 
 type stdioFile struct {
@@ -589,9 +575,6 @@ type fsFile struct {
 	// name is what was used in fs for Open, so it may not be the actual path.
 	name string
 
-	// accessMode is only set when OpenFSFile opened this file.
-	accessMode int
-
 	// file is always set, possibly an os.File like os.Stdin.
 	file fs.File
 
@@ -630,11 +613,6 @@ func (f *fsFile) Ino() (uint64, syscall.Errno) {
 	}
 }
 
-// AccessMode implements File.AccessMode
-func (f *fsFile) AccessMode() int {
-	return f.accessMode
-}
-
 // IsAppend implements File.IsAppend
 func (f *fsFile) IsAppend() bool {
 	return false
diff --git a/internal/platform/osfile.go b/internal/platform/osfile.go
index 674272a4..8560f52d 100644
--- a/internal/platform/osfile.go
+++ b/internal/platform/osfile.go
@@ -47,11 +47,6 @@ func (f *osFile) Ino() (uint64, syscall.Errno) {
 	}
 }
 
-// AccessMode implements File.AccessMode
-func (f *osFile) AccessMode() int {
-	return f.flag & (syscall.O_RDONLY | syscall.O_WRONLY | syscall.O_RDWR)
-}
-
 // IsAppend implements File.IsAppend
 func (f *osFile) IsAppend() bool {
 	return f.flag&syscall.O_APPEND == syscall.O_APPEND
diff --git a/internal/sysfs/readfs.go b/internal/sysfs/readfs.go
index 2c84ae52..0d6852a3 100644
--- a/internal/sysfs/readfs.go
+++ b/internal/sysfs/readfs.go
@@ -80,11 +80,6 @@ func (r *readFile) Ino() (uint64, syscall.Errno) {
 	return r.f.Ino()
 }
 
-// AccessMode implements the same method as documented on platform.File.
-func (r *readFile) AccessMode() int {
-	return r.f.AccessMode()
-}
-
 // IsNonblock implements the same method as documented on platform.File.
 func (r *readFile) IsNonblock() bool {
 	return r.f.IsNonblock()


Test patch:
diff --git a/imports/wasi_snapshot_preview1/fs_test.go b/imports/wasi_snapshot_preview1/fs_test.go
index c09dbe8b..2c15b8fa 100644
--- a/imports/wasi_snapshot_preview1/fs_test.go
+++ b/imports/wasi_snapshot_preview1/fs_test.go
@@ -19,7 +19,6 @@ import (
 	"github.com/tetratelabs/wazero"
 	"github.com/tetratelabs/wazero/api"
 	"github.com/tetratelabs/wazero/internal/fstest"
-	"github.com/tetratelabs/wazero/internal/leb128"
 	"github.com/tetratelabs/wazero/internal/platform"
 	"github.com/tetratelabs/wazero/internal/sys"
 	"github.com/tetratelabs/wazero/internal/sysfs"
@@ -3040,10 +3039,10 @@ func Test_fdWrite_Errors(t *testing.T) {
 		{
 			name:          "length to read exceeds memory by 1",
 			fd:            fd,
-			iovs:          memSize - 9, // iovs[0].offset (where to read "hi") is in memory, but truncated.
+			iovs:          memSize - 7, // iovs[0].offset (where to read "hi") is in memory, but truncated.
 			expectedErrno: wasip1.ErrnoFault,
 			expectedLog: `
-==> wasi_snapshot_preview1.fd_write(fd=4,iovs=65527,iovs_len=1)
+==> wasi_snapshot_preview1.fd_write(fd=4,iovs=65529,iovs_len=1)
 <== (nwritten=,errno=EFAULT)
 `,
 		},
@@ -3065,7 +3064,7 @@ func Test_fdWrite_Errors(t *testing.T) {
 			defer log.Reset()
 
 			mod.Memory().Write(tc.iovs, append(
-				leb128.EncodeUint32(tc.iovs+8), // = iovs[0].offset (where the data "hi" begins)
+				u64.LeBytes(uint64(tc.iovs+8)), // = iovs[0].offset (where the data "hi" begins)
 				// = iovs[0].length (how many bytes are in "hi")
 				2, 0, 0, 0,
 				'h', 'i', // iovs[0].length bytes
diff --git a/internal/platform/file_test.go b/internal/platform/file_test.go
index fc99ff74..8bbb5fc7 100644
--- a/internal/platform/file_test.go
+++ b/internal/platform/file_test.go
@@ -24,11 +24,6 @@ type NoopFile struct {
 	UnimplementedFile
 }
 
-// The current design requires the user to implement AccessMode.
-func (NoopFile) AccessMode() int {
-	return syscall.O_RDONLY
-}
-
 // The current design requires the user to consciously implement Close.
 // However, we could change UnimplementedFile to return zero.
 func (NoopFile) Close() (errno syscall.Errno) { return }
@@ -953,16 +948,6 @@ func TestNewStdioFile(t *testing.T) {
 			require.Zero(t, st.Mtim)
 			require.Zero(t, st.Atim)
 		})
-
-		t.Run(tc.name+" AccessMode", func(t *testing.T) {
-			accessMode := tc.f.AccessMode()
-			switch tc.f {
-			case stdin, stdinFile:
-				require.Equal(t, syscall.O_RDONLY, accessMode)
-			case stdout, stdoutFile:
-				require.Equal(t, syscall.O_WRONLY, accessMode)
-			}
-		})
 	}
 }