amadan

carlos / platform Public

zdd plan 2 task 3 fix 2: the intent path still refuses an existing role child

The dedupe in d0e3a999 changed mkdir behavior: the original code rejected EEXIST
when intent was provided (role-with-intent requires a new name), but pinChild
allowed EEXIST for all callers. This broke PrepareRoleChildWithIntent's guarantee.

Fix: pass allowExisting bool to pinChild. Generation children and role children
without intent pass true (allow reusing). Role children with intent pass false
(require new name). Both match the original behavior exactly.

Also removed the dead defer in prepareRoleChild: nothing after pinChild can set
err, so the defer never executes.

Added test TestPrepareRoleChildWithIntentRefusesExistingDirectory that creates
the directory from inside the intent callback to verify the intent path refuses it.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude e85733a43ed5229cf3f19d6fd0c331e911f1146b parent af809b0
3 files changed, +44 −14
  • internal/cgroup/role_linux.go +2 −9
  • internal/cgroup/role_linux_test.go +33 −0
  • internal/cgroup/strict_gen_linux.go +9 −5
diff --git a/internal/cgroup/role_linux.go b/internal/cgroup/role_linux.go
index d753c2f42..c6e5c5c51 100644
--- a/internal/cgroup/role_linux.go
+++ b/internal/cgroup/role_linux.go
@@ -64,17 +64,10 @@ func (p *Prepared) prepareRoleChild(role string, intent func(ContainmentIdentity
}
return nil
}
- child, err := pinChild(p, name, beforeMkdir)
+ allowExisting := intent == nil
+ child, err := pinChild(p, name, beforeMkdir, allowExisting)
if err != nil {
return nil, err
}
- defer func() {
- if err != nil {
- child.closeFiles()
- strictReservations.Lock()
- delete(strictReservations.held, child.identity)
- strictReservations.Unlock()
- }
- }()
return child, nil
}
diff --git a/internal/cgroup/role_linux_test.go b/internal/cgroup/role_linux_test.go
index 6cb75d1bb..0cde0ec74 100644
--- a/internal/cgroup/role_linux_test.go
+++ b/internal/cgroup/role_linux_test.go
@@ -169,3 +169,36 @@ func TestRoleChildRefusesAfterCleanupAcquisitionCancellation(t *testing.T) {
t.Fatalf("new role after cancelled cleanup: %v", err)
}
}
+
+// TestPrepareRoleChildWithIntentRefusesExistingDirectory verifies that the intent
+// path refuses with ErrStrictRefused when the child directory appears between the
+// intent callback and mkdir (i.e., the intent callback creates the directory).
+func TestPrepareRoleChildWithIntentRefusesExistingDirectory(t *testing.T) {
+ p := strictFixture(t)
+ intentCalled := false
+ intent := func(id ContainmentIdentity) error {
+ intentCalled = true
+ // Simulate another actor creating the directory after our intent checks for it
+ // but before mkdir is called
+ if err := os.Mkdir(filepath.Join(p.dir.Name(), "role-newchild"), 0700); err != nil {
+ t.Fatal(err)
+ }
+ // Also create the required cgroup files
+ for n, v := range map[string]string{"cgroup.events": "populated 0\n", "cgroup.kill": ""} {
+ if err := os.WriteFile(filepath.Join(p.dir.Name(), "role-newchild", n), []byte(v), 0600); err != nil {
+ t.Fatal(err)
+ }
+ }
+ return nil
+ }
+ child, err := p.PrepareRoleChildWithIntent("newchild", intent)
+ if err != nil && !intentCalled {
+ t.Fatalf("intent was not called before error: %v", err)
+ }
+ if !intentCalled {
+ t.Fatal("intent callback was not called")
+ }
+ if !errors.Is(err, ErrStrictRefused) || child != nil {
+ t.Fatalf("intent path did not refuse pre-existing directory: err=%v child=%v", err, child)
+ }
+}
diff --git a/internal/cgroup/strict_gen_linux.go b/internal/cgroup/strict_gen_linux.go
index 006bcab5e..923532316 100644
--- a/internal/cgroup/strict_gen_linux.go
+++ b/internal/cgroup/strict_gen_linux.go
@@ -26,19 +26,23 @@ func tierExpected(l Limit, oomGroup string) map[string]string {
}
}
-// pinChild pins a child cgroup under parent: mkdir, create child struct, FcntlInt,
-// strictOpen (dir, events, kill), strictID, reservation, and population check.
+// pinChild pins a child cgroup under parent: beforeMkdir callback (if provided),
+// mkdir, create child struct, FcntlInt, strictOpen (dir, events, kill), strictID,
+// reservation, and population check.
// beforeMkdir is called before mkdir (for role intent persistence; may be nil).
+// allowExisting controls whether mkdir's EEXIST is accepted: true allows reusing
+// an existing directory (generation children, role children without intent),
+// false refuses it (role children with intent require a new name).
// On success, returns a fully pinned, reserved child.
// On error, all cleanup is done internally; caller needs only to set up a defer
// for cleanup of any subsequent operations (e.g. tier writes for generation children).
-func pinChild(parent *Prepared, name string, beforeMkdir func() error) (*Prepared, error) {
+func pinChild(parent *Prepared, name string, beforeMkdir func() error, allowExisting bool) (*Prepared, error) {
if beforeMkdir != nil {
if err := beforeMkdir(); err != nil {
return nil, err
}
}
- if err := unix.Mkdirat(int(parent.dir.Fd()), name, 0755); err != nil && !errors.Is(err, unix.EEXIST) {
+ if err := unix.Mkdirat(int(parent.dir.Fd()), name, 0755); err != nil && (!allowExisting || !errors.Is(err, unix.EEXIST)) {
return nil, ErrStrictRefused
}
child := &Prepared{strictPreparedState: &strictPreparedState{parent: parent, host: parent.host, limits: map[string]*os.File{}, expected: map[string]string{}, entryName: name, start: func(cmd *exec.Cmd) error { return cmd.Start() }}}
@@ -160,7 +164,7 @@ func (p *Prepared) PrepareGen(n uint64, tier Limit) (out *Prepared, err error) {
return nil, err
}
name := GenChild(n)
- child, err := pinChild(p, name, nil)
+ child, err := pinChild(p, name, nil, true)
if err != nil {
return nil, err
}