fs: extracts syscallfs.ReaderAtOffset for WASI and gojs (#1037)

This extracts a utility `syscallfs.ReaderAtOffset()` to allow WASI and
gojs to re-use the same logic to implement `syscall.Pread`.

What's different than before is that if WASI passes multiple iovecs an
emulated `ReaderAt` will seek to the read position on each call to
`Read` vs once per loop. This was a design decision to keep the call
sites compatible between files that implement ReaderAt and those that
emulate them with Seeker (e.g. avoid the need for a read-scoped closer/
defer function). The main use case for emulation is `embed.file`, whose
seek function is cheap, so there's little performance impact to this.

Signed-off-by: Adrian Cole <adrian@tetrate.io>

RATIONALE.md

@@ -610,15 +610,19 @@ there are no strong incentives to complicate the logic.
 
 `ReadAt` is the Go equivalent to `pread`: it does not affect, and is not
 affected by, the underlying file offset. Unfortunately, `io.ReaderAt` is not
-implemented by all `fs.File`, notably `embed.file`.
+implemented by all `fs.File`. For example, as of Go 1.19, `embed.openFile` does
+not.
 
 The initial implementation of `fd_pread` instead used `Seek`. To avoid a
-regression we fallback to `io.Seeker` when `io.ReaderAt` is not supported.
+regression, we fall back to `io.Seeker` when `io.ReaderAt` is not supported.
 
 This requires obtaining the initial file offset, seeking to the intended read
-offset, and reseting the file offset the initial state. If this final seek
+offset, and resetting the file offset the initial state. If this final seek
 fails, the file offset is left in an undefined state. This is not thread-safe.
 
+While seeking per read seems expensive, the common case of `embed.openFile` is
+only accessing a single int64 field, which is cheap.
+
 ### Pre-opened files
 
 WASI includes `fd_prestat_get` and `fd_prestat_dir_name` functions used to

imports/wasi_snapshot_preview1/fs.go

@@ -502,51 +502,26 @@ func fdReadOrPread(mod api.Module, params []uint64, isPread bool) Errno {
 	fsc := mod.(*wasm.CallContext).Sys.FS()
 
 	fd := uint32(params[0])
+
+	r, ok := fsc.LookupFile(fd)
+	if !ok {
+		return ErrnoBadf
+	}
+
+	var reader io.Reader = r.File
+
 	iovs := uint32(params[1])
 	iovsCount := uint32(params[2])
 
-	var offset int64
 	var resultNread uint32
 	if isPread {
-		offset = int64(params[3])
+		offset := int64(params[3])
+		reader = syscallfs.ReaderAtOffset(r.File, offset)
 		resultNread = uint32(params[4])
 	} else {
 		resultNread = uint32(params[3])
 	}
 
-	r, ok := fsc.LookupFile(fd)
-	if !ok {
-		return ErrnoBadf
-	}
-
-	read := r.File.Read
-	if isPread {
-		if ra, ok := r.File.(io.ReaderAt); ok {
-			// ReadAt is the Go equivalent to pread.
-			read = func(p []byte) (int, error) {
-				n, err := ra.ReadAt(p, offset)
-				offset += int64(n)
-				return n, err
-			}
-		} else if s, ok := r.File.(io.Seeker); ok {
-			// Unfortunately, it is often not supported.
-			// See /RATIONALE.md "fd_pread: io.Seeker fallback when io.ReaderAt is not supported"
-			initialOffset, err := s.Seek(0, io.SeekCurrent)
-			if err != nil {
-				return ErrnoInval
-			}
-			defer func() { _, _ = s.Seek(initialOffset, io.SeekStart) }()
-			if offset != initialOffset {
-				_, err := s.Seek(offset, io.SeekStart)
-				if err != nil {
-					return ErrnoInval
-				}
-			}
-		} else {
-			return ErrnoInval
-		}
-	}
-
 	var nread uint32
 	iovsStop := iovsCount << 3 // iovsCount * 8
 	iovsBuf, ok := mem.Read(iovs, iovsStop)
@@ -563,7 +538,7 @@ func fdReadOrPread(mod api.Module, params []uint64, isPread bool) Errno {
 			return ErrnoFault
 		}
 
-		n, err := read(b)
+		n, err := reader.Read(b)
 		nread += uint32(n)
 
 		shouldContinue, errno := fdRead_shouldContinueRead(uint32(n), l, err)

imports/wasi_snapshot_preview1/fs_test.go

@@ -604,18 +604,8 @@ func Test_fdPread_Errors(t *testing.T) {
 			memory:        []byte{'?', '?', '?', '?'}, // pass result.nread validation
 			expectedErrno: ErrnoBadf,
 			expectedLog: `
-==> wasi_snapshot_preview1.fd_pread(fd=42,iovs=65532,iovs_len=65532,offset=0)
+==> wasi_snapshot_preview1.fd_pread(fd=42,iovs=65532,iovs_len=0,offset=0)
 <== (nread=,errno=EBADF)
-`,
-		},
-		{
-			name:          "seek past file",
-			fd:            fd,
-			offset:        int64(len(contents) + 1),
-			expectedErrno: ErrnoFault,
-			expectedLog: `
-==> wasi_snapshot_preview1.fd_pread(fd=4,iovs=65536,iovs_len=65536,offset=7)
-<== (nread=,errno=EFAULT)
 `,
 		},
 		{
@@ -625,7 +615,7 @@ func Test_fdPread_Errors(t *testing.T) {
 			memory:        []byte{'?'},
 			expectedErrno: ErrnoFault,
 			expectedLog: `
-==> wasi_snapshot_preview1.fd_pread(fd=4,iovs=65536,iovs_len=65535,offset=0)
+==> wasi_snapshot_preview1.fd_pread(fd=4,iovs=65536,iovs_len=0,offset=0)
 <== (nread=,errno=EFAULT)
 `,
 		},
@@ -639,7 +629,7 @@ func Test_fdPread_Errors(t *testing.T) {
 			},
 			expectedErrno: ErrnoFault,
 			expectedLog: `
-==> wasi_snapshot_preview1.fd_pread(fd=4,iovs=65532,iovs_len=65532,offset=0)
+==> wasi_snapshot_preview1.fd_pread(fd=4,iovs=65532,iovs_len=1,offset=0)
 <== (nread=,errno=EFAULT)
 `,
 		},
@@ -654,7 +644,7 @@ func Test_fdPread_Errors(t *testing.T) {
 			},
 			expectedErrno: ErrnoFault,
 			expectedLog: `
-==> wasi_snapshot_preview1.fd_pread(fd=4,iovs=65528,iovs_len=65528,offset=0)
+==> wasi_snapshot_preview1.fd_pread(fd=4,iovs=65528,iovs_len=1,offset=0)
 <== (nread=,errno=EFAULT)
 `,
 		},
@@ -670,7 +660,7 @@ func Test_fdPread_Errors(t *testing.T) {
 			},
 			expectedErrno: ErrnoFault,
 			expectedLog: `
-==> wasi_snapshot_preview1.fd_pread(fd=4,iovs=65527,iovs_len=65527,offset=0)
+==> wasi_snapshot_preview1.fd_pread(fd=4,iovs=65527,iovs_len=1,offset=0)
 <== (nread=,errno=EFAULT)
 `,
 		},
