diff --git a/dbus/dbus_test.go b/dbus/dbus_test.go index 28f3c24..098a217 100644 --- a/dbus/dbus_test.go +++ b/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) diff --git a/dbus/run.go b/dbus/run.go index 0b8267f..da0bbc8 100644 --- a/dbus/run.go +++ b/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. - "--args=3", - "--fd=4", + func(argsFD, statFD int) []string { + if statFD == -1 { + 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{} diff --git a/helper/args.go b/helper/args.go index b06cc2e..c841fd3 100644 --- a/helper/args.go +++ b/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() } diff --git a/helper/helper.go b/helper/helper.go index af6887c..ed4e457 100644 --- a/helper/helper.go +++ b/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 - argsP [2]*os.File - - 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 + argF func(argsFD, statFD int) []string + *exec.Cmd 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 - if pr, pw, err := os.Pipe(); err != nil { + h.p.ready = ready + 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 } - - 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()) - } + if err := h.p.readyWriteArgs(); err != nil { 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 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()) - } - } - }() - + defer h.p.mustClosePipes() return h.Cmd.Wait() } func (h *Helper) Close() error { - if h.ready == nil { - panic("attempted to close helper with no status pipe") - } - - return h.statP[0].Close() + return h.p.closeStatus() } 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)} } diff --git a/helper/helper_test.go b/helper/helper_test.go index e83b53b..835f358 100644 --- a/helper/helper_test.go +++ b/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.Errorf("StartNotify(%v) latent error = %v", - ready, - err) + 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) @@ -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) }) } diff --git a/helper/pipe.go b/helper/pipe.go new file mode 100644 index 0000000..e601328 --- /dev/null +++ b/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() +} diff --git a/helper/stub.go b/helper/stub.go index 6a59173..b6b5d72 100644 --- a/helper/stub.go +++ b/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") }