app: defer system.I revert

Just returning an error after a successful call of commit will leave garbage behind with no way for the caller to clean them. This change ensures revert is always called after successful commit with at least per-process state enabled.

Signed-off-by: Ophestra <cat@gensokyo.uk>

fst/app.go

@@ -1,6 +1,9 @@
 package fst
 
-import "context"
+import (
+	"context"
+	"time"
+)
 
 type App interface {
 	// ID returns a copy of App's unique ID.
@@ -14,10 +17,15 @@ type App interface {
 
 // RunState stores the outcome of a call to [App.Run].
 type RunState struct {
-	// Start is true if fsu is successfully started.
-	Start bool
+	// Time is the exact point in time where the process was created.
+	// Location must be set to UTC.
+	//
+	// Time is nil if no process was ever created.
+	Time *time.Time
 	// ExitCode is the value returned by shim.
 	ExitCode int
+	// RevertErr is stored by the deferred revert call.
+	RevertErr error
 	// WaitErr is error returned by the underlying wait syscall.
 	WaitErr error
 }

internal/app/app.go

@@ -5,7 +5,6 @@ import (
 	"sync"
 
 	"git.gensokyo.uk/security/fortify/fst"
-	"git.gensokyo.uk/security/fortify/internal/app/shim"
 	"git.gensokyo.uk/security/fortify/internal/fmsg"
 	"git.gensokyo.uk/security/fortify/internal/sys"
 )
@@ -22,15 +21,11 @@ func New(os sys.State) (fst.App, error) {
 }
 
 type app struct {
-	// application unique identifier
 	id  *stringPair[fst.ID]
-	// operating system interface
 	sys sys.State
-	// shim process manager
-	shim *shim.Shim
 
-	mu sync.RWMutex
 	*appSeal
+	mu sync.RWMutex
 }
 
 func (a *app) ID() fst.ID { return a.id.unwrap() }
@@ -43,10 +38,6 @@ func (a *app) String() string {
 	a.mu.RLock()
 	defer a.mu.RUnlock()
 
-	if a.shim != nil {
-		return a.shim.String()
-	}
-
 	if a.appSeal != nil {
 		if a.appSeal.user.uid == nil {
 			return fmt.Sprintf("(sealed app %s with invalid uid)", a.id)

internal/app/process.go

@@ -28,7 +28,10 @@ func (a *app) Run(ctx context.Context, rs *fst.RunState) error {
 		panic("attempted to pass nil state to run")
 	}
 
-	// resolve exec paths
+	/*
+		resolve exec paths
+	*/
+
 	shimExec := [2]string{helper.BubblewrapName}
 	if len(a.appSeal.command) > 0 {
 		shimExec[1] = a.appSeal.command[0]
@@ -47,131 +50,41 @@ func (a *app) Run(ctx context.Context, rs *fst.RunState) error {
 		}
 	}
 
-	// startup will go ahead, commit system setup
+	/*
+		prepare/revert os state
+	*/
+
 	if err := a.appSeal.sys.Commit(ctx); err != nil {
 		return err
 	}
-	a.appSeal.needRevert = true
+	store := state.NewMulti(a.sys.Paths().RunDirPath)
+	deferredStoreFunc := func(c state.Cursor) error { return nil }
+	defer func() {
+		var revertErr error
+		storeErr := new(StateStoreError)
+		storeErr.Inner, storeErr.DoErr = store.Do(a.appSeal.user.aid.unwrap(), func(c state.Cursor) {
+			revertErr = func() error {
+				storeErr.InnerErr = deferredStoreFunc(c)
 
-	// start shim via manager
-	a.shim = new(shim.Shim)
-	waitErr := make(chan error, 1)
-	if startTime, err := a.shim.Start(
-		a.appSeal.user.aid.String(),
-		a.appSeal.user.supp,
-		a.appSeal.bwrapSync,
-	); err != nil {
-		return err
-	} else {
-		// shim process created
-		rs.Start = true
+				/*
+					revert app setup transaction
+				*/
 
-		shimSetupCtx, shimSetupCancel := context.WithDeadline(ctx, time.Now().Add(shimSetupTimeout))
-		defer shimSetupCancel()
-
-		// start waiting for shim
-		go func() {
-			waitErr <- a.shim.Unwrap().Wait()
-			// cancel shim setup in case shim died before receiving payload
-			shimSetupCancel()
-		}()
-
-		// send payload
-		if err = a.shim.Serve(shimSetupCtx, &shim.Payload{
-			Argv:  a.appSeal.command,
-			Exec:  shimExec,
-			Bwrap: a.appSeal.container,
-			Home:  a.appSeal.user.data,
-
-			Verbose: fmsg.Load(),
-		}); err != nil {
-			return err
-		}
-
-		// shim accepted setup payload, create process state
-		sd := state.State{
-			ID:   a.id.unwrap(),
-			PID:  a.shim.Unwrap().Process.Pid,
-			Time: *startTime,
-		}
-
-		// register process state
-		var err0 = new(StateStoreError)
-		err0.Inner, err0.DoErr = a.appSeal.store.Do(a.appSeal.user.aid.unwrap(), func(c state.Cursor) {
-			err0.InnerErr = c.Save(&sd, a.appSeal.ct)
-		})
-		a.appSeal.stateInStore = true
-		if err = err0.equiv("cannot save process state:"); err != nil {
-			return err
-		}
-	}
-
-	select {
-	// wait for process and resolve exit code
-	case err := <-waitErr:
-		if err != nil {
-			var exitError *exec.ExitError
-			if !errors.As(err, &exitError) {
-				// should be unreachable
-				rs.WaitErr = err
-			}
-
-			// store non-zero return code
-			rs.ExitCode = exitError.ExitCode()
-		} else {
-			rs.ExitCode = a.shim.Unwrap().ProcessState.ExitCode()
-		}
-		if fmsg.Load() {
-			fmsg.Verbosef("process %d exited with exit code %d", a.shim.Unwrap().Process.Pid, rs.ExitCode)
-		}
-
-	// this is reached when a fault makes an already running shim impossible to continue execution
-	// however a kill signal could not be delivered (should actually always happen like that since fsu)
-	// the effects of this is similar to the alternative exit path and ensures shim death
-	case err := <-a.shim.WaitFallback():
-		rs.ExitCode = 255
-		log.Printf("cannot terminate shim on faulted setup: %v", err)
-
-	// alternative exit path relying on shim behaviour on monitor process exit
-	case <-ctx.Done():
-		fmsg.Verbose("alternative exit path selected")
-	}
-
-	// child process exited, resume output
-	fmsg.Resume()
-
-	// print queued up dbus messages
-	if a.appSeal.dbusMsg != nil {
-		a.appSeal.dbusMsg()
-	}
-
-	// update store and revert app setup transaction
-	e := new(StateStoreError)
-	e.Inner, e.DoErr = a.appSeal.store.Do(a.appSeal.user.aid.unwrap(), func(b state.Cursor) {
-		e.InnerErr = func() error {
-			// destroy defunct state entry
-			if cmd := a.shim.Unwrap(); cmd != nil && a.appSeal.stateInStore {
-				if err := b.Destroy(a.id.unwrap()); err != nil {
-					return err
-				}
-			}
-
-			// enablements of remaining launchers
 				rt, ec := new(system.Enablements), new(system.Criteria)
 				ec.Enablements = new(system.Enablements)
 				ec.Set(system.Process)
-			if states, err := b.Load(); err != nil {
-				return err
+				if states, err := c.Load(); err != nil {
+					// revert per-process state here to limit damage
+					return errors.Join(err, a.appSeal.sys.Revert(ec))
 				} else {
 					if l := len(states); l == 0 {
-					// cleanup globals as the final launcher
 						fmsg.Verbose("no other launchers active, will clean up globals")
 						ec.Set(system.User)
 					} else {
 						fmsg.Verbosef("found %d active launchers, cleaning up without globals", l)
 					}
 
-				// accumulate capabilities of other launchers
+					// accumulate enablements of remaining launchers
 					for i, s := range states {
 						if s.Config != nil {
 							*rt |= s.Config.Confinement.Enablements
@@ -194,36 +107,128 @@ func (a *app) Run(ctx context.Context, rs *fst.RunState) error {
 						}
 					}
 					if len(labels) > 0 {
-					fmsg.Verbose("reverting operations labelled", strings.Join(labels, ", "))
+						fmsg.Verbose("reverting operations type", strings.Join(labels, ", "))
 					}
 				}
 
-			if a.appSeal.needRevert {
-				if err := a.appSeal.sys.Revert(ec); err != nil {
-					return err.(RevertCompoundError)
+				err := a.appSeal.sys.Revert(ec)
+				if err != nil {
+					err = err.(RevertCompoundError)
 				}
-			}
-
-			return nil
+				return err
 			}()
 		})
+		storeErr.Err = errors.Join(revertErr, store.Close())
+		rs.RevertErr = storeErr.equiv("error returned during cleanup:")
+	}()
 
-	e.Err = a.appSeal.store.Close()
-	return e.equiv("error returned during cleanup:", e)
+	/*
+		shim process lifecycle
+	*/
+
+	waitErr := make(chan error, 1)
+	cmd := new(shim.Shim)
+	if startTime, err := cmd.Start(
+		a.appSeal.user.aid.String(),
+		a.appSeal.user.supp,
+		a.appSeal.bwrapSync,
+	); err != nil {
+		return err
+	} else {
+		// whether/when the fsu process was created
+		rs.Time = startTime
+	}
+
+	shimSetupCtx, shimSetupCancel := context.WithDeadline(ctx, time.Now().Add(shimSetupTimeout))
+	defer shimSetupCancel()
+
+	go func() {
+		waitErr <- cmd.Unwrap().Wait()
+		// cancel shim setup in case shim died before receiving payload
+		shimSetupCancel()
+	}()
+
+	if err := cmd.Serve(shimSetupCtx, &shim.Payload{
+		Argv:  a.appSeal.command,
+		Exec:  shimExec,
+		Bwrap: a.appSeal.container,
+		Home:  a.appSeal.user.data,
+
+		Verbose: fmsg.Load(),
+	}); err != nil {
+		return err
+	}
+
+	// shim accepted setup payload, create process state
+	sd := state.State{
+		ID:   a.id.unwrap(),
+		PID:  cmd.Unwrap().Process.Pid,
+		Time: *rs.Time,
+	}
+	var earlyStoreErr = new(StateStoreError) // returned after blocking on waitErr
+	earlyStoreErr.Inner, earlyStoreErr.DoErr = store.Do(a.appSeal.user.aid.unwrap(), func(c state.Cursor) { earlyStoreErr.InnerErr = c.Save(&sd, a.appSeal.ct) })
+	// destroy defunct state entry
+	deferredStoreFunc = func(c state.Cursor) error { return c.Destroy(a.id.unwrap()) }
+
+	select {
+	case err := <-waitErr: // block until fsu/shim returns
+		if err != nil {
+			var exitError *exec.ExitError
+			if !errors.As(err, &exitError) {
+				// should be unreachable
+				rs.WaitErr = err
+			}
+
+			// store non-zero return code
+			rs.ExitCode = exitError.ExitCode()
+		} else {
+			rs.ExitCode = cmd.Unwrap().ProcessState.ExitCode()
+		}
+		if fmsg.Load() {
+			fmsg.Verbosef("process %d exited with exit code %d", cmd.Unwrap().Process.Pid, rs.ExitCode)
+		}
+
+	// this is reached when a fault makes an already running shim impossible to continue execution
+	// however a kill signal could not be delivered (should actually always happen like that since fsu)
+	// the effects of this is similar to the alternative exit path and ensures shim death
+	case err := <-cmd.WaitFallback():
+		rs.ExitCode = 255
+		log.Printf("cannot terminate shim on faulted setup: %v", err)
+
+	// alternative exit path relying on shim behaviour on monitor process exit
+	case <-ctx.Done():
+		fmsg.Verbose("alternative exit path selected")
+	}
+
+	fmsg.Resume()
+	if a.appSeal.dbusMsg != nil {
+		// dump dbus message buffer
+		a.appSeal.dbusMsg()
+	}
+
+	return earlyStoreErr.equiv("cannot save process state:")
 }
 
 // StateStoreError is returned for a failed state save
 type StateStoreError struct {
 	// whether inner function was called
 	Inner bool
-	// error returned by state.Store Do method
+	// returned by the Do method of [state.Store]
 	DoErr error
-	// error returned by state.Backend Save method
+	// returned by the Save/Destroy method of [state.Cursor]
 	InnerErr error
-	// any other errors needing to be tracked
+	// stores an arbitrary error
 	Err error
 }
 
+// save saves exactly one arbitrary error in [StateStoreError].
+func (e *StateStoreError) save(err error) {
+	if err == nil || e.Err != nil {
+		panic("invalid call to save")
+	}
+	e.Err = err
+}
+
 func (e *StateStoreError) equiv(a ...any) error {
 	if e.Inner && e.DoErr == nil && e.InnerErr == nil && e.Err == nil {
 		return nil
@@ -245,7 +250,8 @@ func (e *StateStoreError) Error() string {
 		return e.Err.Error()
 	}
 
-	return "(nil)"
+	// equiv nullifies e for values where this is reached
+	panic("unreachable")
 }
 
 func (e *StateStoreError) Unwrap() (errs []error) {
@@ -262,6 +268,8 @@ func (e *StateStoreError) Unwrap() (errs []error) {
 	return
 }
 
+// A RevertCompoundError encapsulates errors returned by
+// the Revert method of [system.I].
 type RevertCompoundError interface {
 	Error() string
 	Unwrap() []error

internal/app/seal.go

@@ -18,7 +18,6 @@ import (
 	"git.gensokyo.uk/security/fortify/helper/bwrap"
 	"git.gensokyo.uk/security/fortify/internal"
 	"git.gensokyo.uk/security/fortify/internal/fmsg"
-	"git.gensokyo.uk/security/fortify/internal/state"
 	"git.gensokyo.uk/security/fortify/internal/sys"
 	"git.gensokyo.uk/security/fortify/system"
 	"git.gensokyo.uk/security/fortify/wl"
@@ -63,17 +62,11 @@ type appSeal struct {
 	// passed through from [fst.Config]
 	command []string
 
-	// state instance initialised during seal; used during process lifecycle events
-	store state.Store
 	// initial [fst.Config] gob stream for state data;
 	// this is prepared ahead of time as config is mutated during seal creation
 	ct io.WriterTo
 	// dump dbus proxy message buffer
 	dbusMsg func()
-	// whether [system.I] was committed; used during process lifecycle events
-	needRevert bool
-	// whether state was inserted into [state.Store]; used during process lifecycle events
-	stateInStore bool
 
 	user      appUser
 	sys       *system.I
@@ -230,7 +223,6 @@ func (seal *appSeal) finalise(sys sys.State, config *fst.Config, id string) erro
 	*/
 
 	sc := sys.Paths()
-	seal.store = state.NewMulti(sc.RunDirPath)
 	seal.sys = system.New(seal.user.uid.unwrap())
 	seal.sys.IsVerbose = fmsg.Load
 	seal.sys.Verbose = fmsg.Verbose

main.go

@@ -334,7 +334,7 @@ func runApp(config *fst.Config) {
 		fmsg.PrintBaseError(err, "cannot seal app:")
 		internal.Exit(1)
 	} else if err = a.Run(ctx, rs); err != nil {
-		if !rs.Start {
+		if rs.Time == nil {
 			fmsg.PrintBaseError(err, "cannot start app:")
 		} else {
 			logWaitError(err)
@@ -344,6 +344,12 @@ func runApp(config *fst.Config) {
 			rs.ExitCode = 126
 		}
 	}
+	if rs.RevertErr != nil {
+		fmsg.PrintBaseError(rs.RevertErr, "generic error returned during cleanup:")
+		if rs.ExitCode == 0 {
+			rs.ExitCode = 128
+		}
+	}
 	if rs.WaitErr != nil {
 		log.Println("inner wait failed:", rs.WaitErr)
 	}