helper: eliminate commandContext replacement

This is done more cleanly by modifying Args in cmdF. Signed-off-by: Ophestra <cat@gensokyo.uk>
2025-03-15 23:57:44 +09:00
parent bac4e67867
commit 6e7ddb2d2e
12 changed files with 205 additions and 148 deletions
--- a/helper/bwrap_test.go
+++ b/helper/bwrap_test.go
@@ -3,6 +3,8 @@ package helper_test
 import (
 	"context"
 	"errors"
+	"fmt"
+	"io"
 	"os"
 	"os/exec"
 	"strings"
@@ -17,7 +19,7 @@ func TestBwrap(t *testing.T) {
 	sc := &bwrap.Config{
 		Net:           true,
 		Hostname:      "localhost",
-		Chdir:         "/nonexistent",
+		Chdir:         "/proc/nonexistent",
 		Clearenv:      true,
 		NewSession:    true,
 		DieWithParent: true,
@@ -26,14 +28,12 @@ func TestBwrap(t *testing.T) {

 	t.Run("nonexistent bwrap name", func(t *testing.T) {
 		bubblewrapName := helper.BubblewrapName
-		helper.BubblewrapName = "/nonexistent"
-		t.Cleanup(func() {
-			helper.BubblewrapName = bubblewrapName
-		})
+		helper.BubblewrapName = "/proc/nonexistent"
+		t.Cleanup(func() { helper.BubblewrapName = bubblewrapName })

 		h := helper.MustNewBwrap(
 			context.Background(),
-			"fortify",
+			"false",
 			argsWt, false,
 			argF, nil,
 			nil,
@@ -49,14 +49,14 @@ func TestBwrap(t *testing.T) {
 	t.Run("valid new helper nil check", func(t *testing.T) {
 		if got := helper.MustNewBwrap(
 			context.TODO(),
-			"fortify",
+			"false",
 			argsWt, false,
 			argF, nil,
 			nil,
 			sc, nil,
 		); got == nil {
 			t.Errorf("MustNewBwrap(%#v, %#v, %#v) got nil",
-				sc, argsWt, "fortify")
+				sc, argsWt, "false")
 			return
 		}
 	})
@@ -72,7 +72,7 @@ func TestBwrap(t *testing.T) {

 		helper.MustNewBwrap(
 			context.TODO(),
-			"fortify",
+			"false",
 			argsWt, false,
 			argF, nil,
 			nil,
@@ -81,15 +81,13 @@ func TestBwrap(t *testing.T) {
 	})

 	t.Run("start without pipes", func(t *testing.T) {
-		helper.InternalReplaceExecCommand(t)
-
 		ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
 		defer cancel()
 		stdout, stderr := new(strings.Builder), new(strings.Builder)
 		h := helper.MustNewBwrap(
-			ctx, "crash-test-dummy",
+			ctx, os.Args[0],
 			nil, false,
-			argFChecked, func(cmd *exec.Cmd) { cmd.Stdout, cmd.Stderr = stdout, stderr },
+			argFChecked, func(cmd *exec.Cmd) { cmd.Stdout, cmd.Stderr = stdout, stderr; hijackBwrap(cmd) },
 			nil,
 			sc, nil,
 		)
@@ -107,14 +105,23 @@ func TestBwrap(t *testing.T) {
 	})

 	t.Run("implementation compliance", func(t *testing.T) {
-		testHelper(t, func(ctx context.Context, cmdF func(cmd *exec.Cmd), stat bool) helper.Helper {
+		testHelper(t, func(ctx context.Context, setOutput func(stdoutP, stderrP *io.Writer), stat bool) helper.Helper {
 			return helper.MustNewBwrap(
-				ctx, "crash-test-dummy",
+				ctx, os.Args[0],
 				argsWt, stat,
-				argF, cmdF,
+				argF, func(cmd *exec.Cmd) { setOutput(&cmd.Stdout, &cmd.Stderr); hijackBwrap(cmd) },
 				nil,
 				sc, nil,
 			)
-		})
+		}, "exec")
 	})
 }
+
+func hijackBwrap(cmd *exec.Cmd) {
+	if cmd.Args[0] != "bwrap" {
+		panic(fmt.Sprintf("unexpected argv0 %q", cmd.Args[0]))
+	}
+	cmd.Err = nil
+	cmd.Path = os.Args[0]
+	cmd.Args = append([]string{os.Args[0], "-test.run=TestHelperStub", "--"}, cmd.Args...)
+}
--- a/helper/cmd.go
+++ b/helper/cmd.go
@@ -42,7 +42,7 @@ func newHelperCmd(
 ) (cmd *helperCmd, args []string) {
 	cmd = new(helperCmd)
 	cmd.helperFiles, args = newHelperFiles(ctx, wt, stat, argF, extraFiles)
-	cmd.Cmd = commandContext(ctx, name)
+	cmd.Cmd = exec.CommandContext(ctx, name)
 	cmd.Cmd.Cancel = func() error { return cmd.Process.Signal(syscall.SIGTERM) }
 	cmd.WaitDelay = WaitDelay
 	return
--- a/helper/cmd_test.go
+++ b/helper/cmd_test.go
@@ -3,6 +3,7 @@ package helper_test
 import (
 	"context"
 	"errors"
+	"io"
 	"os"
 	"os/exec"
 	"testing"
@@ -10,9 +11,9 @@ import (
 	"git.gensokyo.uk/security/fortify/helper"
 )

-func TestDirect(t *testing.T) {
+func TestCmd(t *testing.T) {
 	t.Run("start non-existent helper path", func(t *testing.T) {
-		h := helper.NewDirect(context.Background(), "/nonexistent", argsWt, false, argF, nil, nil)
+		h := helper.NewDirect(context.Background(), "/proc/nonexistent", argsWt, false, argF, nil, nil)

 		if err := h.Start(); !errors.Is(err, os.ErrNotExist) {
 			t.Errorf("Start: error = %v, wantErr %v",
@@ -22,15 +23,17 @@ func TestDirect(t *testing.T) {

 	t.Run("valid new helper nil check", func(t *testing.T) {
 		if got := helper.NewDirect(context.TODO(), "fortify", argsWt, false, argF, nil, nil); got == nil {
-			t.Errorf("New(%q, %q) got nil",
+			t.Errorf("NewDirect(%q, %q) got nil",
 				argsWt, "fortify")
 			return
 		}
 	})

 	t.Run("implementation compliance", func(t *testing.T) {
-		testHelper(t, func(ctx context.Context, cmdF func(cmd *exec.Cmd), stat bool) helper.Helper {
-			return helper.NewDirect(ctx, "crash-test-dummy", argsWt, stat, argF, cmdF, nil)
-		})
+		testHelper(t, func(ctx context.Context, setOutput func(stdoutP, stderrP *io.Writer), stat bool) helper.Helper {
+			return helper.NewDirect(ctx, os.Args[0], argsWt, stat, argF, func(cmd *exec.Cmd) {
+				setOutput(&cmd.Stdout, &cmd.Stderr)
+			}, nil)
+		}, "exec")
 	})
 }
--- a/helper/helper.go
+++ b/helper/helper.go
@@ -6,17 +6,12 @@ import (
 	"fmt"
 	"io"
 	"os"
-	"os/exec"
 	"time"

 	"git.gensokyo.uk/security/fortify/helper/proc"
 )

-var (
-	WaitDelay = 2 * time.Second
-
-	commandContext = exec.CommandContext
-)
+var WaitDelay = 2 * time.Second

 const (
 	// FortifyHelper is set to 1 when args fd is enabled and 0 otherwise.
--- a/helper/helper_test.go
+++ b/helper/helper_test.go
@@ -4,7 +4,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"os/exec"
+	"io"
 	"strconv"
 	"strings"
 	"testing"
@@ -36,7 +36,8 @@ func argF(argsFd, statFd int) []string {
 }

 func argFChecked(argsFd, statFd int) (args []string) {
-	args = make([]string, 0, 4)
+	args = make([]string, 0, 6)
+	args = append(args, "-test.run=TestHelperStub", "--")
 	if argsFd > -1 {
 		args = append(args, "--args", strconv.Itoa(argsFd))
 	}
@@ -47,13 +48,14 @@ func argFChecked(argsFd, statFd int) (args []string) {
 }

 // this function tests an implementation of the helper.Helper interface
-func testHelper(t *testing.T, createHelper func(ctx context.Context, cmdF func(cmd *exec.Cmd), stat bool) helper.Helper) {
-	helper.InternalReplaceExecCommand(t)
-
+func testHelper(t *testing.T,
+	createHelper func(ctx context.Context, setOutput func(stdoutP, stderrP *io.Writer), stat bool) helper.Helper,
+	prefix string,
+) {
 	t.Run("start helper with status channel and wait", func(t *testing.T) {
 		ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
 		stdout, stderr := new(strings.Builder), new(strings.Builder)
-		h := createHelper(ctx, func(cmd *exec.Cmd) { cmd.Stdout, cmd.Stderr = stdout, stderr }, true)
+		h := createHelper(ctx, func(stdoutP, stderrP *io.Writer) { *stdoutP, *stderrP = stdout, stderr }, true)

 		t.Run("wait not yet started helper", func(t *testing.T) {
 			defer func() {
@@ -75,7 +77,7 @@ func testHelper(t *testing.T, createHelper func(ctx context.Context, cmdF func(c
 		cancel()

 		t.Run("start already started helper", func(t *testing.T) {
-			wantErr := "exec: already started"
+			wantErr := prefix + ": already started"
 			if err := h.Start(); err != nil && err.Error() != wantErr {
 				t.Errorf("Start: error = %v, wantErr %v",
 					err, wantErr)
@@ -108,7 +110,7 @@ func testHelper(t *testing.T, createHelper func(ctx context.Context, cmdF func(c
 		ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second)
 		defer cancel()
 		stdout, stderr := new(strings.Builder), new(strings.Builder)
-		h := createHelper(ctx, func(cmd *exec.Cmd) { cmd.Stdout, cmd.Stderr = stdout, stderr }, false)
+		h := createHelper(ctx, func(stdoutP, stderrP *io.Writer) { *stdoutP, *stderrP = stdout, stderr }, false)

 		if err := h.Start(); err != nil {
 			t.Errorf("Start() error = %v",
--- a/helper/stub.go
+++ b/helper/stub.go
@@ -1,25 +1,24 @@
 package helper

 import (
-	"context"
 	"flag"
 	"fmt"
 	"io"
 	"os"
-	"os/exec"
+	"path"
+	"slices"
 	"strconv"
 	"strings"
 	"syscall"
-	"testing"

 	"git.gensokyo.uk/security/fortify/helper/bwrap"
 	"git.gensokyo.uk/security/fortify/helper/proc"
 	"git.gensokyo.uk/security/fortify/internal"
 )

-// InternalChildStub is an internal function but exported because it is cross-package;
+// InternalHelperStub is an internal function but exported because it is cross-package;
 // it is part of the implementation of the helper stub.
-func InternalChildStub() {
+func InternalHelperStub() {
 	// this test mocks the helper process
 	var ap, sp string
 	if v, ok := os.LookupEnv(FortifyHelper); !ok {
@@ -33,32 +32,15 @@ func InternalChildStub() {
 		sp = v
 	}

-	switch os.Args[3] {
-	case "bwrap":
+	if len(os.Args) > 3 && os.Args[3] == "bwrap" {
 		bwrapStub()
-	default:
-		genericStub(flagRestoreFiles(4, ap, sp))
+	} else {
+		genericStub(flagRestoreFiles(3, ap, sp))
 	}

 	internal.Exit(0)
 }

-// InternalReplaceExecCommand is an internal function but exported because it is cross-package;
-// it is part of the implementation of the helper stub.
-func InternalReplaceExecCommand(t *testing.T) {
-	t.Cleanup(func() { commandContext = exec.CommandContext })
-
-	// replace execCommand to have the resulting *exec.Cmd launch TestHelperChildStub
-	commandContext = func(ctx context.Context, name string, arg ...string) *exec.Cmd {
-		// pass through nonexistent path
-		if name == "/nonexistent" && len(arg) == 0 {
-			return exec.CommandContext(ctx, name)
-		}
-
-		return exec.CommandContext(ctx, os.Args[0], append([]string{"-test.run=TestHelperChildStub", "--", name}, arg...)...)
-	}
-}
-
 func newFile(fd int, name, p string) *os.File {
 	present := false
 	switch p {
@@ -149,26 +131,54 @@ func bwrapStub() {
 		sc := &bwrap.Config{
 			Net:           true,
 			Hostname:      "localhost",
-			Chdir:         "/nonexistent",
+			Chdir:         "/proc/nonexistent",
 			Clearenv:      true,
 			NewSession:    true,
 			DieWithParent: true,
 			AsInit:        true,
 		}
-		if _, err := MustNewCheckedArgs(sc.Args(nil, new(proc.ExtraFilesPre), new([]proc.File))).
+
+		efp := new(proc.ExtraFilesPre)
+		if t, ok := os.LookupEnv("GO_TEST_FORTIFY_BWRAP_STUB_TYPE"); ok {
+			switch t {
+			case "dbus":
+				sc.Net = false
+				sc.Hostname = "fortify-dbus"
+				sc.Chdir = "/"
+				sc.Syscall = &bwrap.SyscallPolicy{DenyDevel: true, Multiarch: true}
+				sc.AsInit = false
+
+				bindTarget := []string{"/tmp/fortify.1971/12622d846cc3fe7b4c10359d01f0eb47"}
+				slices.Sort(bindTarget)
+				for _, name := range bindTarget {
+					sc.Bind(name, name, false, true)
+				}
+				roBindTarget := []string{"/run/user/1971", path.Dir(os.Args[0])}
+				slices.Sort(roBindTarget)
+				for _, name := range roBindTarget {
+					sc.Bind(name, name)
+				}
+
+				// manipulate extra files list so fd ends up as 5
+				efp.Append()
+				efp.Append()
+			}
+		}
+
+		if _, err := MustNewCheckedArgs(sc.Args(nil, efp, new([]proc.File))).
 			WriteTo(want); err != nil {
 			panic("cannot read want: " + err.Error())
 		}

-		if len(flag.CommandLine.Args()) > 0 && flag.CommandLine.Args()[0] == "crash-test-dummy" && got.String() != want.String() {
+		if got.String() != want.String() {
 			panic("bad bwrap args\ngot: " + got.String() + "\nwant: " + want.String())
 		}
 	}()

 	if err := syscall.Exec(
-		os.Args[0],
-		append([]string{os.Args[0], "-test.run=TestHelperChildStub", "--"}, flag.CommandLine.Args()...),
+		flag.CommandLine.Args()[0],
+		flag.CommandLine.Args(),
 		os.Environ()); err != nil {
-		panic("cannot start general stub: " + err.Error())
+		panic("cannot start helper stub: " + err.Error())
 	}
 }
--- a/helper/stub_test.go
+++ b/helper/stub_test.go
@@ -6,6 +6,4 @@ import (
 	"git.gensokyo.uk/security/fortify/helper"
 )

-func TestHelperChildStub(t *testing.T) {
-	helper.InternalChildStub()
-}
+func TestHelperStub(t *testing.T) { helper.InternalHelperStub() }