Repository: tetratelabs/wazero (go)

Issue:
WASI fd_pread incorrectly moves file offset
**Describe the bug**

The [`fd_pread`](https://github.com/WebAssembly/WASI/blob/snapshot-01/phases/snapshot/docs.md#-fd_preadfd-fd-iovs-iovec_array-offset-filesize---errno-size) WASI syscall incorrectly moves the file offset.

Implementation probably can't be shared with `fd_read`, and should use `io.ReaderAt` not `io.ReadSeeker`.
https://github.com/tetratelabs/wazero/blob/6b9d119a7b069561ccb7f536a6d0d97d3772f549/imports/wasi_snapshot_preview1/fs.go#L561-L624

**To Reproduce**

```sh
git clone https://github.com/ncruces/wazero-bugs.git
cd wazero-bugs

echo Expected output:
cc -o main main.c && ./main

echo Actual output:
go run ./cmd
```

**Expected behavior**

```
Expected output:
Line 1
Line 2
Line 1
Line 3
Actual output:
Line 1
Line 2
Line 1
Line 2
```

Note the difference in the last line.

**Environment (please complete the relevant information):**
- Go version: 1.19.4
- wazero Version: v1.0.0-pre.5
- Host architecture: amd64
- Runtime mode: compiler

**Additional context**

[`main.c`](https://github.com/ncruces/wazero-bugs/blob/main/main.c) and [`main.go`](https://github.com/ncruces/wazero-bugs/blob/main/cmd/main.go) should be pretty self explanatory.
`main.wasm` from the repo was compiled with `zig cc --target=wasm32-wasi` version `0.10.0`.

I tried running it with `wasmtime --dir . main.wasm` which produces the correct output.

Pull request:
Fix fdPread: shouldn't move file offset
Fixes #958.

This is breaking a zig test, I still haven't been able to figure out why.

Patch:
diff --git a/RATIONALE.md b/RATIONALE.md
index f2e93d05..6245b90e 100644
--- a/RATIONALE.md
+++ b/RATIONALE.md
@@ -592,6 +592,19 @@ operation will err if it needs to. This helps reduce the complexity of the code
 in wazero and also accommodates the scenario where the bytes read are enough to
 satisfy its processor.
 
+### fd_pread: io.Seeker fallback when io.ReaderAt is not supported
+
+`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`.
+
+The initial implementation of `fd_pread` instead used `Seek`. To avoid a
+regression we fallback 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
+fails, the file offset is left in an undefined state. This is not thread-safe.
+
 ### Pre-opened files
 
 WASI includes `fd_prestat_get` and `fd_prestat_dir_name` functions used to
diff --git a/imports/wasi_snapshot_preview1/fs.go b/imports/wasi_snapshot_preview1/fs.go
index 41f13923..e6abd141 100644
--- a/imports/wasi_snapshot_preview1/fs.go
+++ b/imports/wasi_snapshot_preview1/fs.go
@@ -523,10 +523,28 @@ func fdReadOrPread(mod api.Module, params []uint64, isPread bool) Errno {
 		return ErrnoBadf
 	}
 
+	read := r.Read
 	if isPread {
-		if s, ok := r.(io.Seeker); ok {
-			if _, err := s.Seek(offset, io.SeekStart); err != nil {
-				return ErrnoFault
+		if ra, ok := r.(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.(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
@@ -549,7 +567,7 @@ func fdReadOrPread(mod api.Module, params []uint64, isPread bool) Errno {
 			return ErrnoFault
 		}
 
-		n, err := r.Read(b)
+		n, err := read(b)
 		nread += uint32(n)
 
 		shouldContinue, errno := fdRead_shouldContinueRead(uint32(n), l, err)


Test patch:
diff --git a/imports/wasi_snapshot_preview1/fs_test.go b/imports/wasi_snapshot_preview1/fs_test.go
index f96e5401..7b769861 100644
--- a/imports/wasi_snapshot_preview1/fs_test.go
+++ b/imports/wasi_snapshot_preview1/fs_test.go
@@ -528,6 +528,68 @@ func Test_fdPread(t *testing.T) {
 	}
 }
 
+func Test_fdPread_offset(t *testing.T) {
+	mod, fd, log, r := requireOpenFile(t, "/test_path", []byte("wazero"))
+	defer r.Close(testCtx)
+
+	// Do an initial fdPread.
+
+	iovs := uint32(1) // arbitrary offset
+	initialMemory := []byte{
+		'?',         // `iovs` is after this
+		18, 0, 0, 0, // = iovs[0].offset
+		4, 0, 0, 0, // = iovs[0].length
+		23, 0, 0, 0, // = iovs[1].offset
+		2, 0, 0, 0, // = iovs[1].length
+		'?',
+	}
+	iovsCount := uint32(2)    // The count of iovs
+	resultNread := uint32(26) // arbitrary offset
+
+	expectedMemory := append(
+		initialMemory,
+		'z', 'e', 'r', 'o', // iovs[0].length bytes
+		'?', '?', '?', '?', // resultNread is after this
+		4, 0, 0, 0, // sum(iovs[...].length) == length of "zero"
+		'?',
+	)
+
+	maskMemory(t, mod, len(expectedMemory))
+
+	ok := mod.Memory().Write(0, initialMemory)
+	require.True(t, ok)
+
+	requireErrno(t, ErrnoSuccess, mod, FdPreadName, uint64(fd), uint64(iovs), uint64(iovsCount), 2, uint64(resultNread))
+	actual, ok := mod.Memory().Read(0, uint32(len(expectedMemory)))
+	require.True(t, ok)
+	require.Equal(t, expectedMemory, actual)
+
+	// Verify that the fdPread didn't affect the fdRead offset.
+
+	expectedMemory = append(
+		initialMemory,
+		'w', 'a', 'z', 'e', // iovs[0].length bytes
+		'?',      // iovs[1].offset is after this
+		'r', 'o', // iovs[1].length bytes
+		'?',        // resultNread is after this
+		6, 0, 0, 0, // sum(iovs[...].length) == length of "wazero"
+		'?',
+	)
+
+	requireErrno(t, ErrnoSuccess, mod, FdReadName, uint64(fd), uint64(iovs), uint64(iovsCount), uint64(resultNread))
+	actual, ok = mod.Memory().Read(0, uint32(len(expectedMemory)))
+	require.True(t, ok)
+	require.Equal(t, expectedMemory, actual)
+
+	expectedLog := `
+==> wasi_snapshot_preview1.fd_pread(fd=4,iovs=1,iovs_len=2,offset=2)
+<== (nread=4,errno=ESUCCESS)
+==> wasi_snapshot_preview1.fd_read(fd=4,iovs=1,iovs_len=2)
+<== (nread=6,errno=ESUCCESS)
+`
+	require.Equal(t, expectedLog, "\n"+log.String())
+}
+
 func Test_fdPread_Errors(t *testing.T) {
 	contents := []byte("wazero")
 	mod, fd, log, r := requireOpenFile(t, "/test_path", contents)