Skip to content

Commit 1b674d6

Browse files
committed
cache: harden the exec output-tree path
Address three issues in the action cache's tree materialization, found in adversarial review: 1. GetTree wrote output blobs to filepath.Join(dir, rel) with plain os calls, where rel is an output name read from the (non-self-validating) action cache entry. A tampered entry with a "../" name could escape the exec directory and write attacker-controlled CAS content anywhere the process could — contradicting the confinement the rest of the cache gets from os.Root. Reject any rel that is not filepath.IsLocal. 2. GetTree trusted an already-present file by size alone and never re-hashed it, and staged writes without fsync. A right-sized but torn or corrupt exec file (power loss, bitrot) was therefore trusted forever: wazero hard-fails deserializing it instead of recompiling, and the size match kept GetTree from ever repairing it from the CAS, bricking codegen until a manual cache wipe. Reuse an existing file only when it still hashes to the digest, and stage writes through an fsynced temp + rename. 3. The exec directory was shared across processes at exec/<action-hash>. wazero stages <key>.tmp files in place while compiling, which a concurrent process's PutTree WalkDir could sweep into its action result. ExecDir now returns a fresh private directory per call, and the WASM runner removes it once wazero has loaded the module; the compiled bytes remain reproducible from the CAS. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MPgq3rwip76D554BktbqR4
1 parent 06d3255 commit 1b674d6

3 files changed

Lines changed: 67 additions & 30 deletions

File tree

internal/cache/action.go

Lines changed: 48 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -103,17 +103,30 @@ func (a *ActionCache) PutTree(action Digest, dir string) error {
103103
}
104104

