helper: separate pipes from Helper

Upcoming bwrap helper implementation requires two sets of pipes to be managed, fd will also no longer be constant.

Signed-off-by: Ophestra Umiker <cat@ophivana.moe>

dbus/dbus_test.go

@@ -164,7 +164,7 @@ func TestProxy_Start_Wait_Close_String(t *testing.T) {
 					}
 
 					t.Run("started string of "+id, func(t *testing.T) {
-						wantSubstr := binPath + " --args=3 --fd=4"
+						wantSubstr := binPath + " --args=3"
 						if got := p.String(); !strings.Contains(got, wantSubstr) {
 							t.Errorf("String() = %v, want %v",
 								p.String(), wantSubstr)

dbus/run.go

@@ -3,6 +3,7 @@ package dbus
 import (
 	"errors"
 	"io"
+	"strconv"
 
 	"git.ophivana.moe/cat/fortify/helper"
 )
@@ -18,9 +19,13 @@ func (p *Proxy) Start(ready chan error, output io.Writer) error {
 	}
 
 	h := helper.New(p.seal, p.path,
-		// Helper: Args is always 3 and status if set is always 4.
+		func(argsFD, statFD int) []string {
-		"--args=3",
+			if statFD == -1 {
-		"--fd=4",
+				return []string{"--args=" + strconv.Itoa(argsFD)}
+			} else {
+				return []string{"--args=" + strconv.Itoa(argsFD), "--fd=" + strconv.Itoa(statFD)}
+			}
+		},
 	)
 	// xdg-dbus-proxy does not need to inherit the environment
 	h.Env = []string{}

helper/args.go

@@ -10,11 +10,11 @@ var (
 	ErrContainsNull = errors.New("argument contains null character")
 )
 
-type argsFD []string
+type argsWt []string
 
 // checks whether any element contains the null character
 // must be called before args use and args must not be modified after call
-func (a argsFD) check() error {
+func (a argsWt) check() error {
 	for _, arg := range a {
 		for _, b := range arg {
 			if b == '\x00' {
@@ -26,7 +26,7 @@ func (a argsFD) check() error {
 	return nil
 }
 
-func (a argsFD) WriteTo(w io.Writer) (int64, error) {
+func (a argsWt) WriteTo(w io.Writer) (int64, error) {
 	// assuming already checked
 
 	nt := 0
@@ -43,13 +43,13 @@ func (a argsFD) WriteTo(w io.Writer) (int64, error) {
 	return int64(nt), nil
 }
 
-func (a argsFD) String() string {
+func (a argsWt) String() string {
 	return strings.Join(a, " ")
 }
 
 // NewCheckedArgs returns a checked argument writer for args.
 // Callers must not retain any references to args.
 func NewCheckedArgs(args []string) (io.WriterTo, error) {
-	a := argsFD(args)
+	a := argsWt(args)
 	return a, a.check()
 }

helper/helper.go

@@ -6,7 +6,6 @@ package helper
 import (
 	"errors"
 	"io"
-	"os"
 	"os/exec"
 	"sync"
 )
@@ -17,27 +16,21 @@ var (
 )
 
 const (
+	// FortifyHelper is set for the process launched by Helper.
 	FortifyHelper = "FORTIFY_HELPER"
+	// FortifyStatus is 1 when sync fd is enabled and 0 otherwise.
 	FortifyStatus = "FORTIFY_STATUS"
 )
 
 // Helper wraps *exec.Cmd and manages status and args fd.
 // Args is always 3 and status if set is always 4.
 type Helper struct {
-	args io.WriterTo
+	p *pipes
 
-	statP [2]*os.File
+	argF func(argsFD, statFD int) []string
-	argsP [2]*os.File
+	*exec.Cmd
-
-	ready chan error
-
-	// ExtraFiles specifies additional open files to be inherited by the
-	// new process. It does not include standard input, standard output, or
-	// standard error. If non-nil, entry i becomes file descriptor 5+i.
-	ExtraFiles []*os.File
 
 	lock sync.RWMutex
-	*exec.Cmd
 }
 
 func (h *Helper) StartNotify(ready chan error) error {
@@ -50,95 +43,24 @@ func (h *Helper) StartNotify(ready chan error) error {
 		return errors.New("exec: already started")
 	}
 
-	// create pipes
+	h.p.ready = ready
-	if pr, pw, err := os.Pipe(); err != nil {
+	if argsFD, statFD, err := h.p.prepareCmd(h.Cmd); err != nil {
 		return err
 	} else {
-		h.argsP[0], h.argsP[1] = pr, pw
+		h.Cmd.Args = append(h.Cmd.Args, h.argF(argsFD, statFD)...)
 	}
-	// create status pipes if ready signal is requested
-	var sv string
-	if ready != nil {
-		if pr, pw, err := os.Pipe(); err != nil {
-			return err
-		} else {
-			h.statP[0], h.statP[1] = pr, pw
-		}
 
-		sv = FortifyStatus + "=1"
+	if ready != nil {
+		h.Cmd.Env = append(h.Cmd.Env, FortifyHelper+"=1", FortifyStatus+"=1")
 	} else {
-		sv = FortifyStatus + "=0"
+		h.Cmd.Env = append(h.Cmd.Env, FortifyHelper+"=1", FortifyStatus+"=0")
 	}
 
-	// prepare extra files from caller
-	el := len(h.ExtraFiles)
-	if ready != nil {
-		el += 2
-	} else {
-		el++
-	}
-	ef := make([]*os.File, 0, el)
-	ef = append(ef, h.argsP[0])
-	if ready != nil {
-		ef = append(ef, h.statP[1])
-	}
-	ef = append(ef, h.ExtraFiles...)
-
-	// prepare and start process
-	h.Cmd.ExtraFiles = ef
-	h.Cmd.Env = append(h.Cmd.Env, FortifyHelper+"=1", sv)
 	if err := h.Cmd.Start(); err != nil {
 		return err
 	}
-
+	if err := h.p.readyWriteArgs(); err != nil {
-	statsP, argsP := h.statP[0], h.argsP[1]
-
-	// write arguments and close args pipe
-	if _, err := h.args.WriteTo(argsP); err != nil {
-		if err1 := h.Cmd.Process.Kill(); err1 != nil {
-			// should be unreachable
-			panic(err1.Error())
-		}
 		return err
-	} else {
-		if err = argsP.Close(); err != nil {
-			if err1 := h.Cmd.Process.Kill(); err1 != nil {
-				// should be unreachable
-				panic(err1.Error())
-			}
-			return err
-		}
-	}
-
-	if ready != nil {
-		h.ready = ready
-
-		// monitor stat pipe
-		go func() {
-			n, err := statsP.Read(make([]byte, 1))
-			switch n {
-			case -1:
-				if err1 := h.Cmd.Process.Kill(); err1 != nil {
-					// should be unreachable
-					panic(err1.Error())
-				}
-				// ensure error is not nil
-				if err == nil {
-					err = ErrStatusFault
-				}
-				ready <- err
-			case 0:
-				// ensure error is not nil
-				if err == nil {
-					err = ErrStatusRead
-				}
-				ready <- err
-			case 1:
-				ready <- nil
-			default:
-				panic("unreachable") // unexpected read count
-			}
-		}()
 	}
 
 	return nil
@@ -155,38 +77,12 @@ func (h *Helper) Wait() error {
 		return errors.New("exec: Wait was already called")
 	}
 
-	// ensure pipe close
+	defer h.p.mustClosePipes()
-	defer func() {
-		if err := h.argsP[0].Close(); err != nil && !errors.Is(err, os.ErrClosed) {
-			// unreachable
-			panic(err.Error())
-		}
-		if err := h.argsP[1].Close(); err != nil && !errors.Is(err, os.ErrClosed) {
-			// unreachable
-			panic(err.Error())
-		}
-
-		if h.ready != nil {
-			if err := h.statP[0].Close(); err != nil && !errors.Is(err, os.ErrClosed) {
-				// unreachable
-				panic(err.Error())
-			}
-			if err := h.statP[1].Close(); err != nil && !errors.Is(err, os.ErrClosed) {
-				// unreachable
-				panic(err.Error())
-			}
-		}
-	}()
-
 	return h.Cmd.Wait()
 }
 
 func (h *Helper) Close() error {
-	if h.ready == nil {
+	return h.p.closeStatus()
-		panic("attempted to close helper with no status pipe")
-	}
-
-	return h.statP[0].Close()
 }
 
 func (h *Helper) Start() error {
@@ -195,10 +91,12 @@ func (h *Helper) Start() error {
 
 var execCommand = exec.Command
 
-func New(wt io.WriterTo, name string, arg ...string) *Helper {
+// New initialises a new Helper instance with wt as the null-terminated argument writer.
+// Function argF returns an array of arguments passed directly to the child process.
+func New(wt io.WriterTo, name string, argF func(argsFD, statFD int) []string) *Helper {
 	if wt == nil {
 		panic("attempted to create helper with invalid argument writer")
 	}
 
-	return &Helper{args: wt, Cmd: execCommand(name, arg...)}
+	return &Helper{p: &pipes{args: wt}, argF: argF, Cmd: execCommand(name)}
 }

helper/helper_test.go

@@ -4,9 +4,11 @@ import (
 	"errors"
 	"io"
 	"os"
+	"strconv"
 	"strings"
 	"sync"
 	"testing"
+	"time"
 
 	"git.ophivana.moe/cat/fortify/helper"
 )
@@ -36,12 +38,20 @@ func prepareArgs() {
 	}
 }
 
+func argF(argsFD int, _ int) []string {
+	return []string{"--args", strconv.Itoa(argsFD)}
+}
+
+func argFStatus(argsFD int, statFD int) []string {
+	return []string{"--args", strconv.Itoa(argsFD), "--fd", strconv.Itoa(statFD)}
+}
+
 func TestHelper_StartNotify_Close_Wait(t *testing.T) {
 	helper.InternalReplaceExecCommand(t)
 	argsOnce.Do(prepareArgs)
 
 	t.Run("start non-existent helper path", func(t *testing.T) {
-		h := helper.New(argsWt, "/nonexistent")
+		h := helper.New(argsWt, "/nonexistent", argF)
 
 		if err := h.Start(); !errors.Is(err, os.ErrNotExist) {
 			t.Errorf("Start() error = %v, wantErr %v",
@@ -50,7 +60,7 @@ func TestHelper_StartNotify_Close_Wait(t *testing.T) {
 	})
 
 	t.Run("start helper with status channel", func(t *testing.T) {
-		h := helper.New(argsWt, "crash-test-dummy", "--args=3", "--fd=4")
+		h := helper.New(argsWt, "crash-test-dummy", argFStatus)
 		ready := make(chan error, 1)
 
 		stdout, stderr := new(strings.Builder), new(strings.Builder)
@@ -66,6 +76,7 @@ func TestHelper_StartNotify_Close_Wait(t *testing.T) {
 			}
 		})
 
+		t.Log("starting helper stub")
 		if err := h.StartNotify(ready); err != nil {
 			t.Errorf("StartNotify(%v) error = %v",
 				ready,
@@ -83,17 +94,31 @@ func TestHelper_StartNotify_Close_Wait(t *testing.T) {
 			}
 		})
 
-		if err := <-ready; err != nil {
+		t.Log("waiting on status channel with timeout")
-			t.Errorf("StartNotify(%v) latent error = %v",
+		select {
-				ready,
+		case <-time.NewTimer(5 * time.Second).C:
-				err)
+			t.Errorf("never got a ready response")
+			t.Errorf("stdout:\n%s", stdout.String())
+			t.Errorf("stderr:\n%s", stderr.String())
+			if err := h.Cmd.Process.Kill(); err != nil {
+				panic(err.Error())
+			}
+			return
+		case err := <-ready:
+			if err != nil {
+				t.Errorf("StartNotify(%v) latent error = %v",
+					ready,
+					err)
+			}
 		}
 
+		t.Log("closing status pipe")
 		if err := h.Close(); err != nil {
 			t.Errorf("Close() error = %v",
 				err)
 		}
 
+		t.Log("waiting on helper")
 		if err := h.Wait(); err != nil {
 			t.Errorf("Wait() err = %v stderr = %s",
 				err, stderr)
@@ -131,7 +156,7 @@ func TestHelper_Start_Close_Wait(t *testing.T) {
 	}
 
 	t.Run("start helper", func(t *testing.T) {
-		h := helper.New(wt, "crash-test-dummy", "--args=3")
+		h := helper.New(wt, "crash-test-dummy", argF)
 
 		stdout, stderr := new(strings.Builder), new(strings.Builder)
 		h.Stdout, h.Stderr = stdout, stderr
@@ -172,7 +197,7 @@ func TestHelper_Start_Close_Wait(t *testing.T) {
 func TestNew(t *testing.T) {
 	t.Run("valid new helper nil check", func(t *testing.T) {
 		swt, _ := helper.NewCheckedArgs(make([]string, 1))
-		if got := helper.New(swt, "fortify"); got == nil {
+		if got := helper.New(swt, "fortify", argF); got == nil {
 			t.Errorf("New(%q, %q) got nil",
 				swt, "fortify")
 			return
@@ -188,6 +213,6 @@ func TestNew(t *testing.T) {
 			}
 		}()
 
-		helper.New(nil, "fortify")
+		helper.New(nil, "fortify", argF)
 	})
 }

helper/pipe.go

@@ -0,0 +1,150 @@
+package helper
+
+import (
+	"errors"
+	"io"
+	"os"
+	"os/exec"
+)
+
+type pipes struct {
+	args io.WriterTo
+
+	statP [2]*os.File
+	argsP [2]*os.File
+
+	ready chan error
+
+	cmd *exec.Cmd
+}
+
+func (p *pipes) pipe() error {
+	if p.statP[0] != nil || p.statP[1] != nil ||
+		p.argsP[0] != nil || p.argsP[1] != nil {
+		panic("attempted to pipe twice")
+	}
+	if p.args == nil {
+		panic("attempted to pipe without args")
+	}
+
+	// create pipes
+	if pr, pw, err := os.Pipe(); err != nil {
+		return err
+	} else {
+		p.argsP[0], p.argsP[1] = pr, pw
+	}
+
+	// create status pipes if ready signal is requested
+	if p.ready != nil {
+		if pr, pw, err := os.Pipe(); err != nil {
+			return err
+		} else {
+			p.statP[0], p.statP[1] = pr, pw
+		}
+	}
+
+	return nil
+}
+
+// calls pipe to create pipes and sets them up as ExtraFiles, returning their fd
+func (p *pipes) prepareCmd(cmd *exec.Cmd) (int, int, error) {
+	if err := p.pipe(); err != nil {
+		return -1, -1, err
+	}
+
+	// save a reference of cmd for future use
+	p.cmd = cmd
+
+	// ExtraFiles: If non-nil, entry i becomes file descriptor 3+i.
+	argsFd := 3 + len(cmd.ExtraFiles)
+	cmd.ExtraFiles = append(cmd.ExtraFiles, p.argsP[0])
+
+	if p.ready != nil {
+		cmd.ExtraFiles = append(cmd.ExtraFiles, p.statP[1])
+		return argsFd, argsFd + 1, nil
+	} else {
+		return argsFd, -1, nil
+	}
+}
+
+func (p *pipes) readyWriteArgs() error {
+	statsP, argsP := p.statP[0], p.argsP[1]
+
+	// write arguments and close args pipe
+	if _, err := p.args.WriteTo(argsP); err != nil {
+		if err1 := p.cmd.Process.Kill(); err1 != nil {
+			// should be unreachable
+			panic(err1.Error())
+		}
+		return err
+	} else {
+		if err = argsP.Close(); err != nil {
+			if err1 := p.cmd.Process.Kill(); err1 != nil {
+				// should be unreachable
+				panic(err1.Error())
+			}
+			return err
+		}
+	}
+
+	if p.ready != nil {
+		// monitor stat pipe
+		go func() {
+			n, err := statsP.Read(make([]byte, 1))
+			switch n {
+			case -1:
+				if err1 := p.cmd.Process.Kill(); err1 != nil {
+					// should be unreachable
+					panic(err1.Error())
+				}
+				// ensure error is not nil
+				if err == nil {
+					err = ErrStatusFault
+				}
+				p.ready <- err
+			case 0:
+				// ensure error is not nil
+				if err == nil {
+					err = ErrStatusRead
+				}
+				p.ready <- err
+			case 1:
+				p.ready <- nil
+			default:
+				panic("unreachable") // unexpected read count
+			}
+		}()
+	}
+
+	return nil
+}
+
+func (p *pipes) mustClosePipes() {
+	if err := p.argsP[0].Close(); err != nil && !errors.Is(err, os.ErrClosed) {
+		// unreachable
+		panic(err.Error())
+	}
+	if err := p.argsP[1].Close(); err != nil && !errors.Is(err, os.ErrClosed) {
+		// unreachable
+		panic(err.Error())
+	}
+
+	if p.ready != nil {
+		if err := p.statP[0].Close(); err != nil && !errors.Is(err, os.ErrClosed) {
+			// unreachable
+			panic(err.Error())
+		}
+		if err := p.statP[1].Close(); err != nil && !errors.Is(err, os.ErrClosed) {
+			// unreachable
+			panic(err.Error())
+		}
+	}
+}
+
+func (p *pipes) closeStatus() error {
+	if p.ready == nil {
+		panic("attempted to close helper with no status pipe")
+	}
+
+	return p.statP[0].Close()
+}

helper/stub.go

@@ -1,6 +1,7 @@
 package helper
 
 import (
+	"flag"
 	"io"
 	"os"
 	"os/exec"
@@ -17,9 +18,17 @@ func InternalChildStub() {
 		return
 	}
 
+	argsFD := flag.Int("args", -1, "")
+	statFD := flag.Int("fd", -1, "")
+	_ = flag.CommandLine.Parse(os.Args[4:])
+
 	// simulate args pipe behaviour
 	func() {
-		f := os.NewFile(3, "|0")
+		if *argsFD == -1 {
+			panic("attempted to start helper without passing args pipe fd")
+		}
+
+		f := os.NewFile(uintptr(*argsFD), "|0")
 		if f == nil {
 			panic("attempted to start helper without args pipe")
 		}
@@ -33,9 +42,13 @@ func InternalChildStub() {
 
 	// simulate status pipe behaviour
 	if os.Getenv(FortifyStatus) == "1" {
+		if *statFD == -1 {
+			panic("attempted to start helper with status reporting without passing status pipe fd")
+		}
+
 		wait = make(chan struct{})
 		go func() {
-			f := os.NewFile(4, "|1")
+			f := os.NewFile(uintptr(*statFD), "|1")
 			if f == nil {
 				panic("attempted to start with status reporting without status pipe")
 			}