fix(brainfs,session): clean paths only; session logs open through the brain root (#173)
Review round 1 on PR #187: - realDir checked the cleaned parent while Lstat/OpenFile used the raw path; os.Root resolves ".." after following a symlink, so a ".." placed after a symlinked directory passed the check and redirected the read or write. ReadRegular, ReadRegularMax and WriteRegular now refuse a path that is not clean and local (ErrNotRegular). No current entry point reached it (callers clean first); the primitive no longer relies on that. - session.Append checked the sessions dir with Lstat, then opened by plain path: a symlink swapped in between created logs outside the brain (29k in the reviewer's RENAME_EXCHANGE race). It now opens through an os.Root on the sessions dir's parent, so a swap can only redirect inside the brain (#167), and, since os.Root follows an in-root final symlink even with O_NOFOLLOW, writes only after confirming the path is the regular file it opened (Lstat + os.SameFile). - Endpoint tests assert ErrorIs(brainfs.ErrNotRegular) and have positive controls. golang.org/x/sys moves from indirect to direct (unix.Renameat2 in the session race test); it was already in the module graph. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
@@ -2,14 +2,17 @@ package session
|
||||
|
||||
import (
|
||||
"errors"
|
||||
"fmt"
|
||||
"os"
|
||||
"path/filepath"
|
||||
"sync/atomic"
|
||||
"syscall"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
"github.com/stretchr/testify/assert"
|
||||
"github.com/stretchr/testify/require"
|
||||
"golang.org/x/sys/unix"
|
||||
)
|
||||
|
||||
// session_log passed the caller's session_id straight into
|
||||
@@ -102,3 +105,73 @@ func TestAppendRefusesASymlinkedSessionsDir(t *testing.T) {
|
||||
ents, _ := os.ReadDir(elsewhere)
|
||||
assert.Empty(t, ents, "a log was created through the symlinked dir")
|
||||
}
|
||||
|
||||
// The sessions dir is swapped (RENAME_EXCHANGE) with a symlink to a
|
||||
// directory outside the brain while Append runs. Append opens through an
|
||||
// os.Root on the sessions dir's parent, so a swap can redirect it only
|
||||
// inside that parent (the #167 residual), never outside it.
|
||||
func TestAppendNeverCreatesALogOutsideTheParentUnderADirSwapRace(t *testing.T) {
|
||||
brain := t.TempDir()
|
||||
outside := t.TempDir()
|
||||
dir := filepath.Join(brain, "sessions")
|
||||
other := filepath.Join(brain, "sessions.link")
|
||||
require.NoError(t, os.MkdirAll(dir, 0o755))
|
||||
require.NoError(t, os.Symlink(outside, other))
|
||||
var stop atomic.Bool
|
||||
done := make(chan struct{})
|
||||
go func() {
|
||||
defer close(done)
|
||||
for !stop.Load() {
|
||||
_ = unix.Renameat2(unix.AT_FDCWD, dir, unix.AT_FDCWD, other, unix.RENAME_EXCHANGE)
|
||||
}
|
||||
}()
|
||||
ok := 0
|
||||
for i := 0; i < 20000; i++ {
|
||||
if Append(dir, fmt.Sprintf("s%d", i), Entry{SessionID: "s"}) == nil {
|
||||
ok++
|
||||
}
|
||||
}
|
||||
stop.Store(true)
|
||||
<-done
|
||||
ents, _ := os.ReadDir(outside)
|
||||
assert.Empty(t, ents, "logs were created outside the brain")
|
||||
assert.Positive(t, ok, "positive control: appends to the real dir succeed")
|
||||
}
|
||||
|
||||
// The log file is swapped between a regular file and a symlink to another
|
||||
// file in the brain while Append runs: Append writes only after confirming
|
||||
// the path is the regular file it opened, so the other file never grows.
|
||||
func TestAppendNeverWritesThroughALogSwappedForASymlink(t *testing.T) {
|
||||
brain := t.TempDir()
|
||||
dir := filepath.Join(brain, "sessions")
|
||||
require.NoError(t, os.MkdirAll(dir, 0o755))
|
||||
victim := filepath.Join(brain, "victim.md")
|
||||
require.NoError(t, os.WriteFile(victim, []byte("untouched\n"), 0o644))
|
||||
log := filepath.Join(dir, "s.jsonl")
|
||||
var stop atomic.Bool
|
||||
done := make(chan struct{})
|
||||
go func() {
|
||||
defer close(done)
|
||||
tmp := log + ".swap"
|
||||
for i := 0; !stop.Load(); i++ {
|
||||
_ = os.Remove(tmp)
|
||||
if i%2 == 0 {
|
||||
_ = os.WriteFile(tmp, nil, 0o644)
|
||||
} else {
|
||||
_ = os.Symlink(filepath.Join("..", "victim.md"), tmp)
|
||||
}
|
||||
_ = os.Rename(tmp, log)
|
||||
}
|
||||
}()
|
||||
ok := 0
|
||||
for i := 0; i < 20000; i++ {
|
||||
if Append(dir, "s", Entry{SessionID: "s"}) == nil {
|
||||
ok++
|
||||
}
|
||||
}
|
||||
stop.Store(true)
|
||||
<-done
|
||||
got, _ := os.ReadFile(victim)
|
||||
assert.Equal(t, "untouched\n", string(got), "a log line was written through the swapped-in symlink")
|
||||
assert.Positive(t, ok, "positive control: appends to the regular log succeed")
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user