From ab0cb6f27349c2f7d54eb6cdb99a50d1704abe7f Mon Sep 17 00:00:00 2001 From: Nuno Cruces Date: Fri, 12 Aug 2022 09:04:44 +0100 Subject: [PATCH] Fix fdRead: handle io.Reader corner cases (#740) Signed-off-by: Nuno Cruces Signed-off-by: Adrian Cole Co-authored-by: Adrian Cole --- RATIONALE.md | 30 +++++++++++ wasi_snapshot_preview1/fs.go | 37 +++++++++++--- wasi_snapshot_preview1/fs_test.go | 85 +++++++++++++++++++++++++++++++ 3 files changed, 144 insertions(+), 8 deletions(-) diff --git a/RATIONALE.md b/RATIONALE.md index fd3852dd..1c2ee273 100644 --- a/RATIONALE.md +++ b/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. diff --git a/wasi_snapshot_preview1/fs.go b/wasi_snapshot_preview1/fs.go index 5a1866df..b05dabe9 100644 --- a/wasi_snapshot_preview1/fs.go +++ b/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. // diff --git a/wasi_snapshot_preview1/fs_test.go b/wasi_snapshot_preview1/fs_test.go index fa15b9a4..a98c7878 100644 --- a/wasi_snapshot_preview1/fs_test.go +++ b/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)