@@ -687,8 +677,27 @@ func Test_fdPread_Errors(t *testing.T) {
 			},
 			expectedErrno: ErrnoFault,
 			expectedLog: `
-==> wasi_snapshot_preview1.fd_pread(fd=4,iovs=65527,iovs_len=65527,offset=0)
+==> wasi_snapshot_preview1.fd_pread(fd=4,iovs=65527,iovs_len=1,offset=0)
 <== (nread=,errno=EFAULT)
+`,
+		},
+		{
+			name: "offset negative",
+			fd:   fd,
+			iovs: 1, iovsCount: 1,
+			resultNread: 10,
+			memory: []byte{
+				'?',        // `iovs` is after this
+				9, 0, 0, 0, // = iovs[0].offset
+				1, 0, 0, 0, // = iovs[0].length
+				'?',
+				'?', '?', '?', '?',
+			},
+			offset:        int64(-1),
+			expectedErrno: ErrnoIo,
+			expectedLog: `
+==> wasi_snapshot_preview1.fd_pread(fd=4,iovs=65523,iovs_len=1,offset=-1)
+<== (nread=,errno=EIO)
 `,
 		},
 	}
@@ -703,7 +712,7 @@ func Test_fdPread_Errors(t *testing.T) {
 			memoryWriteOK := mod.Memory().Write(offset, tc.memory)
 			require.True(t, memoryWriteOK)
 
-			requireErrno(t, tc.expectedErrno, mod, FdPreadName, uint64(tc.fd), uint64(tc.iovs+offset), uint64(tc.iovsCount+offset), uint64(tc.offset), uint64(tc.resultNread+offset))
+			requireErrno(t, tc.expectedErrno, mod, FdPreadName, uint64(tc.fd), uint64(tc.iovs+offset), uint64(tc.iovsCount), uint64(tc.offset), uint64(tc.resultNread+offset))
 			require.Equal(t, tc.expectedLog, "\n"+log.String())
 		})
 	}