105105
// GetTree materializes a cached action's outputs as files under dir, or
106-
// returns ErrNotFound on a miss. Files already present with the right size
107-
// are left in place; missing ones are staged in their destination directory
108-
// and renamed so concurrent processes never observe partial files.
106+
// returns ErrNotFound on a miss. A file already present is reused only if its
107+
// contents still hash to the expected digest; otherwise it is rewritten from
108+
// the CAS. Writes are staged, fsynced, and renamed, so a crash cannot leave a
109+
// right-sized but torn file that later reads would trust.
109110
func (a *ActionCache) GetTree(action Digest, dir string) error {
110111
result, err := a.Get(action)
111112
if err != nil {
112113
return err
113114
}
114115
for rel, d := range result.Outputs {
116+
// Output names come from the action cache entry, which — unlike a CAS
117+
// blob — is not self-validating, so a tampered entry could carry a
118+
// name like "../../etc/x". Reject anything that isn't a relative path
119+
// confined to dir; this is the confinement the os.Root gives the rest
120+
// of the cache, which GetTree can't use because dir is a caller-owned
121+
// path a tool must read by absolute name.
122+
if !filepath.IsLocal(rel) {
123+
return fmt.Errorf("cache: unsafe output path %q in action %s", rel, action)
124+
}
115125
path := filepath.Join(dir, filepath.FromSlash(rel))
116-
if fi, err := os.Stat(path); err == nil && fi.Size() == d.SizeBytes {
126+
// Trust an existing file only if it still hashes to the digest;
127+
// size alone can mask in-place corruption that would make the
128+
// consuming tool hard-fail with no way to repair (see wazero).
129+
if existing, err := os.ReadFile(path); err == nil && DigestOf(existing) == d {
117130
continue
118131
}
119132
data, err := a.cas.Get(d)
@@ -123,27 +136,43 @@ func (a *ActionCache) GetTree(action Digest, dir string) error {
123136
if err := os.MkdirAll(filepath.Dir(path), 0755); err != nil {
124137
return fmt.Errorf("cache: %w", err)
125138
}
126-
f, err := os.CreateTemp(filepath.Dir(path), d.Hash[:8]+"-*")
127-
if err != nil {
128-
return fmt.Errorf("cache: %w", err)
129-
}
130-
if _, err := f.Write(data); err != nil {
131-
f.Close()
132-
os.Remove(f.Name())
133-
return fmt.Errorf("cache: %w", err)
134-
}
135-
if err := f.Close(); err != nil {
136-
os.Remove(f.Name())
137-
return fmt.Errorf("cache: %w", err)
138-
}
139-
if err := os.Rename(f.Name(), path); err != nil {
140-
os.Remove(f.Name())
139+
if err := writeFileAtomic(path, data); err != nil {
141140
return fmt.Errorf("cache: %w", err)
142141
}
143142
}
144143
return nil
145144
}
146145

146+
// writeFileAtomic writes data to path via a staged temp file in the same
147+
// directory, fsynced before an atomic rename, so a reader never observes a
148+
// partial or torn file even across a crash.
149+
func writeFileAtomic(path string, data []byte) error {
150+
f, err := os.CreateTemp(filepath.Dir(path), "."+filepath.Base(path)+"-*")
151+
if err != nil {
152+
return err
153+
}
154+
tmp := f.Name()
155+
if _, err := f.Write(data); err != nil {
156+
f.Close()
157+
os.Remove(tmp)
158+
return err
159+
}
160+
if err := f.Sync(); err != nil {
161+
f.Close()
162+
os.Remove(tmp)
163+
return err
164+
}
165+
if err := f.Close(); err != nil {
166+
os.Remove(tmp)
167+
return err
168+
}
169+
if err := os.Rename(tmp, path); err != nil {
170+
os.Remove(tmp)
171+
return err
172+
}
173+
return nil
174+
}
175+
147176
// Put records the result of an action. All outputs must already be in the
148177
// CAS; writes are staged and renamed so concurrent processes never observe a
149178
// partial entry.

internal/cache/cache.go

Lines changed: 15 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -83,15 +83,21 @@ func (c *Cache) Close() error {
8383
return c.root.Close()
8484
}
8585

86-
// ExecDir returns a stable directory for materializing the output tree of
87-
// the given action, for tools that need their outputs on disk (like wazero's
88-
// compilation cache). It lives at exec/<action-hash> under the cache root
89-
// and, like everything else in the cache, is safe to delete at any time: the
90-
// authoritative copy of its contents is the CAS.
86+
// ExecDir creates a fresh, private scratch directory for materializing the
87+
// output tree of the given action, for tools that need their outputs on disk
88+
// (like wazero's compilation cache). Each call returns a new directory under
89+
// exec/, so two concurrent processes never share one — otherwise a tool
90+
// staging files there (wazero writes <key>.tmp files in place) could be swept
91+
// into the other's PutTree. The caller must remove it when done; its contents
92+
// are always reproducible from the CAS, so losing it is harmless.
9193
func (c *Cache) ExecDir(action Digest) (string, error) {
92-
rel := filepath.Join("exec", action.Hash)
93-
if err := c.root.MkdirAll(rel, 0755); err != nil {
94-
return "", fmt.Errorf("failed to create %s directory: %w", rel, err)
94+
base := filepath.Join(c.root.Name(), "exec")
95+
if err := os.MkdirAll(base, 0755); err != nil {
96+
return "", fmt.Errorf("failed to create %s directory: %w", base, err)
9597
}
96-
return filepath.Join(c.root.Name(), rel), nil
98+
dir, err := os.MkdirTemp(base, action.Hash+"-")
99+
if err != nil {
100+
return "", fmt.Errorf("cache: %w", err)
101+
}
102+
return dir, nil
97103
}

internal/ext/wasm/wasm.go

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -139,8 +139,9 @@ func (r *Runner) loadAndCompileWASM(ctx context.Context, store *cache.Cache, exp
139139
// Its only declared input is the module's checksum: the embedded wazero
140140
// version and the target platform are determined by the sqlc binary,
141141
// which is an implicit input of every action. Compiled artifacts are
142-
// materialized into an exec directory for wazero's compilation cache to
143-
// find; the authoritative copies live in the CAS.
142+
// materialized into a private exec directory for wazero's compilation
143+
// cache to find; the authoritative copies live in the CAS. Once wazero
144+
// has loaded the module into memory the directory is no longer needed.
144145
compileAction := store.NewAction("CompileModule").
145146
AddInput("wasm", []byte(expected)).
146147
Digest()
@@ -149,6 +150,7 @@ func (r *Runner) loadAndCompileWASM(ctx context.Context, store *cache.Cache, exp
149150
if err != nil {
150151
return nil, err
151152
}
153+
defer os.RemoveAll(execDir)
152154
compiled := true
153155
if err := store.Actions.GetTree(compileAction, execDir); errors.Is(err, cache.ErrNotFound) {
154156
compiled = false

0 commit comments

Comments
 (0)