From 1a7b909f971dee8c215cf1877cd95a3d690730e1 Mon Sep 17 00:00:00 2001 From: Ophestra Date: Sun, 18 Oct 2026 20:33:03 +0900 Subject: internal/system: file-based big lock Simultaneous commits or reverts can race if they have overlapping targets. This big lock synchronises entry into the Commit and Revert critical sections across all cmd/hakurei processes. Signed-off-by: Ophestra --- internal/outcome/dispatcher_test.go | 4 +-- internal/outcome/finalise.go | 2 +- internal/outcome/run_test.go | 10 +++---- internal/outcome/spdbus_test.go | 4 +-- internal/system/dispatcher_test.go | 2 +- internal/system/system.go | 53 ++++++++++++++++++++++++++++--------- internal/system/system_test.go | 34 ++++++++++++------------ 7 files changed, 68 insertions(+), 41 deletions(-) (limited to 'internal') diff --git a/internal/outcome/dispatcher_test.go b/internal/outcome/dispatcher_test.go index 00771e56..3f4732b5 100644 --- a/internal/outcome/dispatcher_test.go +++ b/internal/outcome/dispatcher_test.go @@ -209,7 +209,7 @@ func checkOpBehaviour(t *testing.T, testCases []opBehaviourTestCase) { if err := s.populateLocal(k, k); err != nil { t.Fatalf("populateLocal: error = %v", err) } - stateSys := s.newSys(config, system.New(panicMsgContext{}, k, checkExpectUid)) + stateSys := s.newSys(config, system.New(panicMsgContext{}, k, checkExpectUid, nil)) if tc.pStateSys != nil { tc.pStateSys(stateSys) } @@ -279,7 +279,7 @@ func checkOpBehaviour(t *testing.T, testCases []opBehaviourTestCase) { } } -func newI() *system.I { return system.New(panicMsgContext{}, panicMsgContext{}, checkExpectUid) } +func newI() *system.I { return system.New(panicMsgContext{}, panicMsgContext{}, checkExpectUid, nil) } // simpleTestCase is a simple freeform test case utilising kstub. type simpleTestCase struct { diff --git a/internal/outcome/finalise.go b/internal/outcome/finalise.go index 4049b8d3..7e5faab6 100644 --- a/internal/outcome/finalise.go +++ b/internal/outcome/finalise.go @@ -74,7 +74,7 @@ func (k *outcome) finalise( return err } - sys := system.New(k.ctx, msg, s.uid.unwrap()) + sys := system.New(k.ctx, msg, s.uid.unwrap(), s.sc.SharePath.Append("lock")) if err := s.newSys(config, sys).toSystem(); err != nil { return err } diff --git a/internal/outcome/run_test.go b/internal/outcome/run_test.go index 81443c5d..d9a41f53 100644 --- a/internal/outcome/run_test.go +++ b/internal/outcome/run_test.go @@ -40,7 +40,7 @@ func TestOutcomeRun(t *testing.T) { wantSys *system.I wantParams *container.Params }{ - {"template", new(stubNixOS), hst.Template(), checkExpectInstanceId, system.New(panicMsgContext{}, message.New(nil), 10009). + {"template", new(stubNixOS), hst.Template(), checkExpectInstanceId, system.New(panicMsgContext{}, message.New(nil), 10009, nil). // spParamsOp Ensure(m("/tmp/hakurei.0"), 0711, false). @@ -198,7 +198,7 @@ func TestOutcomeRun(t *testing.T) { 0x96, 0xd7, 0xbc, 0x15, 0xbd, 0x01, 0x78, 0x0e, 0xb9, 0xa6, 0x07, 0xac, - }, system.New(t.Context(), msg, 10000). + }, system.New(t.Context(), msg, 10000, nil). Ensure(m("/tmp/hakurei.0"), 0711, false). Ensure(m("/tmp/hakurei.0/runtime"), 0711, false). Ensure(m("/tmp/hakurei.0/runtime/0"), 0700, true). @@ -322,7 +322,7 @@ func TestOutcomeRun(t *testing.T) { 0xb1, 0x75, 0x91, 0x17, 0x82, 0xd4, 0x13, 0x36, 0x9b, 0x64, 0xce, 0x7c, - }, system.New(t.Context(), msg, 10009). + }, system.New(t.Context(), msg, 10009, nil). Ensure(m("/tmp/hakurei.0"), 0711, false). Ensure(m("/tmp/hakurei.0/runtime"), 0711, false). Ensure(m("/tmp/hakurei.0/runtime/9"), 0700, true).UpdatePermType(system.User, m("/tmp/hakurei.0/runtime/9"), acl.Read|acl.Write|acl.Execute). @@ -473,7 +473,7 @@ func TestOutcomeRun(t *testing.T) { 0x66, 0xda, 0xbe, 0x57, 0x4c, 0xf0, 0x73, 0xbd, 0xb4, 0x6e, 0xb5, 0xc1, - }, system.New(t.Context(), msg, 10001). + }, system.New(t.Context(), msg, 10001, nil). Ensure(m("/tmp/hakurei.0"), 0711, false). Ensure(m("/tmp/hakurei.0/runtime"), 0711, false). Ensure(m("/tmp/hakurei.0/runtime/1"), 0700, true).UpdatePermType(system.User, m("/tmp/hakurei.0/runtime/1"), acl.Read|acl.Write|acl.Execute). @@ -577,7 +577,7 @@ func TestOutcomeRun(t *testing.T) { t.Fatalf("populateLocal: error = %#v", err) } - gotSys = system.New(t.Context(), msg, sPriv.uid.unwrap()) + gotSys = system.New(t.Context(), msg, sPriv.uid.unwrap(), nil) if err := sPriv.newSys(tc.config, gotSys).toSystem(); err != nil { t.Fatalf("toSystem: error = %#v", err) } diff --git a/internal/outcome/spdbus_test.go b/internal/outcome/spdbus_test.go index 4720b5c8..3b9f07f7 100644 --- a/internal/outcome/spdbus_test.go +++ b/internal/outcome/spdbus_test.go @@ -80,7 +80,7 @@ func TestSpDBusOp(t *testing.T) { "--broadcast=org.freedesktop.portal.*=@/org/freedesktop/portal/*", )}}, nil, nil), }, func() *system.I { - sys := system.New(panicMsgContext{}, message.New(nil), checkExpectUid) + sys := system.New(panicMsgContext{}, message.New(nil), checkExpectUid, nil) sys.Ephemeral(system.Process, m(wantInstancePrefix), 0711, false) if err := sys.ProxyDBus( dbus.NewConfig(config.ID, true, true), nil, @@ -162,7 +162,7 @@ func TestSpDBusOp(t *testing.T) { "--talk=org.freedesktop.UPower", )}}, nil, nil), }, func() *system.I { - sys := system.New(panicMsgContext{}, message.New(nil), checkExpectUid) + sys := system.New(panicMsgContext{}, message.New(nil), checkExpectUid, nil) sys.Ephemeral(system.Process, m(wantInstancePrefix), 0711, false) if err := sys.ProxyDBus( config.SessionBus, config.SystemBus, diff --git a/internal/system/dispatcher_test.go b/internal/system/dispatcher_test.go index dd42b965..bfb8df72 100644 --- a/internal/system/dispatcher_test.go +++ b/internal/system/dispatcher_test.go @@ -208,7 +208,7 @@ func checkOpMeta(t *testing.T, testCases []opMetaTestCase) { // InternalNew initialises [I] with a stub syscallDispatcher. func InternalNew(t *testing.T, want stub.Expect, uid int) (*I, *stub.Stub[syscallDispatcher]) { k := &kstub{stub.New(t, func(s *stub.Stub[syscallDispatcher]) syscallDispatcher { return &kstub{s} }, want)} - sys := New(t.Context(), k, uid) + sys := New(t.Context(), k, uid, nil) sys.syscallDispatcher = k return sys, k.Stub } diff --git a/internal/system/system.go b/internal/system/system.go index 1462f818..340e23ec 100644 --- a/internal/system/system.go +++ b/internal/system/system.go @@ -6,7 +6,9 @@ import ( "errors" "strings" + "hakurei.app/check" "hakurei.app/hst" + "hakurei.app/internal/lockedfile" "hakurei.app/message" ) @@ -67,14 +69,6 @@ func TypeString(e hst.Enablements) string { } } -// New returns the address of a new [I] targeting uid. -func New(ctx context.Context, msg message.Msg, uid int) (sys *I) { - if ctx == nil || msg == nil || uid < 0 { - panic("invalid call to New") - } - return &I{ctx: ctx, msg: msg, uid: uid, syscallDispatcher: direct{}} -} - // An I provides deferred operating system interaction. [I] must not be copied. // Methods of [I] must not be used concurrently. type I struct { @@ -84,15 +78,32 @@ type I struct { ops []Op ctx context.Context - // the behaviour of Commit is only defined for up to one call - committed bool - // the behaviour of Revert is only defined for up to one call - reverted bool + // The behaviour of Commit and Revert is only defined for at most one call. + committed, reverted bool + // Optional Commit and Revert big lock. + mu *lockedfile.Mutex msg message.Msg syscallDispatcher } +// New returns the address of a new [I] targeting uid. +func New( + ctx context.Context, + msg message.Msg, + uid int, + lock *check.Absolute, +) (sys *I) { + if ctx == nil || msg == nil || uid < 0 { + panic("invalid call to New") + } + v := I{ctx: ctx, msg: msg, uid: uid, syscallDispatcher: direct{}} + if lock != nil { + v.mu = lockedfile.MutexAt(lock.String()) + } + return &v +} + func (sys *I) UID() int { return sys.uid } // Equal returns whether all [Op] instances held by sys matches that of target. @@ -118,9 +129,18 @@ func (sys *I) Commit() error { if sys.committed { panic("attempting to commit twice") } + + if sys.mu != nil { + if unlock, err := sys.mu.Lock(); err != nil { + return err + } else { + defer unlock() + } + } sys.committed = true - sp := New(sys.ctx, sys.msg, sys.uid) + sp := New(sys.ctx, sys.msg, sys.uid, nil) + // lock is still held on revert sp.syscallDispatcher = sys.syscallDispatcher sp.ops = make([]Op, 0, len(sys.ops)) // prevent copies during commits defer func() { @@ -153,6 +173,13 @@ func (sys *I) Revert(ec *Criteria) error { if sys.reverted { panic("attempting to revert twice") } + if sys.mu != nil { + if unlock, err := sys.mu.Lock(); err != nil { + return err + } else { + defer unlock() + } + } sys.reverted = true // collect errors diff --git a/internal/system/system_test.go b/internal/system/system_test.go index e466c171..8d863826 100644 --- a/internal/system/system_test.go +++ b/internal/system/system_test.go @@ -82,7 +82,7 @@ func TestNew(t *testing.T) { t.Errorf("recover: %v, want %v", r, want) } }() - New(nil, message.New(nil), 0) + New(nil, message.New(nil), 0, nil) }) t.Run("msg", func(t *testing.T) { @@ -92,7 +92,7 @@ func TestNew(t *testing.T) { t.Errorf("recover: %v, want %v", r, want) } }() - New(t.Context(), nil, 0) + New(t.Context(), nil, 0, nil) }) t.Run("uid", func(t *testing.T) { @@ -102,11 +102,11 @@ func TestNew(t *testing.T) { t.Errorf("recover: %v, want %v", r, want) } }() - New(t.Context(), message.New(nil), -1) + New(t.Context(), message.New(nil), -1, nil) }) }) - sys := New(t.Context(), message.New(nil), 0xbeef) + sys := New(t.Context(), message.New(nil), 0xbeef, nil) if sys.ctx == nil { t.Error("New: ctx = nil") } @@ -125,51 +125,51 @@ func TestEqual(t *testing.T) { want bool }{ {"simple UID", - New(t.Context(), message.New(nil), 150), - New(t.Context(), message.New(nil), 150), + New(t.Context(), message.New(nil), 150, nil), + New(t.Context(), message.New(nil), 150, nil), true}, {"simple UID differ", - New(t.Context(), message.New(nil), 150), - New(t.Context(), message.New(nil), 151), + New(t.Context(), message.New(nil), 150, nil), + New(t.Context(), message.New(nil), 151, nil), false}, {"simple UID nil", - New(t.Context(), message.New(nil), 150), + New(t.Context(), message.New(nil), 150, nil), nil, false}, {"op length mismatch", - New(t.Context(), message.New(nil), 150). + New(t.Context(), message.New(nil), 150, nil). ChangeHosts("chronos"), - New(t.Context(), message.New(nil), 150). + New(t.Context(), message.New(nil), 150, nil). ChangeHosts("chronos"). Ensure(m("/run"), 0755, false), false}, {"op value mismatch", - New(t.Context(), message.New(nil), 150). + New(t.Context(), message.New(nil), 150, nil). ChangeHosts("chronos"). Ensure(m("/run"), 0644, false), - New(t.Context(), message.New(nil), 150). + New(t.Context(), message.New(nil), 150, nil). ChangeHosts("chronos"). Ensure(m("/run"), 0755, false), false}, {"op type mismatch", - New(t.Context(), message.New(nil), 150). + New(t.Context(), message.New(nil), 150, nil). ChangeHosts("chronos"). Wayland(m("/proc/nonexistent/dst"), m("/proc/nonexistent/src"), "\x00", "\x00"), - New(t.Context(), message.New(nil), 150). + New(t.Context(), message.New(nil), 150, nil). ChangeHosts("chronos"). Ensure(m("/run"), 0755, false), false}, {"op equals", - New(t.Context(), message.New(nil), 150). + New(t.Context(), message.New(nil), 150, nil). ChangeHosts("chronos"). Ensure(m("/run"), 0755, false), - New(t.Context(), message.New(nil), 150). + New(t.Context(), message.New(nil), 150, nil). ChangeHosts("chronos"). Ensure(m("/run"), 0755, false), true}, -- cgit v1.3.1