internal/gojs/fs.go

@@ -274,17 +274,13 @@ func syscallRead(mod api.Module, fd uint32, offset interface{}, p []byte) (n uin
 		err = syscall.EBADF
 	}
 
+	var reader io.Reader = f.File
+
 	if offset != nil {
-		if s, ok := f.File.(io.Seeker); ok {
-			if _, err := s.Seek(toInt64(offset), io.SeekStart); err != nil {
-				return 0, err
-			}
-		} else {
-			return 0, syscall.ENOTSUP
-		}
+		reader = syscallfs.ReaderAtOffset(f.File, toInt64(offset))
 	}
 
-	if nRead, e := f.File.Read(p); e == nil || e == io.EOF {
+	if nRead, e := reader.Read(p); e == nil || e == io.EOF {
 		// fs_js.go cannot parse io.EOF so coerce it to nil.
 		// See https://github.com/golang/go/issues/43913
 		n = uint32(nRead)
@@ -309,7 +305,7 @@ func (jsfsWrite) invoke(ctx context.Context, mod api.Module, args ...interface{}
 	}
 	offset := goos.ValueToUint32(args[2])
 	byteCount := goos.ValueToUint32(args[3])
-	fOffset := args[4] // nil unless Pread
+	fOffset := args[4] // nil unless Pwrite
 	callback := args[5].(funcWrapper)
 
 	if byteCount > 0 { // empty is possible on EOF

internal/gojs/testdata/fs/main.go

@@ -1,7 +1,9 @@
 package fs
 
 import (
+	"bytes"
 	"fmt"
+	"io"
 	"log"
 	"os"
 	"syscall"
@@ -34,12 +36,35 @@ func testAdHoc() {
 		}
 	}
 
+	// Read the full contents of the file using io.Reader
 	b, err := os.ReadFile("/animals.txt")
 	if err != nil {
 		log.Panicln(err)
 	}
 	fmt.Println("contents:", string(b))
 
+	// Re-open the same file to test io.ReaderAt
+	f, err := os.Open("/animals.txt")
+	if err != nil {
+		log.Panicln(err)
+	}
+	defer f.Close()
+
+	// Seek to an arbitrary position.
+	if _, err = f.Seek(4, io.SeekStart); err != nil {
+		log.Panicln(err)
+	}
+
+	b1 := make([]byte, len(b))
+	// We expect to revert to the original position on ReadAt zero.
+	if _, err = f.ReadAt(b1, 0); err != nil {
+		log.Panicln(err)
+	}
+
+	if !bytes.Equal(b, b1) {
+		log.Panicln("unexpected ReadAt contents: ", string(b1))
+	}
+
 	b, err = os.ReadFile("/empty.txt")
 	if err != nil {
 		log.Panicln(err)

internal/syscallfs/adapter_test.go

@@ -125,16 +125,36 @@ func TestAdapt_TestFS(t *testing.T) {
 
 	// Make a new fs.FS, noting the Go 1.17 fstest doesn't automatically filter
 	// "." entries in a directory. TODO: remove when we remove 1.17
-	fsFS := make(gofstest.MapFS, len(fstest.FS)-1)
+	mapFS := make(gofstest.MapFS, len(fstest.FS)-1)
 	for k, v := range fstest.FS {
 		if k != "." {
-			fsFS[k] = v
+			mapFS[k] = v
 		}
 	}
 
-	// Adapt a normal fs.FS to syscallfs.FS
-	testFS := Adapt("/", fsFS)
+	tmpDir := t.TempDir()
+	require.NoError(t, fstest.WriteTestFiles(tmpDir))
+	dirFS := os.DirFS(tmpDir)
+
+	// TODO: We can't currently test embed.FS here because the source of
+	// fstest.FS are not real files.
+	tests := []struct {
+		name string
+		fs   fs.FS
+	}{
+		{name: "os.DirFS", fs: dirFS},
+		{name: "fstest.MapFS", fs: mapFS},
+	}
+
+	for _, tc := range tests {
+		tc := tc
 
-	// Adapt it back to fs.FS and run the tests
-	require.NoError(t, fstest.TestFS(&testFSAdapter{testFS}))
+		t.Run(tc.name, func(t *testing.T) {
+			// Adapt a normal fs.FS to syscallfs.FS
+			testFS := Adapt("/", tc.fs)
+
+			// Adapt it back to fs.FS and run the tests
+			require.NoError(t, fstest.TestFS(&testFSAdapter{testFS}))
+		})
+	}
 }