From 85407dd3c09ffbc4c102baa90249ded685badd12 Mon Sep 17 00:00:00 2001 From: Ophestra Umiker Date: Mon, 7 Oct 2024 15:37:52 +0900 Subject: helper: helper.Helper interface For upcoming bwrap implementation of helper.Helper Signed-off-by: Ophestra Umiker --- dbus/dbus.go | 4 +- dbus/run.go | 7 +- helper/direct.go | 93 +++++++++++++++++++++++ helper/direct_test.go | 206 ++++++++++++++++++++++++++++++++++++++++++++++++++ helper/helper.go | 90 ++++------------------ helper/helper_test.go | 204 ------------------------------------------------- 6 files changed, 318 insertions(+), 286 deletions(-) create mode 100644 helper/direct.go create mode 100644 helper/direct_test.go delete mode 100644 helper/helper_test.go diff --git a/dbus/dbus.go b/dbus/dbus.go index 7a51d98c..b993d52d 100644 --- a/dbus/dbus.go +++ b/dbus/dbus.go @@ -12,7 +12,7 @@ import ( // Proxy holds references to a xdg-dbus-proxy process, and should never be copied. // Once sealed, configuration changes will no longer be possible and attempting to do so will result in a panic. type Proxy struct { - helper *helper.Helper + helper helper.Helper path string session [2]string @@ -35,7 +35,7 @@ func (p *Proxy) String() string { defer p.lock.RUnlock() if p.helper != nil { - return p.helper.String() + return p.helper.Unwrap().String() } if p.seal != nil { diff --git a/dbus/run.go b/dbus/run.go index da0bbc8d..5e489ba3 100644 --- a/dbus/run.go +++ b/dbus/run.go @@ -27,12 +27,13 @@ func (p *Proxy) Start(ready chan error, output io.Writer) error { } }, ) + cmd := h.Unwrap() // xdg-dbus-proxy does not need to inherit the environment - h.Env = []string{} + cmd.Env = []string{} if output != nil { - h.Stdout = output - h.Stderr = output + cmd.Stdout = output + cmd.Stderr = output } if err := h.StartNotify(ready); err != nil { return err diff --git a/helper/direct.go b/helper/direct.go new file mode 100644 index 00000000..24f9e75d --- /dev/null +++ b/helper/direct.go @@ -0,0 +1,93 @@ +package helper + +import ( + "errors" + "io" + "os/exec" + "sync" +) + +// direct wraps *exec.Cmd and manages status and args fd. +// Args is always 3 and status if set is always 4. +type direct struct { + // helper pipes + // cannot be nil + p *pipes + + // returns an array of arguments passed directly + // to the helper process + argF func(argsFD, statFD int) []string + + lock sync.RWMutex + *exec.Cmd +} + +func (h *direct) StartNotify(ready chan error) error { + h.lock.Lock() + defer h.lock.Unlock() + + // Check for doubled Start calls before we defer failure cleanup. If the prior + // call to Start succeeded, we don't want to spuriously close its pipes. + if h.Cmd.Process != nil { + return errors.New("exec: already started") + } + + h.p.ready = ready + if argsFD, statFD, err := h.p.prepareCmd(h.Cmd); err != nil { + return err + } else { + h.Cmd.Args = append(h.Cmd.Args, h.argF(argsFD, statFD)...) + } + + if ready != nil { + h.Cmd.Env = append(h.Cmd.Env, FortifyHelper+"=1", FortifyStatus+"=1") + } else { + h.Cmd.Env = append(h.Cmd.Env, FortifyHelper+"=1", FortifyStatus+"=0") + } + + if err := h.Cmd.Start(); err != nil { + return err + } + if err := h.p.readyWriteArgs(); err != nil { + return err + } + + return nil +} + +func (h *direct) Wait() error { + h.lock.RLock() + defer h.lock.RUnlock() + + if h.Cmd.Process == nil { + return errors.New("exec: not started") + } + if h.Cmd.ProcessState != nil { + return errors.New("exec: Wait was already called") + } + + defer h.p.mustClosePipes() + return h.Cmd.Wait() +} + +func (h *direct) Close() error { + return h.p.closeStatus() +} + +func (h *direct) Start() error { + return h.StartNotify(nil) +} + +func (h *direct) Unwrap() *exec.Cmd { + return h.Cmd +} + +// New initialises a new direct 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 &direct{p: &pipes{args: wt}, argF: argF, Cmd: execCommand(name)} +} diff --git a/helper/direct_test.go b/helper/direct_test.go new file mode 100644 index 00000000..12a7b526 --- /dev/null +++ b/helper/direct_test.go @@ -0,0 +1,206 @@ +package helper_test + +import ( + "errors" + "io" + "os" + "strconv" + "strings" + "testing" + "time" + + "git.ophivana.moe/cat/fortify/helper" +) + +var ( + want = []string{ + "unix:path=/run/dbus/system_bus_socket", + "/tmp/fortify.1971/12622d846cc3fe7b4c10359d01f0eb47/system_bus_socket", + "--filter", + "--talk=org.bluez", + "--talk=org.freedesktop.Avahi", + "--talk=org.freedesktop.UPower", + } + + wantPayload = strings.Join(want, "\x00") + "\x00" + argsWt = helper.MustNewCheckedArgs(want) +) + +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) + + t.Run("start non-existent helper path", func(t *testing.T) { + h := helper.New(argsWt, "/nonexistent", argF) + + if err := h.Start(); !errors.Is(err, os.ErrNotExist) { + t.Errorf("Start() error = %v, wantErr %v", + err, os.ErrNotExist) + } + }) + + t.Run("start helper with status channel", func(t *testing.T) { + h := helper.New(argsWt, "crash-test-dummy", argFStatus) + ready := make(chan error, 1) + cmd := h.Unwrap() + + stdout, stderr := new(strings.Builder), new(strings.Builder) + cmd.Stdout, cmd.Stderr = stdout, stderr + + t.Run("wait not yet started helper", func(t *testing.T) { + wantErr := "exec: not started" + if err := h.Wait(); err != nil && err.Error() != wantErr { + t.Errorf("Wait(%v) error = %v, wantErr %v", + ready, + err, wantErr) + return + } + }) + + t.Log("starting helper stub") + if err := h.StartNotify(ready); err != nil { + t.Errorf("StartNotify(%v) error = %v", + ready, + err) + return + } + + t.Run("start already started helper", func(t *testing.T) { + wantErr := "exec: already started" + if err := h.StartNotify(ready); err != nil && err.Error() != wantErr { + t.Errorf("StartNotify(%v) error = %v, wantErr %v", + ready, + err, wantErr) + return + } + }) + + t.Log("waiting on status channel with timeout") + select { + case <-time.NewTimer(5 * time.Second).C: + t.Errorf("never got a ready response") + t.Errorf("stdout:\n%s", stdout.String()) + t.Errorf("stderr:\n%s", stderr.String()) + if err := 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) + } + + t.Run("wait already finalised helper", func(t *testing.T) { + wantErr := "exec: Wait was already called" + if err := h.Wait(); err != nil && err.Error() != wantErr { + t.Errorf("Wait(%v) error = %v, wantErr %v", + ready, + err, wantErr) + return + } + }) + + if got := stdout.String(); !strings.HasPrefix(got, wantPayload) { + t.Errorf("StartNotify(%v) stdout = %v, want %v", + ready, + got, wantPayload) + } + }) +} +func TestHelper_Start_Close_Wait(t *testing.T) { + helper.InternalReplaceExecCommand(t) + + var wt io.WriterTo + if a, err := helper.NewCheckedArgs(want); err != nil { + t.Errorf("NewCheckedArgs(%q) error = %v", + want, + err) + return + } else { + wt = a + } + + t.Run("start helper", func(t *testing.T) { + h := helper.New(wt, "crash-test-dummy", argF) + cmd := h.Unwrap() + + stdout, stderr := new(strings.Builder), new(strings.Builder) + cmd.Stdout, cmd.Stderr = stdout, stderr + + if err := h.Start(); err != nil { + t.Errorf("Start() error = %v", + err) + return + } + + t.Run("close helper without status pipe", func(t *testing.T) { + defer func() { + wantPanic := "attempted to close helper with no status pipe" + if r := recover(); r != wantPanic { + t.Errorf("Close() panic = %v, wantPanic %v", + r, wantPanic) + } + }() + if err := h.Close(); err != nil { + t.Errorf("Close() error = %v", + err) + return + } + }) + + if err := h.Wait(); err != nil { + t.Errorf("Wait() err = %v stderr = %s", + err, stderr) + } + + if got := stdout.String(); !strings.HasPrefix(got, wantPayload) { + t.Errorf("Start() stdout = %v, want %v", + got, wantPayload) + } + }) +} + +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", argF); got == nil { + t.Errorf("New(%q, %q) got nil", + swt, "fortify") + return + } + }) + + t.Run("invalid new helper panic", func(t *testing.T) { + defer func() { + want := "attempted to create helper with invalid argument writer" + if r := recover(); r != want { + t.Errorf("New: panic = %q, want %q", + r, want) + } + }() + + helper.New(nil, "fortify", argF) + }) +} diff --git a/helper/helper.go b/helper/helper.go index ed4e4577..a634d595 100644 --- a/helper/helper.go +++ b/helper/helper.go @@ -5,9 +5,7 @@ package helper import ( "errors" - "io" "os/exec" - "sync" ) var ( @@ -22,81 +20,19 @@ const ( 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 { - p *pipes - - argF func(argsFD, statFD int) []string - *exec.Cmd - - lock sync.RWMutex -} - -func (h *Helper) StartNotify(ready chan error) error { - h.lock.Lock() - defer h.lock.Unlock() - - // Check for doubled Start calls before we defer failure cleanup. If the prior - // call to Start succeeded, we don't want to spuriously close its pipes. - if h.Cmd.Process != nil { - return errors.New("exec: already started") - } - - h.p.ready = ready - if argsFD, statFD, err := h.p.prepareCmd(h.Cmd); err != nil { - return err - } else { - h.Cmd.Args = append(h.Cmd.Args, h.argF(argsFD, statFD)...) - } - - if ready != nil { - h.Cmd.Env = append(h.Cmd.Env, FortifyHelper+"=1", FortifyStatus+"=1") - } else { - h.Cmd.Env = append(h.Cmd.Env, FortifyHelper+"=1", FortifyStatus+"=0") - } - - if err := h.Cmd.Start(); err != nil { - return err - } - if err := h.p.readyWriteArgs(); err != nil { - return err - } - - return nil -} - -func (h *Helper) Wait() error { - h.lock.RLock() - defer h.lock.RUnlock() - - if h.Cmd.Process == nil { - return errors.New("exec: not started") - } - if h.Cmd.ProcessState != nil { - return errors.New("exec: Wait was already called") - } - - defer h.p.mustClosePipes() - return h.Cmd.Wait() -} - -func (h *Helper) Close() error { - return h.p.closeStatus() -} - -func (h *Helper) Start() error { - return h.StartNotify(nil) +type Helper interface { + // StartNotify starts the helper process. + // A status pipe is passed to the helper if ready is not nil. + StartNotify(ready chan error) error + // Start starts the helper process. + Start() error + // Close closes the status pipe. + // If helper is started without the status pipe, Close panics. + Close() error + // Wait calls wait on the child process and cleans up pipes. + Wait() error + // Unwrap returns the underlying exec.Cmd instance. + Unwrap() *exec.Cmd } var execCommand = exec.Command - -// 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{p: &pipes{args: wt}, argF: argF, Cmd: execCommand(name)} -} diff --git a/helper/helper_test.go b/helper/helper_test.go deleted file mode 100644 index 0bbc5695..00000000 --- a/helper/helper_test.go +++ /dev/null @@ -1,204 +0,0 @@ -package helper_test - -import ( - "errors" - "io" - "os" - "strconv" - "strings" - "testing" - "time" - - "git.ophivana.moe/cat/fortify/helper" -) - -var ( - want = []string{ - "unix:path=/run/dbus/system_bus_socket", - "/tmp/fortify.1971/12622d846cc3fe7b4c10359d01f0eb47/system_bus_socket", - "--filter", - "--talk=org.bluez", - "--talk=org.freedesktop.Avahi", - "--talk=org.freedesktop.UPower", - } - - wantPayload = strings.Join(want, "\x00") + "\x00" - argsWt = helper.MustNewCheckedArgs(want) -) - -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) - - t.Run("start non-existent helper path", func(t *testing.T) { - h := helper.New(argsWt, "/nonexistent", argF) - - if err := h.Start(); !errors.Is(err, os.ErrNotExist) { - t.Errorf("Start() error = %v, wantErr %v", - err, os.ErrNotExist) - } - }) - - t.Run("start helper with status channel", func(t *testing.T) { - h := helper.New(argsWt, "crash-test-dummy", argFStatus) - ready := make(chan error, 1) - - stdout, stderr := new(strings.Builder), new(strings.Builder) - h.Stdout, h.Stderr = stdout, stderr - - t.Run("wait not yet started helper", func(t *testing.T) { - wantErr := "exec: not started" - if err := h.Wait(); err != nil && err.Error() != wantErr { - t.Errorf("Wait(%v) error = %v, wantErr %v", - ready, - err, wantErr) - return - } - }) - - t.Log("starting helper stub") - if err := h.StartNotify(ready); err != nil { - t.Errorf("StartNotify(%v) error = %v", - ready, - err) - return - } - - t.Run("start already started helper", func(t *testing.T) { - wantErr := "exec: already started" - if err := h.StartNotify(ready); err != nil && err.Error() != wantErr { - t.Errorf("StartNotify(%v) error = %v, wantErr %v", - ready, - err, wantErr) - return - } - }) - - t.Log("waiting on status channel with timeout") - select { - case <-time.NewTimer(5 * time.Second).C: - 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) - } - - t.Run("wait already finalised helper", func(t *testing.T) { - wantErr := "exec: Wait was already called" - if err := h.Wait(); err != nil && err.Error() != wantErr { - t.Errorf("Wait(%v) error = %v, wantErr %v", - ready, - err, wantErr) - return - } - }) - - if got := stdout.String(); !strings.HasPrefix(got, wantPayload) { - t.Errorf("StartNotify(%v) stdout = %v, want %v", - ready, - got, wantPayload) - } - }) -} -func TestHelper_Start_Close_Wait(t *testing.T) { - helper.InternalReplaceExecCommand(t) - - var wt io.WriterTo - if a, err := helper.NewCheckedArgs(want); err != nil { - t.Errorf("NewCheckedArgs(%q) error = %v", - want, - err) - return - } else { - wt = a - } - - t.Run("start helper", func(t *testing.T) { - h := helper.New(wt, "crash-test-dummy", argF) - - stdout, stderr := new(strings.Builder), new(strings.Builder) - h.Stdout, h.Stderr = stdout, stderr - - if err := h.Start(); err != nil { - t.Errorf("Start() error = %v", - err) - return - } - - t.Run("close helper without status pipe", func(t *testing.T) { - defer func() { - wantPanic := "attempted to close helper with no status pipe" - if r := recover(); r != wantPanic { - t.Errorf("Close() panic = %v, wantPanic %v", - r, wantPanic) - } - }() - if err := h.Close(); err != nil { - t.Errorf("Close() error = %v", - err) - return - } - }) - - if err := h.Wait(); err != nil { - t.Errorf("Wait() err = %v stderr = %s", - err, stderr) - } - - if got := stdout.String(); !strings.HasPrefix(got, wantPayload) { - t.Errorf("Start() stdout = %v, want %v", - got, wantPayload) - } - }) -} - -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", argF); got == nil { - t.Errorf("New(%q, %q) got nil", - swt, "fortify") - return - } - }) - - t.Run("invalid new helper panic", func(t *testing.T) { - defer func() { - want := "attempted to create helper with invalid argument writer" - if r := recover(); r != want { - t.Errorf("New: panic = %q, want %q", - r, want) - } - }() - - helper.New(nil, "fortify", argF) - }) -} -- cgit v1.3.1