Repository: tetratelabs/wazero
Pull request #911 (2022-12-10 01:07:33)

dwarf: ensure Goroutine-safety in DWARFLines.Line function
Signed-off-by: Takeshi Yoneda <takeshi@tetrate.io>


Patch:
diff --git a/imports/wasi_snapshot_preview1/fs.go b/imports/wasi_snapshot_preview1/fs.go
index 7c46e829..79a31180 100644
--- a/imports/wasi_snapshot_preview1/fs.go
+++ b/imports/wasi_snapshot_preview1/fs.go
@@ -611,8 +611,8 @@ func fdReadFn(ctx context.Context, mod api.Module, stack []uint64) (nread uint32
 }
 
 func fdReadOrPread(ctx context.Context, mod api.Module, stack []uint64, isPread bool) (uint32, Errno) {
-	sysCtx := mod.(*wasm.CallContext).Sys
 	mem := mod.Memory()
+	sysCtx := mod.(*wasm.CallContext).Sys
 
 	fd := uint32(stack[0])
 	iovs := uint32(stack[1])
@@ -698,6 +698,9 @@ var fdReaddir = proxyResultParams(&wasm.HostFunc{
 }, fdReaddirName)
 
 func fdReaddirFn(ctx context.Context, mod api.Module, stack []uint64) (uint32, Errno) {
+	mem := mod.Memory()
+	fsc := mod.(*wasm.CallContext).Sys.FS(ctx)
+
 	fd := uint32(stack[0])
 	buf := uint32(stack[1])
 	bufLen := uint32(stack[2])
@@ -706,15 +709,21 @@ func fdReaddirFn(ctx context.Context, mod api.Module, stack []uint64) (uint32, E
 	// it in such a way that becomes negative.
 	cookie := int64(stack[3])
 
+	// The bufLen must be enough to write a dirent. Otherwise, the caller can't
+	// read what the next cookie is.
+	if bufLen < direntSize {
+		return 0, ErrnoInval
+	}
+
 	// Validate the FD is a directory
-	rd, dir, errno := openedDir(ctx, mod, fd)
+	rd, dir, errno := openedDir(ctx, fsc, fd)
 	if errno != ErrnoSuccess {
 		return 0, errno
 	}
 
 	// expect a cookie only if we are continuing a read.
 	if cookie == 0 && dir.CountRead > 0 {
-		return 0, ErrnoInval // invalid as a cookie is minimally one.
+		return 0, ErrnoInval // cookie is minimally one.
 	}
 
 	// First, determine the maximum directory entries that can be encoded as
@@ -750,8 +759,6 @@ func fdReaddirFn(ctx context.Context, mod api.Module, stack []uint64) (uint32, E
 		}
 	}
 
-	mem := mod.Memory()
-
 	// Determine how many dirents we can write, excluding a potentially
 	// truncated entry.
 	bufused, direntCount, writeTruncatedEntry := maxDirents(entries, bufLen)
@@ -930,8 +937,7 @@ func writeDirent(buf []byte, dNext uint64, dNamlen uint32, dType bool) {
 }
 
 // openedDir returns the directory and ErrnoSuccess if the fd points to a readable directory.
-func openedDir(ctx context.Context, mod api.Module, fd uint32) (fs.ReadDirFile, *internalsys.ReadDir, Errno) {
-	fsc := mod.(*wasm.CallContext).Sys.FS(ctx)
+func openedDir(ctx context.Context, fsc *internalsys.FSContext, fd uint32) (fs.ReadDirFile, *internalsys.ReadDir, Errno) {
 	if f, ok := fsc.OpenedFile(ctx, fd); !ok {
 		return nil, nil, ErrnoBadf
 	} else if d, ok := f.File.(fs.ReadDirFile); !ok {
@@ -1103,8 +1109,8 @@ var fdWrite = proxyResultParams(&wasm.HostFunc{
 }, fdWriteName)
 
 func fdWriteFn(ctx context.Context, mod api.Module, stack []uint64) (uint32, Errno) {
-	sysCtx := mod.(*wasm.CallContext).Sys
 	mem := mod.Memory()
+	sysCtx := mod.(*wasm.CallContext).Sys
 
 	fd := uint32(stack[0])
 	iovs := uint32(stack[1])
diff --git a/internal/wasmdebug/dwarf.go b/internal/wasmdebug/dwarf.go
index c0dcd05c..6ada7e49 100644
--- a/internal/wasmdebug/dwarf.go
+++ b/internal/wasmdebug/dwarf.go
@@ -6,6 +6,7 @@ import (
 	"fmt"
 	"io"
 	"sort"
+	"sync"
 )
 
 // DWARFLines is used to retrieve source code line information from the DWARF data.
@@ -15,6 +16,7 @@ type DWARFLines struct {
 	// linesPerEntry maps dwarf.Offset for dwarf.Entry to the list of lines contained by the entry.
 	// The value is sorted in the increasing order by the address.
 	linesPerEntry map[dwarf.Offset][]line
+	mux           sync.Mutex
 }
 
 type line struct {
@@ -37,6 +39,12 @@ func (d *DWARFLines) Line(instructionOffset uint64) string {
 		return ""
 	}
 
+	// DWARFLines is created per Wasm binary, so there's a possibility that multiple instances
+	// created from a same binary face runtime error at the same time, and that results in
+	// concurrent access to this function.
+	d.mux.Lock()
+	defer d.mux.Unlock()
+
 	r := d.d.Reader()
 
 	// Get the dwarf.Entry containing the instruction.


Test patch:
diff --git a/imports/wasi_snapshot_preview1/fs_test.go b/imports/wasi_snapshot_preview1/fs_test.go
index 74afa788..b8281923 100644
--- a/imports/wasi_snapshot_preview1/fs_test.go
+++ b/imports/wasi_snapshot_preview1/fs_test.go
@@ -1248,7 +1248,7 @@ func Test_fdReaddir(t *testing.T) {
 	tests := []struct {
 		name            string
 		dir             func() *internalsys.FileEntry
-		buf, bufLen     uint32
+		bufLen          uint32
 		cookie          int64
 		expectedMem     []byte
 		expectedMemSize int
@@ -1263,7 +1263,7 @@ func Test_fdReaddir(t *testing.T) {
 
 				return &internalsys.FileEntry{File: dir}
 			},
-			buf: 0, bufLen: 1,
+			bufLen:          direntSize,
 			cookie:          0,
 			expectedBufused: 0,
 			expectedMem:     []byte{},
@@ -1277,7 +1277,7 @@ func Test_fdReaddir(t *testing.T) {
 
 				return &internalsys.FileEntry{File: dir}
 			},
-			buf: 0, bufLen: 4096,
+			bufLen:          4096,
 			cookie:          0,
 			expectedBufused: 78, // length of all entries
 			expectedMem:     append(append(dirent1, dirent2...), dirent3...),
@@ -1286,23 +1286,6 @@ func Test_fdReaddir(t *testing.T) {
 				Entries:   testDirEntries,
 			},
 		},
-		{
-			name: "can't read",
-			dir: func() *internalsys.FileEntry {
-				dir, err := fdReadDirFs.Open("dir")
-				require.NoError(t, err)
-
-				return &internalsys.FileEntry{File: dir}
-			},
-			buf: 0, bufLen: 23, // length is too short for header
-			cookie:          0,
-			expectedBufused: 23, // == bufLen which is the size of the dirent
-			expectedMem:     nil,
-			expectedReadDir: &internalsys.ReadDir{
-				CountRead: 2,
-				Entries:   testDirEntries[:2],
-			},
-		},
 		{
 			name: "can't read name",
 			dir: func() *internalsys.FileEntry {
@@ -1311,7 +1294,7 @@ func Test_fdReaddir(t *testing.T) {
 
 				return &internalsys.FileEntry{File: dir}
 			},
-			buf: 0, bufLen: 24, // length is long enough for first, but not the name.
+			bufLen:          24, // length is long enough for first, but not the name.
 			cookie:          0,
 			expectedBufused: 24,           // == bufLen which is the size of the dirent
 			expectedMem:     dirent1[:24], // header without name
@@ -1328,7 +1311,7 @@ func Test_fdReaddir(t *testing.T) {
 
 				return &internalsys.FileEntry{File: dir}
 			},
-			buf: 0, bufLen: 25, // length is long enough for first + the name, but not more.
+			bufLen:          25, // length is long enough for first + the name, but not more.
 			cookie:          0,
 			expectedBufused: 25, // length to read exactly first.
 			expectedMem:     dirent1,
@@ -1353,7 +1336,7 @@ func Test_fdReaddir(t *testing.T) {
 					},
 				}
 			},
-			buf: 0, bufLen: 26, // length is long enough for exactly second.
+			bufLen:          26, // length is long enough for exactly second.
 			cookie:          1,  // d_next of first
 			expectedBufused: 26, // length to read exactly second.
 			expectedMem:     dirent2,
@@ -1378,7 +1361,7 @@ func Test_fdReaddir(t *testing.T) {
 					},
 				}
 			},
-			buf: 0, bufLen: 30, // length is longer than the second entry, but not long enough for a header.
+			bufLen:          30, // length is longer than the second entry, but not long enough for a header.
 			cookie:          1,  // d_next of first
 			expectedBufused: 30, // length to read some more, but not enough for a header, so buf was exhausted.
 			expectedMem:     dirent2,
@@ -1404,7 +1387,7 @@ func Test_fdReaddir(t *testing.T) {
 					},
 				}
 			},
-			buf: 0, bufLen: 50, // length is longer than the second entry + enough for the header of third.
+			bufLen:          50, // length is longer than the second entry + enough for the header of third.
 			cookie:          1,  // d_next of first
 			expectedBufused: 50, // length to read exactly second and the header of third.
 			expectedMem:     append(dirent2, dirent3[0:24]...),
@@ -1429,7 +1412,7 @@ func Test_fdReaddir(t *testing.T) {
 					},
 				}
 			},
-			buf: 0, bufLen: 53, // length is long enough for second and third.
+			bufLen:          53, // length is long enough for second and third.
 			cookie:          1,  // d_next of first
 			expectedBufused: 53, // length to read exactly one second and third.
 			expectedMem:     append(dirent2, dirent3...),
@@ -1454,7 +1437,7 @@ func Test_fdReaddir(t *testing.T) {
 					},
 				}
 			},
