Repository: tetratelabs/wazero
Pull request #1295 (2023-03-27 09:35:28)

fs: stat and cache mode for stdio devices
fs: stat and cache mode for stdio devices

Ensure that stdio device modes are consistent with the given
file descriptors by stat'ing, instead of returning mocks.

Use `Stat()` on `poll_oneoff()` too, instead of `IsTerminal()`,
thus avoiding a useless syscall.

Delete leftover type decl `fileModeStat`.

Delete IsPlatform() and related, as they are superseded by the stat() check.

Signed-off-by: Edoardo Vacchi <evacchi@users.noreply.github.com>

---
I originally noticed this because I was [trying to run this Python interpreter but the REPL hung](https://github.com/vmware-labs/webassembly-language-runtimes/releases/tag/python%2F3.11.1%2B20230217-15dfbed).

The main culprit is that we are stubbing stdin/out/err and `fs.ModeCharDevice` must imply `fs.ModeDevice`; this in turn causes 

https://github.com/tetratelabs/wazero/blob/a4226906cf30ba5261d2fab2ff4df62a8109d6ea/imports/wasi_snapshot_preview1/fs.go#L360-L365

to return UNKNOWN instead (value 0)

I have dug further, and it seems that we are defaulting stdin/stdout/stderr to be block devices instead of CharDevices

https://github.com/tetratelabs/wazero/blob/372e0222ddb7dbc64bc7829fc112dd0b88a15e9e/internal/sys/fs.go#L83-L87

this in turn causes my new `TestStdioStat` to fail, but if I flip those defaults I cause _other_ tests to fail.

Hence, it follows that we are not really being consistent with the operating system, but we are testing our own mocks (at least in some cases) 🤔 

~Draft for now.~

evacchi: In practice, my doubts amount to: "shouldn't we stat stdin/stderr/stdout at boot time instead of mocking?"

Patch:
diff --git a/imports/wasi_snapshot_preview1/poll.go b/imports/wasi_snapshot_preview1/poll.go
index 2ccc29b1..43ca00a4 100644
--- a/imports/wasi_snapshot_preview1/poll.go
+++ b/imports/wasi_snapshot_preview1/poll.go
@@ -2,10 +2,10 @@ package wasi_snapshot_preview1
 
 import (
 	"context"
+	"io/fs"
 	"syscall"
 
 	"github.com/tetratelabs/wazero/api"
-	"github.com/tetratelabs/wazero/internal/platform"
 	internalsys "github.com/tetratelabs/wazero/internal/sys"
 	"github.com/tetratelabs/wazero/internal/wasip1"
 	"github.com/tetratelabs/wazero/internal/wasm"
@@ -142,9 +142,10 @@ func processFDEvent(mod api.Module, eventType byte, inBuf []byte) syscall.Errno
 	// files.
 	errno := syscall.ENOTSUP
 	if eventType == wasip1.EventTypeFdRead {
-		if _, ok := fsc.LookupFile(fd); ok {
-			// If fd is a pipe, IsTerminal is false.
-			if platform.IsTerminal(uintptr(fd)) {
+		if f, ok := fsc.LookupFile(fd); ok {
+			st, _ := f.Stat()
+			// if fd is a pipe, then it is not a char device (a tty)
+			if st.Mode&fs.ModeCharDevice != 0 {
 				errno = syscall.EBADF
 			}
 		} else {
diff --git a/internal/platform/platform.go b/internal/platform/platform.go
index ccfa1a75..7cf6230c 100644
--- a/internal/platform/platform.go
+++ b/internal/platform/platform.go
@@ -51,8 +51,3 @@ func MunmapCodeSegment(code []byte) error {
 	}
 	return munmapCodeSegment(code)
 }
-
-// IsTerminal returns true if the given file descriptor is a terminal.
-func IsTerminal(fd uintptr) bool {
-	return isTerminal(fd)
-}
diff --git a/internal/platform/terminal.go b/internal/platform/terminal.go
deleted file mode 100644
index 326a16b8..00000000
--- a/internal/platform/terminal.go
+++ /dev/null
@@ -1,14 +0,0 @@
-//go:build darwin || linux || freebsd
-
-package platform
-
-import (
-	"syscall"
-	"unsafe"
-)
-
-func isTerminal(fd uintptr) bool {
-	var val syscall.Termios
-	_, _, err := syscall.Syscall6(syscall.SYS_IOCTL, fd, ioctlReadTermios, uintptr(unsafe.Pointer(&val)), 0, 0, 0)
-	return err == 0
-}
diff --git a/internal/platform/terminal_bsd.go b/internal/platform/terminal_bsd.go
deleted file mode 100644
index 658a0a40..00000000
--- a/internal/platform/terminal_bsd.go
+++ /dev/null
@@ -1,7 +0,0 @@
-//go:build darwin || freebsd
-
-package platform
-
-import "syscall"
-
-const ioctlReadTermios = syscall.TIOCGETA
diff --git a/internal/platform/terminal_linux.go b/internal/platform/terminal_linux.go
deleted file mode 100644
index 2f4bc3b9..00000000
--- a/internal/platform/terminal_linux.go
+++ /dev/null
@@ -1,5 +0,0 @@
-package platform
-
-import "syscall"
-
-const ioctlReadTermios = syscall.TCGETS
diff --git a/internal/platform/terminal_unsupported.go b/internal/platform/terminal_unsupported.go
deleted file mode 100644
index 8e05593f..00000000
--- a/internal/platform/terminal_unsupported.go
+++ /dev/null
@@ -1,7 +0,0 @@
-//go:build !(darwin || linux || freebsd || windows)
-
-package platform
-
-func isTerminal(fd uintptr) bool {
-	return false
-}
diff --git a/internal/platform/terminal_windows.go b/internal/platform/terminal_windows.go
deleted file mode 100644
index fb4d1d41..00000000
--- a/internal/platform/terminal_windows.go
+++ /dev/null
@@ -1,32 +0,0 @@
-package platform
-
-import (
-	"syscall"
-	"unsafe"
-)
-
-var procGetConsoleMode = kernel32.NewProc("GetConsoleMode")
-
-func isTerminal(fd uintptr) bool {
-	handle := mapToWindowsHandle(fd)
-
-	var st uint32
-	r, _, e := syscall.Syscall(procGetConsoleMode.Addr(), 2, handle, uintptr(unsafe.Pointer(&st)), 0)
-	return r != 0 && e == 0
-}
-
-// mapToWindowsHandle maps file descriptors 0..2 to a valid Windows handle
-func mapToWindowsHandle(fd uintptr) uintptr {
-	var handle uintptr
-	switch fd {
-	case 0:
-		handle = uintptr(syscall.Stdin)
-	case 1:
-		handle = uintptr(syscall.Stdout)
-	case 2:
-		handle = uintptr(syscall.Stderr)
-	default:
-		handle = fd
-	}
-	return handle
-}
diff --git a/internal/sys/fs.go b/internal/sys/fs.go
index 7504a14a..29ed47b7 100644
--- a/internal/sys/fs.go
+++ b/internal/sys/fs.go
@@ -34,7 +34,7 @@ const (
 
 const (
 	modeDevice     = uint32(fs.ModeDevice | 0o640)
-	modeCharDevice = uint32(fs.ModeCharDevice | 0o640)
+	modeCharDevice = uint32(fs.ModeDevice | fs.ModeCharDevice | 0o640)
 )
 
 type stdioFileWriter struct {
@@ -254,9 +254,21 @@ type FileTable = descriptor.Table[uint32, *FileEntry]
 // the file descriptor table as FdPreopen.
 func NewFSContext(stdin io.Reader, stdout, stderr io.Writer, rootFS sysfs.FS) (fsc *FSContext, err error) {
 	fsc = &FSContext{rootFS: rootFS}
-	fsc.openedFiles.Insert(stdinReader(stdin))
-	fsc.openedFiles.Insert(stdioWriter(stdout, noopStdoutStat))
-	fsc.openedFiles.Insert(stdioWriter(stderr, noopStderrStat))
+	inReader, err := stdinReader(stdin)
+	if err != nil {
+		return nil, err
+	}
+	fsc.openedFiles.Insert(inReader)
+	outWriter, err := stdioWriter(stdout, noopStdoutStat)
+	if err != nil {
+		return nil, err
+	}
+	fsc.openedFiles.Insert(outWriter)
+	errWriter, err := stdioWriter(stderr, noopStderrStat)
+	if err != nil {
+		return nil, err
+	}
+	fsc.openedFiles.Insert(errWriter)
 
 	if _, ok := rootFS.(sysfs.UnimplementedFS); ok {
 		return fsc, nil
@@ -284,42 +296,40 @@ func NewFSContext(stdin io.Reader, stdout, stderr io.Writer, rootFS sysfs.FS) (f
 	return fsc, nil
 }
 
-func stdinReader(r io.Reader) *FileEntry {
+func stdinReader(r io.Reader) (*FileEntry, error) {
 	if r == nil {
 		r = eofReader{}
 	}
-	s := stdioStat(r, noopStdinStat)
-	return &FileEntry{Name: noopStdinStat.Name(), File: &stdioFileReader{r: r, s: s}}
+	s, err := stdioStat(r, noopStdinStat)
+	if err != nil {
+		return nil, err
+	}
+	return &FileEntry{Name: noopStdinStat.Name(), File: &stdioFileReader{r: r, s: s}}, nil
 }
 
-func stdioWriter(w io.Writer, defaultStat stdioFileInfo) *FileEntry {
+func stdioWriter(w io.Writer, defaultStat stdioFileInfo) (*FileEntry, error) {
 	if w == nil {
 		w = io.Discard
 	}
-	s := stdioStat(w, defaultStat)
-	return &FileEntry{Name: s.Name(), File: &stdioFileWriter{w: w, s: s}}
+	s, err := stdioStat(w, defaultStat)
+	if err != nil {
+		return nil, err
+	}
+	return &FileEntry{Name: s.Name(), File: &stdioFileWriter{w: w, s: s}}, nil
 }
 
-func stdioStat(f interface{}, defaultStat stdioFileInfo) fs.FileInfo {
-	if f, ok := f.(*os.File); ok && platform.IsTerminal(f.Fd()) {
-		return stdioFileInfo{defaultStat[0], modeCharDevice}
+func stdioStat(f interface{}, defaultStat stdioFileInfo) (fs.FileInfo, error) {
+	if f, ok := f.(*os.File); ok {
+		if st, err := f.Stat(); err == nil {
+			mode := uint32(st.Mode() & fs.ModeType)
+			return stdioFileInfo{defaultStat[0], mode}, nil
+		} else {
+			return nil, err
+		}
 	}
-	return defaultStat
+	return defaultStat, nil
 }
 
-// fileModeStat is a fake fs.FileInfo which only returns its mode.
-// This is used for character devices.
-type fileModeStat fs.FileMode
-
-var _ fs.FileInfo = fileModeStat(0)
-
-func (s fileModeStat) Size() int64        { return 0 }
-func (s fileModeStat) Mode() fs.FileMode  { return fs.FileMode(s) }
-func (s fileModeStat) ModTime() time.Time { return time.Unix(0, 0) }
-func (s fileModeStat) Sys() interface{}   { return nil }
-func (s fileModeStat) Name() string       { return "" }
-func (s fileModeStat) IsDir() bool        { return false }
-
 // RootFS returns the underlying filesystem. Any files that should be added to
 // the table should be inserted via InsertFile.
 func (c *FSContext) RootFS() sysfs.FS {


Test patch:
diff --git a/imports/wasi_snapshot_preview1/poll_test.go b/imports/wasi_snapshot_preview1/poll_test.go
index efef9c7d..69a36c61 100644
--- a/imports/wasi_snapshot_preview1/poll_test.go
+++ b/imports/wasi_snapshot_preview1/poll_test.go
@@ -1,11 +1,11 @@
 package wasi_snapshot_preview1_test
 
 import (
+	"io/fs"
 	"os"
 	"testing"
 
 	"github.com/tetratelabs/wazero"
-	"github.com/tetratelabs/wazero/internal/platform"
 	"github.com/tetratelabs/wazero/internal/sys"
 	"github.com/tetratelabs/wazero/internal/testing/require"
 	"github.com/tetratelabs/wazero/internal/wasip1"
@@ -66,8 +66,10 @@ func Test_pollOneoff_Errors(t *testing.T) {
 	// example, when run in a debugger, this could end up true.
 	// See also `terminal_test.go`.
 	expectedFdReadErr := wasip1.ErrnoNotsup
-	if platform.IsTerminal(os.Stdin.Fd()) {
-		expectedFdReadErr = wasip1.ErrnoBadf
+	if stat, err := os.Stdin.Stat(); err != nil {
+		if stat.Mode()&fs.ModeCharDevice != 0 {
+			expectedFdReadErr = wasip1.ErrnoBadf
+		}
 	}
 
 	tests := []struct {
diff --git a/imports/wasi_snapshot_preview1/wasi_stdlib_test.go b/imports/wasi_snapshot_preview1/wasi_stdlib_test.go
index e5fb23b4..6875d65c 100644
--- a/imports/wasi_snapshot_preview1/wasi_stdlib_test.go
+++ b/imports/wasi_snapshot_preview1/wasi_stdlib_test.go
@@ -12,7 +12,6 @@ import (
 
 	"github.com/tetratelabs/wazero"
 	"github.com/tetratelabs/wazero/imports/wasi_snapshot_preview1"
-	"github.com/tetratelabs/wazero/internal/platform"
 	"github.com/tetratelabs/wazero/internal/testing/require"
 	"github.com/tetratelabs/wazero/sys"
 )
@@ -221,13 +220,15 @@ func Test_Poll(t *testing.T) {
 	moduleConfig := wazero.NewModuleConfig().WithArgs("wasi", "poll")
 	console := compileAndRun(t, moduleConfig, wasmZigCc)
 	// The "real" expected behavior is to return "NOINPUT",
-	// however the poll API is currently relying on IsTerminal
-	// which makes the behavior platform-specific **during tests**
-	// and unfortunately hard to mock. For now, we just make sure
-	// the result is consistent.
-	if platform.IsTerminal(os.Stdin.Fd()) {
-		require.Equal(t, "NOINPUT\n", console)
-	} else {
-		require.Equal(t, "STDIN\n", console)
+	// however the poll API is currently relying on stat'ing the file
+	// descriptor for stdin which makes the behavior platform-specific
+	// **during tests** and unfortunately hard to mock.
+	// For now, we just make sure the result is consistent.
+	if stat, err := os.Stdin.Stat(); err != nil {
+		if stat.Mode()&fs.ModeCharDevice != 0 {
+			require.Equal(t, "NOINPUT\n", console)
+			return
+		}
 	}
+	require.Equal(t, "STDIN\n", console)
 }
diff --git a/internal/platform/bench_test.go b/internal/platform/bench_test.go
index 9a6b0122..fe04098b 100644
--- a/internal/platform/bench_test.go
+++ b/internal/platform/bench_test.go
@@ -7,12 +7,6 @@ import (
 	"testing"
 )
 
-func Benchmark_IsTerminal(b *testing.B) {
-	for i := 0; i < b.N; i++ {
-		IsTerminal(os.Stdout.Fd())
-	}
-}
-
 func Benchmark_UtimensFile(b *testing.B) {
 	tmpDir := b.TempDir()
 	f, err := os.Create(path.Join(tmpDir, "file"))
diff --git a/internal/platform/terminal_test.go b/internal/platform/terminal_test.go
deleted file mode 100644
index 11b9fbae..00000000
--- a/internal/platform/terminal_test.go
+++ /dev/null
@@ -1,65 +0,0 @@
-package platform
-
-import (
-	"os"
-	"path"
-	"testing"
-
-	"github.com/tetratelabs/wazero/internal/testing/require"
-)
-
-func Test_IsTerminal(t *testing.T) {
-	if !CompilerSupported() {
-		t.Skip() // because it will always return false
-	}
-
-	dir := t.TempDir()
-	require.NoError(t, os.WriteFile(path.Join(dir, "foo"), nil, 0o400))
-	file, err := os.Open(path.Join(dir, "foo"))
-	require.NoError(t, err)
-	defer file.Close()
-
-	// We aren't guaranteed to have a terminal device for os.Stdout, due to how
-	// `go test` forks processes. Instead, we test if this is consistent. For
-	// example, when run in a debugger, this could end up true.
-	stdioIsTTY := IsTerminal(os.Stdout.Fd())
-
-	tests := []struct {
-		name     string
-		file     *os.File
-		expected bool
-	}{
-		{name: "Stdin", file: os.Stdin, expected: stdioIsTTY},
-		{name: "Stdout", file: os.Stdout, expected: stdioIsTTY},
-		{name: "Stderr", file: os.Stderr, expected: stdioIsTTY},
-		{name: "File", file: file, expected: false},
-	}
-
-	for _, tt := range tests {
-		tc := tt
-		t.Run(tc.name, func(t *testing.T) {
-			require.Equal(t, tc.expected, IsTerminal(tc.file.Fd()))
-		})
-	}
-}
-
-func Test_IsTerminalFd(t *testing.T) {
-	stdioIsTTY := IsTerminal(os.Stdout.Fd())
-
-	tests := []struct {
-		name     string
-		fd       uintptr
-		expected bool
-	}{
-		{name: "Stdin", fd: 0, expected: stdioIsTTY},
-		{name: "Stdout", fd: 1, expected: stdioIsTTY},
-		{name: "Stderr", fd: 2, expected: stdioIsTTY},
-	}
-
-	for _, tt := range tests {
-		tc := tt
-		t.Run(tc.name, func(t *testing.T) {
-			require.Equal(t, tc.expected, IsTerminal(tc.fd))
-		})
-	}
-}
diff --git a/internal/platform/terminal_windows_test.go b/internal/platform/terminal_windows_test.go
deleted file mode 100644
index c8664d2c..00000000
--- a/internal/platform/terminal_windows_test.go
+++ /dev/null
@@ -1,14 +0,0 @@
-package platform
-
-import (
-	"syscall"
-	"testing"
-
-	"github.com/tetratelabs/wazero/internal/testing/require"
-)
-
-func Test_mapToWindowsHandle(t *testing.T) {
-	require.Equal(t, uintptr(syscall.Stdin), mapToWindowsHandle(0))
-	require.Equal(t, uintptr(syscall.Stdout), mapToWindowsHandle(1))
-	require.Equal(t, uintptr(syscall.Stderr), mapToWindowsHandle(2))
-}
diff --git a/internal/sys/fs_test.go b/internal/sys/fs_test.go
index 20f5d449..26c58ff8 100644
--- a/internal/sys/fs_test.go
+++ b/internal/sys/fs_test.go
@@ -6,6 +6,7 @@ import (
 	"errors"
 	"io"
 	"io/fs"
+	"log"
 	"os"
 	"path"
 	"syscall"
@@ -359,3 +360,56 @@ func TestWriterForFile(t *testing.T) {
 	require.Equal(t, noopStderr.File, WriterForFile(testFS, FdStderr))
 	require.Nil(t, WriterForFile(testFS, FdPreopen))
 }
+
+func TestStdioStat(t *testing.T) {
+	stat, err := stdioStat(os.Stdin, noopStdinStat)
+	require.NoError(t, err)
+	stdinStatMode := stat.Mode()
+	// ensure we are consistent with sys stdin
+	osStdinStat, _ := os.Stdin.Stat()
+	osStdinMode := osStdinStat.Mode().Type()
+	require.Equal(t, osStdinMode&fs.ModeDevice, stdinStatMode&fs.ModeDevice)
+	require.Equal(t, osStdinMode&fs.ModeCharDevice, stdinStatMode&fs.ModeCharDevice)
+
+	stat, err = stdioStat(os.Stdout, noopStdoutStat)
+	stdoutStatMode := stat.Mode()
+	require.NoError(t, err)
+	// ensure we are consistent with sys stdout
+	osStdoutStat, _ := os.Stdout.Stat()
+	osStdoutMode := osStdoutStat.Mode().Type()
+	require.Equal(t, osStdoutMode&fs.ModeDevice, stdoutStatMode&fs.ModeDevice)
+	require.Equal(t, osStdoutMode&fs.ModeCharDevice, stdoutStatMode&fs.ModeCharDevice)
+
+	stat, err = stdioStat(os.Stderr, noopStderrStat)
+	require.NoError(t, err)
+	stderrStatMode := stat.Mode()
+	// ensure we are consistent with sys stderr
+	osStderrStat, _ := os.Stderr.Stat()
+	osStderrMode := osStderrStat.Mode().Type()
+	require.Equal(t, osStderrMode&fs.ModeDevice, stderrStatMode&fs.ModeDevice)
+	require.Equal(t, osStderrMode&fs.ModeCharDevice, stderrStatMode&fs.ModeCharDevice)
+
+	// simulate regular file attached to stdin
+	f, err := os.CreateTemp("", "somefile")
+	if err != nil {
+		log.Fatal(err)
+	}
+	defer os.Remove(f.Name()) // clean up
+	stat, err = stdioStat(f, noopStdinStat)
+	require.NoError(t, err)
+	fStat := stat.Mode()
+	osFStat, _ := f.Stat()
+	osFStatMode := osFStat.Mode()
+	require.Equal(t, osFStatMode&fs.ModeDevice, fStat&fs.ModeDevice)
+	require.Equal(t, osFStatMode&fs.ModeCharDevice, fStat&fs.ModeCharDevice)
+
+	// interface{} returns default
+	stat, err = stdioStat("whatevs", noopStdinStat)
+	require.NoError(t, err)
+	require.Equal(t, noopStdinStat, stat)
+
+	// nil *File returns err
+	var nilFile *os.File
+	_, err = stdioStat(nilFile, noopStdinStat)
+	require.Error(t, err)
+}