Repository: tetratelabs/wazero
Pull request #1459 (2023-05-12 06:12:19)

memory: avoid integer overflow


Patch:
diff --git a/imports/wasi_snapshot_preview1/fs.go b/imports/wasi_snapshot_preview1/fs.go
index ba7f9521..44543942 100644
--- a/imports/wasi_snapshot_preview1/fs.go
+++ b/imports/wasi_snapshot_preview1/fs.go
@@ -205,7 +205,7 @@ func fdFdstatGetFn(_ context.Context, mod api.Module, params []uint64) syscall.E
 		if f.File.IsNonblock() {
 			fdflags |= wasip1.FD_NONBLOCK
 		}
-	} else if f.File.AccessMode() != syscall.O_RDONLY {
+	} else if f.IsAppend() {
 		fdflags |= wasip1.FD_APPEND
 	}
 
diff --git a/internal/sys/fs.go b/internal/sys/fs.go
index 4f7c0c5c..d9b0d66e 100644
--- a/internal/sys/fs.go
+++ b/internal/sys/fs.go
@@ -255,6 +255,11 @@ type cachedStat struct {
 	Ino uint64
 }
 
+// IsAppend returns true if the file is open with syscall.O_APPEND.
+func (f *FileEntry) IsAppend() bool {
+	return f.openFlag&syscall.O_APPEND != 0
+}
+
 // Inode returns the cached inode from of platform.Stat_t or an error if it
 // couldn't be retrieved.
 func (f *FileEntry) Inode() (ino uint64, errno syscall.Errno) {
@@ -462,6 +467,8 @@ func (c *FSContext) ChangeOpenFlag(fd int32, flag int) syscall.Errno {
 		return errno
 	} else if isDir {
 		return syscall.EISDIR
+	} else if flag&syscall.O_APPEND == f.openFlag&syscall.O_APPEND {
+		return 0 // don't re-open
 	}
 
 	if flag&syscall.O_APPEND != 0 {
@@ -470,6 +477,9 @@ func (c *FSContext) ChangeOpenFlag(fd int32, flag int) syscall.Errno {
 		f.openFlag &= ^syscall.O_APPEND
 	}
 
+	// Clear any create flag, as we are re-opening, not re-creating.
+	f.openFlag &= ^syscall.O_CREAT
+
 	// Changing the flag while opening is not really supported well in Go. Even when using
 	// syscall package, the feasibility of doing so really depends on the platform. For examples:
 	//
diff --git a/internal/wasm/memory.go b/internal/wasm/memory.go
index c882b0f0..e2d994f9 100644
--- a/internal/wasm/memory.go
+++ b/internal/wasm/memory.go
@@ -111,7 +111,7 @@ func (m *MemoryInstance) ReadFloat64Le(offset uint32) (float64, bool) {
 
 // Read implements the same method as documented on api.Memory.
 func (m *MemoryInstance) Read(offset, byteCount uint32) ([]byte, bool) {
-	if !m.hasSize(offset, byteCount) {
+	if !m.hasSize(offset, uint64(byteCount)) {
 		return nil, false
 	}
 	return m.Buffer[offset : offset+byteCount : offset+byteCount], true
@@ -157,7 +157,7 @@ func (m *MemoryInstance) WriteFloat64Le(offset uint32, v float64) bool {
 
 // Write implements the same method as documented on api.Memory.
 func (m *MemoryInstance) Write(offset uint32, val []byte) bool {
-	if !m.hasSize(offset, uint32(len(val))) {
+	if !m.hasSize(offset, uint64(len(val))) {
 		return false
 	}
 	copy(m.Buffer[offset:], val)
@@ -166,7 +166,7 @@ func (m *MemoryInstance) Write(offset uint32, val []byte) bool {
 
 // WriteString implements the same method as documented on api.Memory.
 func (m *MemoryInstance) WriteString(offset uint32, val string) bool {
-	if !m.hasSize(offset, uint32(len(val))) {
+	if !m.hasSize(offset, uint64(len(val))) {
 		return false
 	}
 	copy(m.Buffer[offset:], val)
@@ -243,8 +243,8 @@ func (m *MemoryInstance) size() uint32 {
 // hasSize returns true if Len is sufficient for byteCount at the given offset.
 //
 // Note: This is always fine, because memory can grow, but never shrink.
-func (m *MemoryInstance) hasSize(offset uint32, byteCount uint32) bool {
-	return uint64(offset)+uint64(byteCount) <= uint64(len(m.Buffer)) // uint64 prevents overflow on add
+func (m *MemoryInstance) hasSize(offset uint32, byteCount uint64) bool {
+	return uint64(offset)+byteCount <= uint64(len(m.Buffer)) // uint64 prevents overflow on add
 }
 
 // readUint32Le implements ReadUint32Le without using a context. This is extracted as both ints and floats are stored in


Test patch:
diff --git a/imports/wasi_snapshot_preview1/fs_test.go b/imports/wasi_snapshot_preview1/fs_test.go
index 29bacc0a..fd4d2b83 100644
--- a/imports/wasi_snapshot_preview1/fs_test.go
+++ b/imports/wasi_snapshot_preview1/fs_test.go
@@ -250,7 +250,8 @@ func Test_fdFdstatGet(t *testing.T) {
 
 	stdin.File = stdinFile
 
-	fileFD, errno := fsc.OpenFile(preopen, file, os.O_RDONLY, 0)
+	// Make this file writeable, to ensure flags read-back correctly.
+	fileFD, errno := fsc.OpenFile(preopen, file, os.O_RDWR|os.O_APPEND, 0)
 	require.EqualErrno(t, 0, errno)
 
 	dirFD, errno := fsc.OpenFile(preopen, dir, os.O_RDONLY, 0)
@@ -325,13 +326,13 @@ func Test_fdFdstatGet(t *testing.T) {
 			fd:   fileFD,
 			expectedMemory: []byte{
 				4, 0, // fs_filetype
-				0, 0, 0, 0, 0, 0, // fs_flags
+				1, 0, 0, 0, 0, 0, // fs_flags
 				0xff, 0x1, 0xe0, 0x8, 0x0, 0x0, 0x0, 0x0, // fs_rights_base
 				0, 0, 0, 0, 0, 0, 0, 0, // fs_rights_inheriting
 			},
 			expectedLog: `
 ==> wasi_snapshot_preview1.fd_fdstat_get(fd=4)
-<== (stat={filetype=REGULAR_FILE,fdflags=,fs_rights_base=FD_DATASYNC|FD_READ|FD_SEEK|FDSTAT_SET_FLAGS|FD_SYNC|FD_TELL|FD_WRITE|FD_ADVISE|FD_ALLOCATE,fs_rights_inheriting=},errno=ESUCCESS)
+<== (stat={filetype=REGULAR_FILE,fdflags=APPEND,fs_rights_base=FD_DATASYNC|FD_READ|FD_SEEK|FDSTAT_SET_FLAGS|FD_SYNC|FD_TELL|FD_WRITE|FD_ADVISE|FD_ALLOCATE,fs_rights_inheriting=},errno=ESUCCESS)
 `,
 		},
 		{
@@ -481,11 +482,6 @@ func Test_fdFdstatGet_StdioNonblock(t *testing.T) {
 
 func Test_fdFdstatSetFlags(t *testing.T) {
 	tmpDir := t.TempDir() // open before loop to ensure no locking problems.
-	const fileName = "file.txt"
-
-	// Create the target file.
-	realPath := joinPath(tmpDir, fileName)
-	require.NoError(t, os.WriteFile(realPath, []byte("0123456789"), 0o600))
 
 	stdinR, stdinW := openPipe(t)
 	defer closePipe(stdinR, stdinW)
@@ -505,8 +501,18 @@ func Test_fdFdstatSetFlags(t *testing.T) {
 	preopen := fsc.RootFS()
 	defer r.Close(testCtx)
 
-	// First, open it with O_APPEND.
-	fd, errno := fsc.OpenFile(preopen, fileName, os.O_RDWR|os.O_APPEND, 0)
+	// First, O_CREATE the file with O_APPEND. We use O_EXCL because that
+	// triggers an EEXIST error if called a second time with O_CREATE. Our
+	// logic should clear O_CREATE preventing this.
+	const fileName = "file.txt"
+	// Create the target file.
+	fd, errno := fsc.OpenFile(preopen, fileName, os.O_RDWR|os.O_APPEND|os.O_CREATE|syscall.O_EXCL, 0o600)
+	require.EqualErrno(t, 0, errno)
+
+	// Write the initial text to the file.
+	f, ok := fsc.LookupFile(fd)
+	require.True(t, ok)
+	_, errno = f.File.Write([]byte("0123456789"))
 	require.EqualErrno(t, 0, errno)
 
 	writeWazero := func() {
diff --git a/internal/sys/fs_test.go b/internal/sys/fs_test.go
index 59839630..36dd7876 100644
--- a/internal/sys/fs_test.go
+++ b/internal/sys/fs_test.go
@@ -329,7 +329,7 @@ func TestFSContext_ChangeOpenFlag(t *testing.T) {
 	tmpDir := t.TempDir()
 	dirFs := sysfs.NewDirFS(tmpDir)
 
-	const fileName = "dir"
+	const fileName = "file"
 	require.NoError(t, os.WriteFile(path.Join(tmpDir, fileName), []byte("0123456789"), 0o600))
 
 	c := Context{}
@@ -360,6 +360,22 @@ func TestFSContext_ChangeOpenFlag(t *testing.T) {
 	f2, ok := fsc.openedFiles.Lookup(fd)
 	require.True(t, ok)
 	require.Equal(t, f2.openFlag&syscall.O_APPEND, 0)
+
+	t.Run("create exclusive", func(t *testing.T) {
+		// O_EXCL triggers an EEXIST error if called a second time with
+		// O_CREATE. This test proves the internal logic clears O_CREATE on
+		// re-open.
+		const fileName = "exclusive"
+		fd, errno := fsc.OpenFile(dirFs, fileName, os.O_RDWR|os.O_CREATE|syscall.O_EXCL, 0o600)
+		require.EqualErrno(t, 0, errno)
+
+		errno = fsc.ChangeOpenFlag(fd, syscall.O_APPEND)
+		require.EqualErrno(t, 0, errno)
+
+		f1, ok := fsc.openedFiles.Lookup(fd)
+		require.True(t, ok)
+		require.Equal(t, f1.openFlag&syscall.O_APPEND, syscall.O_APPEND)
+	})
 }
 
 func TestStdio(t *testing.T) {
diff --git a/internal/wasm/memory_test.go b/internal/wasm/memory_test.go
index 832b2e45..5957741d 100644
--- a/internal/wasm/memory_test.go
+++ b/internal/wasm/memory_test.go
@@ -2,8 +2,10 @@ package wasm
 
 import (
 	"math"
+	"reflect"
 	"strings"
 	"testing"
+	"unsafe"
 
 	"github.com/tetratelabs/wazero/api"
 	"github.com/tetratelabs/wazero/internal/testing/require"
@@ -194,7 +196,7 @@ func TestMemoryInstance_HasSize(t *testing.T) {
 		tc := tt
 
 		t.Run(tc.name, func(t *testing.T) {
-			require.Equal(t, tc.expected, memory.hasSize(tc.offset, uint32(tc.sizeInBytes)))
+			require.Equal(t, tc.expected, memory.hasSize(tc.offset, tc.sizeInBytes))
 		})
 	}
 }
@@ -735,6 +737,23 @@ func TestMemoryInstance_Write(t *testing.T) {
 	require.False(t, ok)
 }
 
+func TestMemoryInstance_Write_overflow(t *testing.T) {
+	mem := &MemoryInstance{Buffer: []byte{0, 0, 0, 0, 16, 0, 0, 0}, Min: 1}
+
+	// Test overflow
+	huge := uint64(math.MaxUint32 + 1 + 4)
+	if huge != uint64(int(huge)) {
+		t.Skip("Skipping on 32-bit")
+	}
+
+	buf := []byte{16, 0, 0, 4}
+	header := (*reflect.SliceHeader)(unsafe.Pointer(&buf))
+	header.Len = int(huge)
+	header.Cap = int(huge)
+
+	require.False(t, mem.Write(4, buf))
+}
+
 func TestMemoryInstance_WriteString(t *testing.T) {
 	mem := &MemoryInstance{Buffer: []byte{0, 0, 0, 0, 16, 0, 0, 0}, Min: 1}