Fix fdRead: handle io.Reader corner cases (#740)

Signed-off-by: Nuno Cruces <[email protected]>
Signed-off-by: Adrian Cole <[email protected]>
Co-authored-by: Adrian Cole <[email protected]>

Author: ncruces

RATIONALE.md

@@ -441,6 +441,36 @@ See
 * https://github.com/golang/go/blob/go1.19rc2/src/syscall/fs_js.go#L324
 * https://github.com/WebAssembly/wasi-libc/pull/214#issue-673090117
 
+### Why ignore the error returned by io.Reader when n > 1?
+
+Per https://pkg.go.dev/io#Reader, if we receive an error, any bytes read should
+be processed first. At the syscall abstraction (`fd_read`), the caller is the
+processor, so we can't process the bytes inline and also return the error (as
+`EIO`).
+
+Let's assume we want to return the bytes read on error to the caller. This
+implies we at least temporarily ignore the error alongside them. The choice
+remaining is whether to persist the error returned with the read until a
+possible next call, or ignore the error.
+
+If we persist an error returned, it would be coupled to a file descriptor, but
+effectively it is boolean as this case coerces to `EIO`. If we track a "last
+error" on a file descriptor, it could be complicated for a couple reasons
+including whether the error is transient or permanent, or if the error would
+apply to any FD operation, or just read. Finally, there may never be a
+subsequent read as perhaps the bytes leading up to the error are enough to
+satisfy the processor.
+
+This decision boils down to whether or not to track an error bit per file
+descriptor or not. If not, the assumption is that a subsequent operation would
+also error, this time without reading any bytes.
+
+The current opinion is to go with the simplest path, which is to return the
+bytes read and ignore the error the there were any. Assume a subsequent
+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.
+
 ### FdPrestatDirName
 
 `FdPrestatDirName` is a WASI function to return the path of the pre-opened directory of a file descriptor.

wasi_snapshot_preview1/fs.go

@@ -386,6 +386,7 @@ var fdRead = wasm.NewGoFunc(
 	[]string{"fd", "iovs", "iovs_len", "result.size"},
 	func(ctx context.Context, mod api.Module, fd, iovs, iovsCount, resultSize uint32) Errno {
 		sysCtx := mod.(*wasm.CallContext).Sys
+		mem := mod.Memory()
 		reader := internalsys.FdReader(ctx, sysCtx, fd)
 		if reader == nil {
 			return ErrnoBadf
@@ -394,33 +395,53 @@ var fdRead = wasm.NewGoFunc(
 		var nread uint32
 		for i := uint32(0); i < iovsCount; i++ {
 			iovPtr := iovs + i*8
-			offset, ok := mod.Memory().ReadUint32Le(ctx, iovPtr)
+			offset, ok := mem.ReadUint32Le(ctx, iovPtr)
 			if !ok {
 				return ErrnoFault
 			}
-			l, ok := mod.Memory().ReadUint32Le(ctx, iovPtr+4)
+			l, ok := mem.ReadUint32Le(ctx, iovPtr+4)
 			if !ok {
 				return ErrnoFault
 			}
-			b, ok := mod.Memory().Read(ctx, offset, l)
+			b, ok := mem.Read(ctx, offset, l)
 			if !ok {
 				return ErrnoFault
 			}
-			n, err := reader.Read(b) // Note: n <= l
+
+			n, err := reader.Read(b)
 			nread += uint32(n)
-			if errors.Is(err, io.EOF) {
+
+			shouldContinue, errno := fdRead_shouldContinueRead(uint32(n), l, err)
+			if errno != 0 {
+				return errno
+			} else if !shouldContinue {
 				break
-			} else if err != nil {
-				return ErrnoIo
 			}
 		}
-		if !mod.Memory().WriteUint32Le(ctx, resultSize, nread) {
+		if !mem.WriteUint32Le(ctx, resultSize, nread) {
 			return ErrnoFault
 		}
 		return ErrnoSuccess
 	},
 )
 
+// fdRead_shouldContinueRead decides whether to continue reading the next iovec
+// based on the amount read (n/l) and a possible error returned from io.Reader.
+//
+// Note: When there are both bytes read (n) and an error, this continues.
+// See /RATIONALE.md "Why ignore the error returned by io.Reader when n > 1?"
+func fdRead_shouldContinueRead(n, l uint32, err error) (bool, Errno) {
+	if errors.Is(err, io.EOF) {
+		return false, 0 // EOF isn't an error, and we shouldn't continue.
+	} else if err != nil && n == 0 {
+		return false, ErrnoIo
+	} else if err != nil {
+		return false, 0 // Allow the caller to process n bytes.
+	}
+	// Continue reading, unless there's a partial read or nothing to read.
+	return n == l && n != 0, 0
+}
+
 // fdReaddir is the WASI function named functionFdReaddir which reads directory
 // entries from a directory.
 //

wasi_snapshot_preview1/fs_test.go

@@ -615,6 +615,91 @@ func Test_fdRead_Errors(t *testing.T) {
 	}
 }
 
+func Test_fdRead_shouldContinueRead(t *testing.T) {
+	tests := []struct {
+		name          string
+		n, l          uint32
+		err           error
+		expectedOk    bool
+		expectedErrno Errno
+	}{
+		{
+			name: "break when nothing to read",
+			n:    0,
+			l:    0,
+		},
+		{
+			name: "break when nothing read",
+			n:    0,
+			l:    4,
+		},
+		{
+			name: "break on partial read",
+			n:    3,
+			l:    4,
+		},
+		{
+			name:       "continue on full read",
+			n:          4,
+			l:          4,
+			expectedOk: true,
+		},
+		{
+			name: "break on EOF on nothing to read",
+			err:  io.EOF,
+		},
+		{
+			name: "break on EOF on nothing read",
+			l:    4,
+			err:  io.EOF,
+		},
+		{
+			name: "break on EOF on partial read",
+			n:    3,
+			l:    4,
+			err:  io.EOF,
+		},
+		{
+			name: "break on EOF on full read",
+			n:    4,
+			l:    4,
+			err:  io.EOF,
+		},
+		{
+			name:          "return ErrnoIo on error on nothing to read",
+			err:           io.ErrClosedPipe,
+			expectedErrno: ErrnoIo,
+		},
+		{
+			name:          "return ErrnoIo on error on nothing read",
+			l:             4,
+			err:           io.ErrClosedPipe,
+			expectedErrno: ErrnoIo,
+		},
+		{ // Special case, allows processing data before err
+			name: "break on error on partial read",
+			n:    3,
+			l:    4,
+			err:  io.ErrClosedPipe,
+		},
+		{ // Special case, allows processing data before err
+			name: "break on error on full read",
+			n:    4,
+			l:    4,
+			err:  io.ErrClosedPipe,
+		},
+	}
+	for _, tt := range tests {
+		tc := tt
+
+		t.Run(tc.name, func(t *testing.T) {
+			ok, errno := fdRead_shouldContinueRead(tc.n, tc.l, tc.err)
+			require.Equal(t, tc.expectedOk, ok)
+			require.Equal(t, tc.expectedErrno, errno)
+		})
+	}
+}
+
 // Test_fdReaddir only tests it is stubbed for GrainLang per #271
 func Test_fdReaddir(t *testing.T) {
 	log := requireErrnoNosys(t, functionFdReaddir, 0, 0, 0, 0, 0)