-			buf: 0, bufLen: 27, // length is long enough for exactly third.
+			bufLen:          27, // length is long enough for exactly third.
 			cookie:          2,  // d_next of second.
 			expectedBufused: 27, // length to read exactly third.
 			expectedMem:     dirent3,
@@ -1479,9 +1462,9 @@ func Test_fdReaddir(t *testing.T) {
 					},
 				}
 			},
-			buf: 0, bufLen: 100, // length is long enough for third and more, but there is nothing more.
-			cookie:          2,  // d_next of second.
-			expectedBufused: 27, // length to read exactly third.
+			bufLen:          100, // length is long enough for third and more, but there is nothing more.
+			cookie:          2,   // d_next of second.
+			expectedBufused: 27,  // length to read exactly third.
 			expectedMem:     dirent3,
 			expectedReadDir: &internalsys.ReadDir{
 				CountRead: 3,
@@ -1506,17 +1489,17 @@ func Test_fdReaddir(t *testing.T) {
 
 			maskMemory(t, testCtx, mod, int(tc.bufLen))
 
-			// use an arbitrarily high value for the buf used position.
-			resultBufused := uint32(16192)
+			resultBufused := uint32(0) // where to write the amount used out of bufLen
+			buf := uint32(8)           // where to start the dirents
 			requireErrno(t, ErrnoSuccess, mod, fdReaddirName,
-				uint64(fd), uint64(tc.buf), uint64(tc.bufLen), uint64(tc.cookie), uint64(resultBufused))
+				uint64(fd), uint64(buf), uint64(tc.bufLen), uint64(tc.cookie), uint64(resultBufused))
 
 			// read back the bufused and compare memory against it
 			bufUsed, ok := mod.Memory().ReadUint32Le(testCtx, resultBufused)
 			require.True(t, ok)
 			require.Equal(t, tc.expectedBufused, bufUsed)
 
-			mem, ok := mod.Memory().Read(testCtx, tc.buf, bufUsed)
+			mem, ok := mod.Memory().Read(testCtx, buf, bufUsed)
 			require.True(t, ok)
 
 			if tc.expectedMem != nil {
@@ -1569,26 +1552,30 @@ func Test_fdReaddir_Errors(t *testing.T) {
 `,
 		},
 		{
-			name:          "invalid fd",
-			fd:            42, // arbitrary invalid fd
+			name: "invalid fd",
+			fd:   42,                    // arbitrary invalid fd
+			buf:  0, bufLen: direntSize, // enough to read the dirent
+			resultBufused: 1000, // arbitrary
 			expectedErrno: ErrnoBadf,
 			expectedLog: `
---> proxy.fd_readdir(fd=42,buf=0,buf_len=0,cookie=0,result.bufused=0)
-	--> wasi_snapshot_preview1.fd_readdir(fd=42,buf=0,buf_len=0,cookie=0,result.bufused=0)
-		==> wasi_snapshot_preview1.fdReaddir(fd=42,buf=0,buf_len=0,cookie=0)
+--> proxy.fd_readdir(fd=42,buf=0,buf_len=24,cookie=0,result.bufused=1000)
+	--> wasi_snapshot_preview1.fd_readdir(fd=42,buf=0,buf_len=24,cookie=0,result.bufused=1000)
+		==> wasi_snapshot_preview1.fdReaddir(fd=42,buf=0,buf_len=24,cookie=0)
 		<== (bufused=0,EBADF)
 	<-- EBADF
 <-- 8
 `,
 		},
 		{
-			name:          "not a dir",
-			fd:            fileFD,
+			name: "not a dir",
+			fd:   fileFD,
+			buf:  0, bufLen: direntSize, // enough to read the dirent
+			resultBufused: 1000, // arbitrary
 			expectedErrno: ErrnoNotdir,
 			expectedLog: `
---> proxy.fd_readdir(fd=5,buf=0,buf_len=0,cookie=0,result.bufused=0)
-	--> wasi_snapshot_preview1.fd_readdir(fd=5,buf=0,buf_len=0,cookie=0,result.bufused=0)
-		==> wasi_snapshot_preview1.fdReaddir(fd=5,buf=0,buf_len=0,cookie=0)
+--> proxy.fd_readdir(fd=5,buf=0,buf_len=24,cookie=0,result.bufused=1000)
+	--> wasi_snapshot_preview1.fd_readdir(fd=5,buf=0,buf_len=24,cookie=0,result.bufused=1000)
+		==> wasi_snapshot_preview1.fdReaddir(fd=5,buf=0,buf_len=24,cookie=0)
 		<== (bufused=0,ENOTDIR)
 	<-- ENOTDIR
 <-- 54
@@ -1625,16 +1612,34 @@ func Test_fdReaddir_Errors(t *testing.T) {
 `,
 		},
 		{
-			name: "resultBufused is outside memory",
+			name: "bufLen must be enough to write a struct",
 			fd:   dirFD,
 			buf:  0, bufLen: 1,
-			resultBufused: memLen,
-			expectedErrno: ErrnoFault,
+			resultBufused: 1000,
+			expectedErrno: ErrnoInval,
 			expectedLog: `
---> proxy.fd_readdir(fd=4,buf=0,buf_len=1,cookie=0,result.bufused=65536)
-	--> wasi_snapshot_preview1.fd_readdir(fd=4,buf=0,buf_len=1,cookie=0,result.bufused=65536)
-	<-- EFAULT
-<-- 21
+--> proxy.fd_readdir(fd=4,buf=0,buf_len=1,cookie=0,result.bufused=1000)
+	--> wasi_snapshot_preview1.fd_readdir(fd=4,buf=0,buf_len=1,cookie=0,result.bufused=1000)
+		==> wasi_snapshot_preview1.fdReaddir(fd=4,buf=0,buf_len=1,cookie=0)
+		<== (bufused=0,EINVAL)
+	<-- EINVAL
+<-- 28
+`,
+		},
+		{
+			name: "cookie invalid when no prior state",
+			fd:   dirFD,
+			buf:  0, bufLen: 1000,
+			cookie:        1,
+			resultBufused: 2000,
+			expectedErrno: ErrnoInval,
+			expectedLog: `
+--> proxy.fd_readdir(fd=4,buf=0,buf_len=1000,cookie=1,result.bufused=2000)
+	--> wasi_snapshot_preview1.fd_readdir(fd=4,buf=0,buf_len=1000,cookie=1,result.bufused=2000)
+		==> wasi_snapshot_preview1.fdReaddir(fd=4,buf=0,buf_len=1000,cookie=1)
+		<== (bufused=0,EINVAL)
+	<-- EINVAL
+<-- 28
 `,
 		},
 		{
diff --git a/imports/wasi_snapshot_preview1/wasi_bench_test.go b/imports/wasi_snapshot_preview1/wasi_bench_test.go
index da640e78..687eee73 100644
--- a/imports/wasi_snapshot_preview1/wasi_bench_test.go
+++ b/imports/wasi_snapshot_preview1/wasi_bench_test.go
@@ -1,12 +1,16 @@
 package wasi_snapshot_preview1
 
 import (
+	"embed"
+	"io/fs"
+	"os"
 	"testing"
 
 	"github.com/tetratelabs/wazero"
 	"github.com/tetratelabs/wazero/api"
 	"github.com/tetratelabs/wazero/internal/sys"
 	"github.com/tetratelabs/wazero/internal/testing/proxy"
+	"github.com/tetratelabs/wazero/internal/wasm"
 )
 
 // configArgsEnviron ensures the result data are the same between args and ENV.
@@ -40,10 +44,7 @@ func Benchmark_ArgsEnviron(b *testing.B) {
 				if err != nil {
 					b.Fatal(err)
 				}
-				errno := Errno(results[0])
-				if errno != 0 {
-					b.Fatal(ErrnoName(errno))
-				}
+				requireEsuccess(b, results)
 			}
 		})
 	}
@@ -110,15 +111,126 @@ func Benchmark_fdRead(b *testing.B) {
 				if err != nil {
 					b.Fatal(err)
 				}
-				errno := Errno(results[0])
-				if errno != 0 {
-					b.Fatal(ErrnoName(errno))
+				requireEsuccess(b, results)
+			}
+		})
+	}
+}
+
+//go:embed testdata
+var testdata embed.FS
+
+func Benchmark_fdReaddir(b *testing.B) {
+	embedFS, err := fs.Sub(testdata, "testdata")
+	if err != nil {
+		b.Fatal(err)
+	}
+
+	benches := []struct {
+		name string
+		fs   fs.FS
+		// continued is true to test performance on a follow-up call. The
+		// preceding will call fd_read with 24 bytes, which is enough to read
+		// the initial entry's size, but not enough to read its name. This
+		// ensures the next fd_read is allowed to pass a cookie, because it
+		// read fd_next, while ensuring it will write all the entries.
+		continued bool
+	}{
+		{
+			name: "embed.FS",
+			fs:   embedFS,
+		},
+		{
+			name:      "embed.FS - continued",
+			fs:        embedFS,
+			continued: true,
+		},
+		{
+			name: "os.DirFS",
+			fs:   os.DirFS("testdata"),
+		},
+		{
+			name:      "os.DirFS - continued",
+			fs:        os.DirFS("testdata"),
+			continued: true,
+		},
+	}
+
+	for _, bb := range benches {
+		bc := bb
+
+		b.Run(bc.name, func(b *testing.B) {
+			r := wazero.NewRuntime(testCtx)
+			defer r.Close(testCtx)
+
+			mod, err := instantiateProxyModule(r, wazero.NewModuleConfig().WithFS(bc.fs))
+			if err != nil {
+				b.Fatal(err)
+			}
+
+			fn := mod.ExportedFunction(fdReaddirName)
+
+			// Open the root directory as a file-descriptor.
+			fsc := mod.(*wasm.CallContext).Sys.FS(testCtx)
+			fd, err := fsc.OpenFile(testCtx, ".")
+			if err != nil {
+				b.Fatal(err)
+			}
+			f, ok := fsc.OpenedFile(testCtx, fd)
+			if !ok {
+				b.Fatal("couldn't open fd ", fd)
+			}
+			defer fsc.CloseFile(testCtx, fd)
+
+			b.ResetTimer()
+			b.ReportAllocs()
+			for i := 0; i < b.N; i++ {
+				b.StopTimer()
+
+				cookie := 0        // where to begin (last read d_next)
+				resultBufused := 0 // where to write the amount used out of bufLen
+				buf := 8           // where to start the dirents
+				bufLen := 8096     // allow up to 8KB buffer usage
+
+				// Recreate the file under the file-descriptor
+				if err = f.File.Close(); err != nil {
+					b.Fatal(err)
 				}
+				if f.File, err = bc.fs.Open("."); err != nil {
+					b.Fatal(err)
+				}
+				f.ReadDir = nil
+
+				// Make an initial call to build the state of an unread directory
+				if bc.continued {
+					results, err := fn.Call(testCtx, uint64(fd), uint64(buf), uint64(24), uint64(cookie), uint64(resultBufused))
+					if err != nil {
+						b.Fatal(err)
+					}
+					requireEsuccess(b, results)
+					cookie = 1 // WASI doesn't document this, but we write the first d_next as 1
+				}
+
+				// Time the call to write the dirents
+				b.StartTimer()
+				results, err := fn.Call(testCtx, uint64(fd), uint64(buf), uint64(bufLen), uint64(cookie), uint64(resultBufused))
+				if err != nil {
+					b.Fatal(err)
+				}
+				b.StopTimer()
+
+				requireEsuccess(b, results)
 			}
 		})
 	}
 }
 
+func requireEsuccess(b *testing.B, results []uint64) {
+	if errno := Errno(results[0]); errno != 0 {
+		b.Fatal(ErrnoName(errno))
+	}
+}
+
 type writerFunc func(p []byte) (n int, err error)
 
 // Write implements io.Writer by calling writerFunc.
@@ -179,10 +291,7 @@ func Benchmark_fdWrite(b *testing.B) {
 				if err != nil {
 					b.Fatal(err)
 				}
-				errno := Errno(results[0])
-				if errno != 0 {
-					b.Fatal(ErrnoName(errno))
-				}
+				requireEsuccess(b, results)
 			}
 		})
 	}
diff --git a/internal/wasmdebug/dwarf_test.go b/internal/wasmdebug/dwarf_test.go
index da14d42e..84797c2c 100644
--- a/internal/wasmdebug/dwarf_test.go
+++ b/internal/wasmdebug/dwarf_test.go
@@ -3,6 +3,7 @@ package wasmdebug_test
 import (
 	"fmt"
 	"math"
+	"sync"
 	"testing"
 
 	"github.com/tetratelabs/wazero/api"
@@ -44,8 +45,19 @@ func TestDWARFLines_Line_TinyGO(t *testing.T) {
 
 	for _, tc := range tests {
 		t.Run(tc.exp, func(t *testing.T) {
-			actual := mod.DWARFLines.Line(tc.offset)
-			require.Contains(t, actual, tc.exp)
+			// Ensures that DWARFLines.Line is goroutine-safe.
+			const concurrent = 100
+			var wg sync.WaitGroup
+			wg.Add(concurrent)
+
+			for i := 0; i < concurrent; i++ {
+				go func() {
+					defer wg.Done()
+					actual := mod.DWARFLines.Line(tc.offset)
+					require.Contains(t, actual, tc.exp)
+				}()
+			}
+			wg.Wait()
 		})
 	}
 }