Repository: tetratelabs/wazero
Pull request #2148 (2024-03-12 02:34:06)

validation: handles edge case of unexpected else
https://github.com/tetratelabs/wazero/actions/runs/8242242934/job/22540898762


Patch:
diff --git a/.github/workflows/commit.yaml b/.github/workflows/commit.yaml
index 9cc4db36..c8ed060e 100644
--- a/.github/workflows/commit.yaml
+++ b/.github/workflows/commit.yaml
@@ -95,7 +95,7 @@ jobs:
 
       # Run -race could be really slow without -short, so run them together on this workflow.
       # Since -short is not added in the scratch tests, all the tests are run in CI in practice.
-      - run: make test go_test_options='-timeout 10m -race -short'
+      - run: make test go_test_options='-timeout 20m -race -short'
 
       - name: "Generate coverage report"  # only once (not per OS)
         if: runner.os == 'Linux'
diff --git a/internal/wasm/func_validation.go b/internal/wasm/func_validation.go
index df811bc5..8da68907 100644
--- a/internal/wasm/func_validation.go
+++ b/internal/wasm/func_validation.go
@@ -1829,6 +1829,10 @@ func (m *Module) validateFunctionWithMaxStackValues(
 			pc += num
 		} else if op == OpcodeElse {
 			bl := &controlBlockStack.stack[len(controlBlockStack.stack)-1]
+			if bl.op != OpcodeIf {
+				return fmt.Errorf("else instruction must be used in if block: %#x", pc)
+			}
+			bl.op = OpcodeElse
 			bl.elseAt = pc
 			// Check the type soundness of the instructions *before* entering this else Op.
 			if err := valueTypeStack.popResults(OpcodeIf, bl.blockType.Results, true); err != nil {


Test patch:
diff --git a/imports/wasi_snapshot_preview1/gotip_test.go b/imports/wasi_snapshot_preview1/gotip_test.go
deleted file mode 100644
index 490fc959..00000000
--- a/imports/wasi_snapshot_preview1/gotip_test.go
+++ /dev/null
@@ -1,62 +0,0 @@
-package wasi_snapshot_preview1_test
-
-import (
-	"context"
-	"fmt"
-	"os"
-	"os/exec"
-	"path"
-	"path/filepath"
-	"runtime"
-	"testing"
-	"time"
-)
-
-func TestMain(m *testing.M) {
-	// Find the gotip binary (if present), and compile the Wasm binary.
-	if gotipBin, err := findGotipBin(); err != nil {
-		println("gotip: skipping due missing binary:", err)
-	} else if wasmGotip, err = compileWasip1Wasm(gotipBin); err != nil {
-		println("gotip: skipping due compilation error:", err)
-	}
-	os.Exit(m.Run())
-}
-
-// compileWasip1Wasm allows us to generate a binary with runtime.GOOS=wasip1
-// and runtime.GOARCH=wasm. This intentionally does so on-demand, because the
-// wasm is too big to check in. Pls, not everyone will have gotip.
-func compileWasip1Wasm(gotipBin string) ([]byte, error) {
-	// Prepare the working directory.
-	workdir, err := os.MkdirTemp("", "wasi")
-	if err != nil {
-		return nil, err
-	}
-	defer os.RemoveAll(workdir)
-
-	ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
-	defer cancel()
-
-	bin := path.Join(workdir, "wasi.wasm")
-	cmd := exec.CommandContext(ctx, gotipBin, "build", "-o", bin, ".") //nolint:gosec
-	cmd.Env = append(os.Environ(), "GOOS=wasip1", "GOARCH=wasm")
-	cmd.Dir = "testdata/gotip"
-	out, err := cmd.CombinedOutput()
-	if err != nil {
-		return nil, fmt.Errorf("couldn't compile %s: %w", string(out), err)
-	}
-
-	return os.ReadFile(bin) //nolint:gosec
-}
-
-func findGotipBin() (string, error) {
-	binName := "gotip"
-	if runtime.GOOS == "windows" {
-		binName += ".exe"
-	}
-	gotipBin := filepath.Join(runtime.GOROOT(), "bin", binName)
-	if _, err := os.Stat(gotipBin); err == nil {
-		return gotipBin, nil
-	}
-	// Now, search the path
-	return exec.LookPath(binName)
-}
diff --git a/imports/wasi_snapshot_preview1/testdata/gotip/wasi.go b/imports/wasi_snapshot_preview1/testdata/go/wasi.go
similarity index 100%
rename from imports/wasi_snapshot_preview1/testdata/gotip/wasi.go
rename to imports/wasi_snapshot_preview1/testdata/go/wasi.go
diff --git a/imports/wasi_snapshot_preview1/wasi_snapshot_preview1_test.go b/imports/wasi_snapshot_preview1/wasi_snapshot_preview1_test.go
new file mode 100644
index 00000000..08d5a7e7
--- /dev/null
+++ b/imports/wasi_snapshot_preview1/wasi_snapshot_preview1_test.go
@@ -0,0 +1,59 @@
+package wasi_snapshot_preview1_test
+
+import (
+	"context"
+	"fmt"
+	"os"
+	"os/exec"
+	"path"
+	"testing"
+	"time"
+
+	"github.com/tetratelabs/wazero"
+)
+
+func TestMain(m *testing.M) {
+	var err error
+	if wasmGo, err = compileWasip1Wasm(); err != nil {
+		println("go: skipping due compilation error:", err)
+	}
+	os.Exit(m.Run())
+}
+
+// runtimeCfg is a shared runtime configuration for tests within this package to reduce the compilation time of the binary.
+var runtimeCfg = wazero.NewRuntimeConfig().WithCompilationCache(wazero.NewCompilationCache())
+
+// compileWasip1Wasm allows us to generate a binary with runtime.GOOS=wasip1
+// and runtime.GOARCH=wasm. This intentionally does so on-demand, because the
+// wasm is too big to check in.
+func compileWasip1Wasm() ([]byte, error) {
+	// Prepare the working directory.
+	workdir, err := os.MkdirTemp("", "wasi")
+	if err != nil {
+		return nil, err
+	}
+	defer os.RemoveAll(workdir)
+
+	ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
+	defer cancel()
+
+	binPath := path.Join(workdir, "wasi.wasm")
+	cmd := exec.CommandContext(ctx, "go", "build", "-o", binPath, ".") //nolint:gosec
+	cmd.Env = append(os.Environ(), "GOOS=wasip1", "GOARCH=wasm")
+	cmd.Dir = "testdata/go"
+	out, err := cmd.CombinedOutput()
+	if err != nil {
+		return nil, fmt.Errorf("couldn't compile %s: %w", string(out), err)
+	}
+
+	bin, err := os.ReadFile(binPath) //nolint:gosec
+	if err != nil {
+		return nil, err
+	}
+
+	// Proactively compile the binary to reduce the test time.
+	r := wazero.NewRuntimeWithConfig(context.Background(), runtimeCfg)
+	defer r.Close(context.Background())
+	_, err = r.CompileModule(context.Background(), bin)
+	return bin, err
+}
diff --git a/imports/wasi_snapshot_preview1/wasi_stdlib_test.go b/imports/wasi_snapshot_preview1/wasi_stdlib_test.go
index 93ac1a56..9a9d284b 100644
--- a/imports/wasi_snapshot_preview1/wasi_stdlib_test.go
+++ b/imports/wasi_snapshot_preview1/wasi_stdlib_test.go
@@ -40,8 +40,8 @@ var sleepALittle = func() { time.Sleep(500 * time.Millisecond) }
 //go:embed testdata/cargo-wasi/wasi.wasm
 var wasmCargoWasi []byte
 
-// wasmGotip is conditionally compiled from testdata/gotip/wasi.go
-var wasmGotip []byte
+// wasmGo is conditionally compiled from testdata/go/wasi.go
+var wasmGo []byte
 
 // wasmTinyGo was compiled from testdata/tinygo/wasi.go
 //
@@ -65,8 +65,8 @@ func Test_fdReaddir_ls(t *testing.T) {
 		"zig-cc":     wasmZigCc,
 		"zig":        wasmZig,
 	}
-	if wasmGotip != nil {
-		toolchains["gotip"] = wasmGotip
+	if wasmGo != nil {
+		toolchains["go"] = wasmGo
 	}
 
 	tmpDir := t.TempDir()
@@ -178,8 +178,8 @@ func Test_fdReaddir_stat(t *testing.T) {
 		"zig-cc":     wasmZigCc,
 		"zig":        wasmZig,
 	}
-	if wasmGotip != nil {
-		toolchains["gotip"] = wasmGotip
+	if wasmGo != nil {
+		toolchains["go"] = wasmGo
 	}
 
 	for toolchain, bin := range toolchains {
@@ -242,13 +242,16 @@ func compileAndRunWithPreStart(t *testing.T, ctx context.Context, config wazero.
 	// same for console and stderr as sometimes the stack trace is in one or the other.
 	var consoleBuf bytes.Buffer
 
-	r := wazero.NewRuntime(ctx)
+	r := wazero.NewRuntimeWithConfig(ctx, runtimeCfg)
 	defer r.Close(ctx)
 
 	_, err := wasi_snapshot_preview1.Instantiate(ctx, r)
 	require.NoError(t, err)
 
-	mod, err := r.InstantiateWithConfig(ctx, bin, config.
+	compiled, err := r.CompileModule(ctx, bin)
+	require.NoError(t, err)
+
+	mod, err := r.InstantiateModule(ctx, compiled, config.
 		WithStdout(&consoleBuf).
 		WithStderr(&consoleBuf).
 		WithStartFunctions()) // clear
@@ -269,59 +272,6 @@ func compileAndRunWithPreStart(t *testing.T, ctx context.Context, config wazero.
 	return
 }
 
-// compileAndRunForked executes the test case with the wazero runtime in a separate process, and waits for it to terminate.
-//
-// Stdout is captured to a buffer, and stderr is dumped to os.Stderr.
-// It returns the capture buffer and boolean; the boolean is true if we are running in the outer process.
-// If it is false, it means we are running in the subprocess; all the verification logic should be handled
-// when the boolean is true.
-//
-// A typical usage pattern will be:
-//
-//	if buf, hasRun := compileAndRunForked(...); hasRun {
-//		validateContents(buf)
-//	}
-func compileAndRunForked(t *testing.T, ctx context.Context, config wazero.ModuleConfig, tname string, bin []byte) ([]byte, bool) {
-	var buf bytes.Buffer
-	// We use the technique described in https://go.dev/talks/2014/testing.slide#23
-	// We check if we are running forked by ensuring that a "magic" environment variable is set.
-	if os.Getenv("_TEST_FORKED") != "1" {
-		// If said variable is not set, then we need to exec this same executable, specifying the name of test.
-		// We could use t.Name(), but because t may be arbitrarily nested, it is better to get the name of the test
-		// from a parameter.
-		cmd := exec.Command(os.Args[0], "-test.run", tname)
-		cmd.Stdout = &buf
-		cmd.Stderr = os.Stderr
-		cmd.Env = append(os.Environ(), "_TEST_FORKED=1")
-		err := cmd.Run()
-		if e, ok := err.(*exec.ExitError); ok && !e.Success() {
-			require.NoError(t, e, "The test quit with an error code: %v\n", e)
-		}
-		res := buf.Bytes()
-		// This is a test, so in case of success, it will include the "PASS\n" string:
-		// we remove that from the output, to return a clean stdout.
-		return res[0 : len(res)-len("PASS\n")], true
-	}
-
-	r := wazero.NewRuntime(ctx)
-	defer r.Close(ctx)
-
-	_, err := wasi_snapshot_preview1.Instantiate(ctx, r)
-	require.NoError(t, err)
-
-	mod, err := r.InstantiateWithConfig(ctx, bin, config.
-		WithStartFunctions()) // clear
-	require.NoError(t, err)
-
-	_, err = mod.ExportedFunction("_start").Call(ctx)
-	if exitErr, ok := err.(*sys.ExitError); ok {
-		require.Zero(t, exitErr.ExitCode())
-	} else {
-		require.NoError(t, err)
-	}
-	return nil, false
-}
-
 func Test_Poll(t *testing.T) {
 	// The following test cases replace Stdin with a custom reader.
 	// For more precise coverage, see poll_test.go.
@@ -439,8 +389,8 @@ func Test_Sock(t *testing.T) {
 		"cargo-wasi": wasmCargoWasi,
 		"zig-cc":     wasmZigCc,
 	}
-	if wasmGotip != nil {
-		toolchains["gotip"] = wasmGotip
+	if wasmGo != nil {
+		toolchains["go"] = wasmGo
 	}
 
 	for toolchain, bin := range toolchains {
@@ -483,8 +433,8 @@ func testSock(t *testing.T, bin []byte) {
 
 func Test_HTTP(t *testing.T) {
 	toolchains := map[string][]byte{}
-	if wasmGotip != nil {
-		toolchains["gotip"] = wasmGotip
+	if wasmGo != nil {
+		toolchains["go"] = wasmGo
 	}
 
 	for toolchain, bin := range toolchains {
@@ -535,8 +485,8 @@ func testHTTP(t *testing.T, bin []byte) {
 
 func Test_Stdin(t *testing.T) {
 	toolchains := map[string][]byte{}
-	if wasmGotip != nil {
-		toolchains["gotip"] = wasmGotip
+	if wasmGo != nil {
+		toolchains["go"] = wasmGo
 	}
 
 	for toolchain, bin := range toolchains {
@@ -568,12 +518,18 @@ func testStdin(t *testing.T, bin []byte) {
 	ch := make(chan struct{}, 1)
 	go func() {
 		defer close(ch)
+		r := wazero.NewRuntimeWithConfig(testCtx, runtimeCfg)
+		defer func() {
+			require.NoError(t, r.Close(testCtx))
+		}()
 
-		r := wazero.NewRuntime(testCtx)
-		defer r.Close(testCtx)
 		_, err := wasi_snapshot_preview1.Instantiate(testCtx, r)
 		require.NoError(t, err)
-		_, err = r.InstantiateWithConfig(testCtx, bin, moduleConfig)
+
+		compiled, err := r.CompileModule(testCtx, wasmGo)
+		require.NoError(t, err)
+
+		_, err = r.InstantiateModule(testCtx, compiled, moduleConfig) // clear
 		require.NoError(t, err)
 	}()
 
@@ -590,44 +546,32 @@ func testStdin(t *testing.T, bin []byte) {
 }
 
 func Test_LargeStdout(t *testing.T) {
-	toolchains := map[string][]byte{}
-	if wasmGotip != nil {
-		toolchains["gotip"] = wasmGotip
-	}
+	if wasmGo != nil {
+		var buf bytes.Buffer
+		r := wazero.NewRuntimeWithConfig(testCtx, runtimeCfg)
+		defer func() {
+			require.NoError(t, r.Close(testCtx))
+		}()
 
-	for toolchain, bin := range toolchains {
-		toolchain := toolchain
-		bin := bin
-		name := t.Name()
-		t.Run(toolchain, func(t *testing.T) {
-			testLargeStdout(t, name, bin)
-		})
-	}
-}
-
-func testLargeStdout(t *testing.T, tname string, bin []byte) {
-	// This test dumps a large Go source file to stdout. The generated result
-	// should be valid code, otherwise it means that stdout is corrupted.
-	//
-	// The error conditions are more easily reproduced by executing in a subprocess
-	// and capturing its stdout.
-	if buf, hasRun := compileAndRunForked(t, testCtx, wazero.NewModuleConfig().
-		WithArgs("wasi", "largestdout").
-		WithStdout(os.Stdout), tname, bin); hasRun {
+		_, err := wasi_snapshot_preview1.Instantiate(testCtx, r)
+		require.NoError(t, err)
 
-		tempDir := t.TempDir()
-		temp, err := os.Create(joinPath(tempDir, "out.go"))
+		compiled, err := r.CompileModule(testCtx, wasmGo)
 		require.NoError(t, err)
-		defer temp.Close()
 
+		_, err = r.InstantiateModule(testCtx, compiled, wazero.NewModuleConfig().
+			WithArgs("wasi", "largestdout").
+			WithStdout(&buf)) // clear
 		require.NoError(t, err)
-		_, _ = temp.Write(buf)
-		_ = temp.Close()
 
-		gotipBin, err := findGotipBin()
+		tempDir := t.TempDir()
+		temp, err := os.Create(joinPath(tempDir, "out.go"))
 		require.NoError(t, err)
 
-		cmd := exec.CommandContext(testCtx, gotipBin, "build", "-o",
+		// Check if the output Go source code is valid.
+		_, _ = temp.Write(buf.Bytes())
+		require.NoError(t, temp.Close())
+		cmd := exec.CommandContext(testCtx, "go", "build", "-o",
 			joinPath(tempDir, "outbin"), temp.Name())
 		require.NoError(t, err)
 		output, err := cmd.CombinedOutput()
diff --git a/imports/wasi_snapshot_preview1/wasi_stdlib_unix_test.go b/imports/wasi_snapshot_preview1/wasi_stdlib_unix_test.go
index 53195d5a..8af6daf3 100644
--- a/imports/wasi_snapshot_preview1/wasi_stdlib_unix_test.go
+++ b/imports/wasi_snapshot_preview1/wasi_stdlib_unix_test.go
@@ -67,9 +67,9 @@ type fifo struct {
 	path string
 }
 
-func Test_NonblockGotip(t *testing.T) {
+func Test_NonblockGo(t *testing.T) {
 	// - Create `numFifos` FIFOs.
-	// - Instantiate `wasmGotip` with the names of the FIFO in the order of creation
+	// - Instantiate `wasmGo` with the names of the FIFO in the order of creation
 	// - The test binary opens the FIFOs in the given order and spawns a goroutine for each
 	// - The unit test writes to the FIFO in reverse order.
 	// - Each goroutine reads from the given FIFO and writes the contents to stderr
@@ -81,8 +81,8 @@ func Test_NonblockGotip(t *testing.T) {
 	//
 	// Adapted from https://github.com/golang/go/blob/0fcc70ecd56e3b5c214ddaee4065ea1139ae16b5/src/runtime/internal/wasitest/nonblock_test.go
 
-	if wasmGotip == nil {
-		t.Skip("skipping because wasi.go was not compiled (gotip missing or compilation error)")
+	if wasmGo == nil {
+		t.Skip("skipping because wasi.go was not compiled (go missing or compilation error)")
 	}
 	const numFifos = 8
 
@@ -122,13 +122,18 @@ func Test_NonblockGotip(t *testing.T) {
 
 			ch := make(chan string, 1)
 			go func() {
-				r := wazero.NewRuntime(testCtx)
-				defer r.Close(testCtx)
+				r := wazero.NewRuntimeWithConfig(testCtx, runtimeCfg)
+				defer func() {
+					require.NoError(t, r.Close(testCtx))
+				}()
 
 				_, err := wasi_snapshot_preview1.Instantiate(testCtx, r)
 				require.NoError(t, err)
 
-				mod, err := r.InstantiateWithConfig(testCtx, wasmGotip, moduleConfig) // clear
+				compiled, err := r.CompileModule(testCtx, wasmGo)
+				require.NoError(t, err)
+
+				mod, err := r.InstantiateModule(testCtx, compiled, moduleConfig)
 				require.NoError(t, err)
 
 				_, err = mod.ExportedFunction("_start").Call(testCtx)
diff --git a/internal/integration_test/fuzz/fuzz/fuzz_targets/validation.rs b/internal/integration_test/fuzz/fuzz/fuzz_targets/validation.rs
index 7c2dead3..4848960e 100644
--- a/internal/integration_test/fuzz/fuzz/fuzz_targets/validation.rs
+++ b/internal/integration_test/fuzz/fuzz/fuzz_targets/validation.rs
@@ -17,6 +17,7 @@ fn run(data: &[u8]) -> Result<()> {
 
     // Generate the configuration with possibly invalid functions.
     let mut config = Config::arbitrary(&mut u)?;
+    config.threads_enabled = true;
     config.allow_invalid_funcs = true;
 
     let module = wasm_smith::Module::new(config.clone(), &mut u)?;
diff --git a/internal/integration_test/fuzz/wazerolib/validate.go b/internal/integration_test/fuzz/wazerolib/validate.go
index 9b4c5757..6d52302b 100644
--- a/internal/integration_test/fuzz/wazerolib/validate.go
+++ b/internal/integration_test/fuzz/wazerolib/validate.go
@@ -4,12 +4,15 @@ import (
 	"context"
 
 	"github.com/tetratelabs/wazero"
+	"github.com/tetratelabs/wazero/api"
+	"github.com/tetratelabs/wazero/experimental"
 )
 
 // Ensure that validation and compilation do not panic!
 func tryCompile(wasmBin []byte) {
 	ctx := context.Background()
-	r := wazero.NewRuntimeWithConfig(ctx, wazero.NewRuntimeConfigCompiler())
+	r := wazero.NewRuntimeWithConfig(ctx, wazero.NewRuntimeConfigCompiler().
+		WithCoreFeatures(api.CoreFeaturesV2|experimental.CoreFeaturesThreads))
 	defer func() {
 		if err := r.Close(context.Background()); err != nil {
 			panic(err)
diff --git a/internal/wasm/func_validation_test.go b/internal/wasm/func_validation_test.go
index f7e53324..453ae457 100644
--- a/internal/wasm/func_validation_test.go
+++ b/internal/wasm/func_validation_test.go
@@ -3591,14 +3591,37 @@ func TestFunctionValidation_redundantEnd(t *testing.T) {
 
 // TestFunctionValidation_redundantEnd is found in th validation fuzzing.
 func TestFunctionValidation_redundantElse(t *testing.T) {
-	m := &Module{
-		TypeSection:     []FunctionType{{}},
-		FunctionSection: []Index{0},
-		CodeSection:     []Code{{Body: []byte{OpcodeEnd, OpcodeElse}}},
+	for _, tc := range []struct {
+		body   []byte
+		expErr string
+	}{
+		{
+			body:   []byte{OpcodeEnd, OpcodeElse},
+			expErr: "unexpected end of function at pc=0x1",
+		},
+		{
+			body:   []byte{OpcodeElse, OpcodeEnd},
+			expErr: "else instruction must be used in if block: 0x0",
+		},
+		{
+			body:   []byte{OpcodeBlock, 0, OpcodeElse, OpcodeEnd},
+			expErr: "else instruction must be used in if block: 0x2",
+		},
+		{
+			body: []byte{
+				OpcodeI32Const, 0,
+				OpcodeIf, 0, OpcodeElse, OpcodeElse, OpcodeEnd, OpcodeEnd,
+			},
+			expErr: "else instruction must be used in if block: 0x5",
+		},
+	} {
+		t.Run(tc.expErr, func(t *testing.T) {
+			m := &Module{TypeSection: []FunctionType{{}}, FunctionSection: []Index{0}, CodeSection: []Code{{Body: tc.body}}}
+			err := m.validateFunction(&stacks{}, api.CoreFeaturesV2,
+				0, nil, nil, nil, nil, nil, bytes.NewReader(nil))
+			require.EqualError(t, err, tc.expErr)
+		})
 	}
-	err := m.validateFunction(&stacks{}, api.CoreFeaturesV2,
-		0, nil, nil, nil, nil, nil, bytes.NewReader(nil))
-	require.EqualError(t, err, "unexpected end of function at pc=0x1")
 }
 
 func Test_SplitCallStack(t *testing.T) {