amadan

carlos / platform Public

activator: flat env and config reads open O_NONBLOCK and accept only a regular file (final review I3, minor 6)

A FIFO planted at a flat <host>.env opened in blocking mode and waited for
a writer, under adminTokenMu, inside the startup backfill that runs before
the activator starts: one planted file kept every instance on the box
from waking. openOwnEnvNoFollow now opens O_NONBLOCK as well as
O_NOFOLLOW, accepts only a regular file from fstat, and hands the
descriptor back blocking. The flat read of <host>.config.env goes through
the same opener and the same 1 MiB cap as the nested read, closing the
first of minor 6's two asymmetries. The windows stand-in checks the
opened file the same way. The FIFO test is now a table over the nested
own file, the flat own file and the flat configuration, bounded at two
seconds each.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Paul Campbell pushed by claude c74d0cda42cc9bb974feda2d17151da91dda6e03 parent 3654228
4 files changed, +151 −46
  • internal/activator/instancekey.go +34 −12
  • internal/activator/instancekey_linux_test.go +67 −22
  • internal/activator/ownenv_other.go +19 −5
  • internal/activator/ownenv_unix.go +31 −7
diff --git a/internal/activator/instancekey.go b/internal/activator/instancekey.go
index d760c64f9..72e9744dc 100644
--- a/internal/activator/instancekey.go
+++ b/internal/activator/instancekey.go
@@ -240,15 +240,40 @@ func openInstanceEnv(dir, name string, beneath, create bool) (*os.File, error) {
// sibling chose (spec §7.2).
var errOwnEnvSymlink = errors.New("activator: the instance env file is a symlink, refused")
-// readOwnEnv reads a flat row's own env file through openOwnEnvNoFollow,
-// so the read refuses a link exactly as the append does.
+// errOwnEnvNotRegular is a flat row's own env file, or its rendered
+// configuration, found to be something other than a regular file: a FIFO,
+// which a blocking open would wait on for a writer that never comes (the
+// startup backfill holds adminTokenMu while it reads, so one planted FIFO
+// would keep every instance on the box from waking), or a device. Refused
+// on every path, as the link is.
+var errOwnEnvNotRegular = errors.New("activator: the instance env file is not a regular file, refused")
+
+// ownEnvReadLimit caps a flat read of an own env file or its rendered
+// configuration, as instancefs.ReadBeneathLimit caps the nested one: no
+// read under adminTokenMu is unbounded.
+const ownEnvReadLimit = 1 << 20
+
+// errOwnEnvTooLarge is a flat env file over ownEnvReadLimit.
+var errOwnEnvTooLarge = errors.New("activator: the instance env file is larger than the limit, refused")
+
+// readOwnEnv reads a flat row's own env file, or its rendered
+// configuration, through openOwnEnvNoFollow, so the read refuses a link
+// or a FIFO exactly as the append does, and never reads more than
+// ownEnvReadLimit.
func readOwnEnv(path string) ([]byte, error) {
f, err := openOwnEnvNoFollow(path, os.O_RDONLY, 0)
if err != nil {
return nil, err
}
defer f.Close()
- return io.ReadAll(f)
+ raw, err := io.ReadAll(io.LimitReader(f, ownEnvReadLimit+1))
+ if err != nil {
+ return nil, err
+ }
+ if len(raw) > ownEnvReadLimit {
+ return nil, errOwnEnvTooLarge
+ }
+ return raw, nil
}
// settleInstanceEnv is the durability step every successful ensure ends
@@ -275,19 +300,16 @@ func ensureInstanceKey(dir, name string, beneath, create bool) (InstanceKeyOutco
path := filepath.Join(dir, name)
adminTokenMu.Lock()
defer adminTokenMu.Unlock()
+ // Both files are read the same guarded way: through instancefs on the
+ // nested layout, and on the flat one through readOwnEnv, which refuses
+ // a link or a FIFO at either name and caps the read.
read := func(n string) ([]byte, error) {
if beneath {
- return instancefs.ReadBeneathLimit(dir, n, 1<<20)
+ return instancefs.ReadBeneathLimit(dir, n, ownEnvReadLimit)
}
- return os.ReadFile(filepath.Join(dir, n))
- }
- var raw []byte
- var err error
- if beneath {
- raw, err = read(name)
- } else {
- raw, err = readOwnEnv(path)
+ return readOwnEnv(filepath.Join(dir, n))
}
+ raw, err := read(name)
switch {
case errors.Is(err, os.ErrNotExist):
if !create {
diff --git a/internal/activator/instancekey_linux_test.go b/internal/activator/instancekey_linux_test.go
index 9aca29b74..095cdac08 100644
--- a/internal/activator/instancekey_linux_test.go
+++ b/internal/activator/instancekey_linux_test.go
@@ -1,6 +1,7 @@
package activator
import (
+ "errors"
"os"
"path/filepath"
"strings"
@@ -11,6 +12,7 @@ import (
"github.com/carlosframework/platform/internal/instancectl"
"github.com/carlosframework/platform/internal/provision"
+ "github.com/carlosframework/platform/internal/registry"
)
// The nested prepare mints the key into the instance directory's own env
@@ -90,28 +92,71 @@ func TestEnsureInstanceKeyBeneathRefusesASymlink(t *testing.T) {
}
}
-// Review focus 4: a FIFO planted at a nested env file's name fails the
-// backfill fast; it never blocks agent start.
+// Review focus 4 and the final review's I3: a FIFO planted at an env
+// file's name fails the backfill fast and is refused; it never blocks
+// agent start. The nested row's files are opened through instancefs; the
+// flat row's own file and its rendered configuration through
+// openOwnEnvNoFollow, which opens O_NONBLOCK and accepts only a regular
+// file. The startup backfill runs under adminTokenMu before the activator
+// starts, so a blocking open here would keep every instance on the box
+// from waking.
func TestBackfillInstanceKeyRefusesAFIFOWithoutBlocking(t *testing.T) {
- b, rt := nestedBackend(t, &fakeControl{inv: "inv1"}, &fakeUnit{})
- dir := provision.InstanceDir(b.DataDir, rt.Host)
- if err := os.MkdirAll(dir, 0o700); err != nil {
- t.Fatal(err)
- }
- if err := unix.Mkfifo(filepath.Join(dir, rt.Host+".env"), 0o600); err != nil {
- t.Fatal(err)
- }
- done := make(chan error, 1)
- go func() {
- _, err := b.BackfillInstanceKey(rt)
- done <- err
- }()
- select {
- case err := <-done:
- if err == nil {
- t.Fatal("backfill over a FIFO succeeded, want an error")
- }
- case <-time.After(2 * time.Second):
- t.Fatal("backfill blocked on a FIFO")
+ for _, tc := range []struct {
+ name string
+ flat bool
+ file string // the file the FIFO is planted at, relative to the row's directory
+ }{
+ {"nested own file", false, ".env"},
+ {"flat own file", true, ".env"},
+ {"flat configuration", true, ".config.env"},
+ } {
+ t.Run(tc.name, func(t *testing.T) {
+ var b *ExecBackend
+ var rt registry.Route
+ var dir string
+ if tc.flat {
+ b = testExecBackend(t)
+ rt = registry.Route{Host: "shop.example.com", Kind: "instance"}
+ dir = b.DataDir
+ // A flat own file exists, so the backfill reaches the
+ // configuration read when the FIFO is the configuration.
+ if tc.file != ".env" {
+ if err := os.WriteFile(filepath.Join(dir, rt.Host+".env"), []byte("A=1\n"), 0o600); err != nil {
+ t.Fatal(err)
+ }
+ }
+ } else {
+ b, rt = nestedBackend(t, &fakeControl{inv: "inv1"}, &fakeUnit{})
+ dir = provision.InstanceDir(b.DataDir, rt.Host)
+ if err := os.MkdirAll(dir, 0o700); err != nil {
+ t.Fatal(err)
+ }
+ }
+ if err := unix.Mkfifo(filepath.Join(dir, rt.Host+tc.file), 0o600); err != nil {
+ t.Fatal(err)
+ }
+ done := make(chan error, 1)
+ go func() {
+ _, err := b.BackfillInstanceKey(rt)
+ done <- err
+ }()
+ select {
+ case err := <-done:
+ if err == nil {
+ t.Fatal("backfill over a FIFO succeeded, want an error")
+ }
+ if tc.flat && !errors.Is(err, errOwnEnvNotRegular) {
+ t.Fatalf("backfill over a flat FIFO = %v, want errOwnEnvNotRegular", err)
+ }
+ case <-time.After(2 * time.Second):
+ t.Fatal("backfill blocked on a FIFO")
+ }
+ if tc.file == ".env" {
+ return // the own file is the FIFO: nothing to read back
+ }
+ if raw, err := os.ReadFile(filepath.Join(dir, rt.Host+".env")); err != nil || string(raw) != "A=1\n" {
+ t.Fatalf("the own file changed beside a refused configuration: %q, %v", raw, err)
+ }
+ })
}
}
diff --git a/internal/activator/ownenv_other.go b/internal/activator/ownenv_other.go
index 1f22d2b77..cc7c2ac2d 100644
--- a/internal/activator/ownenv_other.go
+++ b/internal/activator/ownenv_other.go
@@ -7,13 +7,27 @@ import (
)
// openOwnEnvNoFollow is the unix helper's stand-in where there is no
-// O_NOFOLLOW (windows). The agent only runs on Linux; this keeps the
-// cross-compiled builds working and still refuses a link it can see, by
-// the same Lstat check EnsureAdminToken makes. It is check-then-open, so
-// it is weaker than the unix open, which is the one that matters.
+// O_NOFOLLOW or O_NONBLOCK (windows). The agent only runs on Linux; this
+// keeps the cross-compiled builds working and still refuses a link it can
+// see, by the same Lstat check EnsureAdminToken makes, and a non-file once
+// it has opened it. It is check-then-open, so it is weaker than the unix
+// open, which is the one that matters.
func openOwnEnvNoFollow(path string, flag int, perm os.FileMode) (*os.File, error) {
if fi, err := os.Lstat(path); err == nil && fi.Mode()&os.ModeSymlink != 0 {
return nil, errOwnEnvSymlink
}
- return os.OpenFile(path, flag, perm)
+ f, err := os.OpenFile(path, flag, perm)
+ if err != nil {
+ return nil, err
+ }
+ st, err := f.Stat()
+ if err != nil {
+ f.Close()
+ return nil, err
+ }
+ if !st.Mode().IsRegular() {
+ f.Close()
+ return nil, errOwnEnvNotRegular
+ }
+ return f, nil
}
diff --git a/internal/activator/ownenv_unix.go b/internal/activator/ownenv_unix.go
index f37c31789..c9830d5ff 100644
--- a/internal/activator/ownenv_unix.go
+++ b/internal/activator/ownenv_unix.go
@@ -5,19 +5,43 @@ package activator
import (
"errors"
"os"
+ "syscall"
"golang.org/x/sys/unix"
)
-// openOwnEnvNoFollow opens a flat row's own <host>.env and refuses a
-// symlink at it in the open itself (O_NOFOLLOW), so there is no window
-// between a check and the open for a same-uid sibling (spec §7.2) to plant
-// a link in. A refused link is the bare errOwnEnvSymlink: the caller names
-// the path, once. A nested row's file needs none of this: instancefs.OpenBeneath already refuses links.
+// openOwnEnvNoFollow opens a flat row's own env file (<host>.env, or its
+// sibling <host>.config.env) and refuses, in the open itself, what a
+// same-uid sibling (spec §7.2) could have planted at the name: a symlink,
+// by O_NOFOLLOW; and a FIFO or a device, by opening O_NONBLOCK (so the open
+// never waits for a writer that will not come) and then accepting only a
+// regular file from fstat. There is no window between a check and the
+// open. A refused link is the bare errOwnEnvSymlink and a refused non-file
+// the bare errOwnEnvNotRegular: the caller names the path, once. The
+// descriptor is handed back blocking: a regular file never blocks, and the
+// appends and fsyncs that follow want ordinary semantics. A nested row's
+// file needs none of this: instancefs.OpenBeneath does the same with
+// openat2.
func openOwnEnvNoFollow(path string, flag int, perm os.FileMode) (*os.File, error) {
- f, err := os.OpenFile(path, flag|unix.O_NOFOLLOW, perm)
+ f, err := os.OpenFile(path, flag|unix.O_NOFOLLOW|unix.O_NONBLOCK, perm)
if errors.Is(err, unix.ELOOP) {
return nil, errOwnEnvSymlink
}
- return f, err
+ if err != nil {
+ return nil, err
+ }
+ st, err := f.Stat()
+ if err != nil {
+ f.Close()
+ return nil, err
+ }
+ if !st.Mode().IsRegular() {
+ f.Close()
+ return nil, errOwnEnvNotRegular
+ }
+ if err := syscall.SetNonblock(int(f.Fd()), false); err != nil {
+ f.Close()
+ return nil, err
+ }
+ return f, nil
}