From fd6f9c7308d42c36e4702600007d0d263e65e0ca Mon Sep 17 00:00:00 2001 From: jrobotham Date: Wed, 22 Jul 2026 22:08:49 +0000 Subject: [PATCH] fix(client): make target dirs writable before extracting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extract restores over the directory a consumer previously extracted into. When that tree contains read-only directories (e.g. Hermit marks unpacked package trees read-only), GNU tar can't unlink the existing files to overwrite them — the parent directory isn't writable — and fails the whole extract with "tar: : Cannot open: File exists". Callers then see the restore fail and fall back to a full reinstall. Add owner-write to every directory under the target before extraction. tar re-applies each archived directory's mode as it unpacks, so only directories absent from the incoming archive are left relaxed. If extraction fails, the original modes are restored so a failed restore doesn't leave a previously read-only tree writable. Co-Authored-By: Claude Opus 4.8 --- client/archive.go | 63 ++++++++++++++++++++++++++++++++++++++++++- client/client_test.go | 63 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 125 insertions(+), 1 deletion(-) diff --git a/client/archive.go b/client/archive.go index a4d196f1..53b7d772 100644 --- a/client/archive.go +++ b/client/archive.go @@ -5,6 +5,7 @@ import ( "context" "fmt" "io" + "io/fs" "os" "os/exec" "path/filepath" @@ -52,7 +53,12 @@ func Archive(ctx context.Context, w io.Writer, baseDir string, includePaths []st // Extract decompresses a zstd+tar stream from r into directory, preserving // file permissions, ownership, and symlinks. threads controls zstd // parallelism; 0 uses all CPU cores. -func Extract(ctx context.Context, r io.Reader, directory string, threads int) error { +// +// Existing read-only directories are made owner-writable so tar can replace +// their contents. tar re-applies archived modes, and a failed extract +// restores the originals; read-only directories absent from the archive are +// left owner-writable. +func Extract(ctx context.Context, r io.Reader, directory string, threads int) (rerr error) { if threads <= 0 { threads = runtime.GOMAXPROCS(0) } @@ -61,6 +67,18 @@ func Extract(ctx context.Context, r io.Reader, directory string, threads int) er return errors.Wrap(err, "failed to create target directory") } + // Relax read-only dirs (e.g. Hermit package trees) so tar can overwrite + // their contents, restoring the original modes if anything fails. + relaxed, err := makeTreeWritable(directory) + defer func() { + if rerr != nil { + restoreDirModes(relaxed) + } + }() + if err != nil { + return errors.Wrap(err, "failed to make target directory writable") + } + zstdCmd := exec.CommandContext(ctx, "zstd", "-dc", fmt.Sprintf("-T%d", threads)) //nolint:gosec tarCmd := exec.CommandContext(ctx, "tar", "-xpf", "-", "-C", directory) @@ -103,6 +121,49 @@ func Extract(ctx context.Context, r io.Reader, directory string, threads int) er return errors.Join(errs...) } +// dirMode is a directory path and its pre-relaxation mode. +type dirMode struct { + path string + mode fs.FileMode +} + +// makeTreeWritable adds owner-write to root and every directory beneath it, +// returning the changed directories with their original modes. tar overwrites +// a file by unlinking it, which requires a writable parent directory. +func makeTreeWritable(root string) ([]dirMode, error) { + var changed []dirMode + err := filepath.WalkDir(root, func(path string, d fs.DirEntry, walkErr error) error { + switch { + case walkErr != nil: + return walkErr + case !d.IsDir(): + return nil + } + info, err := d.Info() + if err != nil { + return errors.Wrapf(err, "failed to stat %s", path) + } + // Keep setuid/setgid/sticky so relaxing and restoring don't strip them. + mode := info.Mode() & (fs.ModePerm | fs.ModeSetuid | fs.ModeSetgid | fs.ModeSticky) + if mode&0o200 == 0 { + if err := os.Chmod(path, mode|0o200); err != nil { + return errors.Wrapf(err, "failed to make %s writable", path) + } + changed = append(changed, dirMode{path: path, mode: mode}) + } + return nil + }) + return changed, errors.Wrap(err, "failed to walk target directory") +} + +// restoreDirModes best-effort restores modes relaxed by makeTreeWritable; +// failures are ignored so the extraction error stays the one reported. +func restoreDirModes(dirs []dirMode) { + for _, d := range dirs { + os.Chmod(d.path, d.mode) //nolint:errcheck,gosec + } +} + // runTarZstdPipeline runs tar piped through pzstd, writing compressed output // to w. The caller is responsible for closing w after this returns. func runTarZstdPipeline(ctx context.Context, tarArgs []string, threads int, w io.Writer) error { diff --git a/client/client_test.go b/client/client_test.go index b89f51a5..2bd08497 100644 --- a/client/client_test.go +++ b/client/client_test.go @@ -387,6 +387,69 @@ func TestArchiveExtract(t *testing.T) { assert.False(t, slices.Contains(names, "y.log")) } +func TestExtractOverReadOnlyTree(t *testing.T) { + src := t.TempDir() + assert.NoError(t, os.MkdirAll(filepath.Join(src, "pkg", "conf"), 0o755)) + assert.NoError(t, os.WriteFile(filepath.Join(src, "pkg", "conf", "cacerts"), []byte("v2"), 0o444)) + assert.NoError(t, os.Chmod(filepath.Join(src, "pkg", "conf"), 0o555)) + assert.NoError(t, os.Chmod(filepath.Join(src, "pkg"), 0o555)) + t.Cleanup(func() { makeWritable(t, src) }) + + var buf bytes.Buffer + assert.NoError(t, client.Archive(t.Context(), &buf, src, []string{"pkg"}, nil, 0)) + + dst := t.TempDir() + assert.NoError(t, os.MkdirAll(filepath.Join(dst, "pkg", "conf"), 0o755)) + assert.NoError(t, os.WriteFile(filepath.Join(dst, "pkg", "conf", "cacerts"), []byte("v1"), 0o444)) + assert.NoError(t, os.Chmod(filepath.Join(dst, "pkg", "conf"), 0o555)) + assert.NoError(t, os.Chmod(filepath.Join(dst, "pkg"), 0o555)) + t.Cleanup(func() { makeWritable(t, dst) }) + + assert.NoError(t, client.Extract(t.Context(), &buf, dst, 0)) + + got, err := os.ReadFile(filepath.Join(dst, "pkg", "conf", "cacerts")) + assert.NoError(t, err) + assert.Equal(t, "v2", string(got)) + + assertReadOnlyDir(t, filepath.Join(dst, "pkg")) + assertReadOnlyDir(t, filepath.Join(dst, "pkg", "conf")) +} + +func TestExtractFailureRestoresDirModes(t *testing.T) { + dst := t.TempDir() + assert.NoError(t, os.MkdirAll(filepath.Join(dst, "pkg", "conf"), 0o755)) + assert.NoError(t, os.WriteFile(filepath.Join(dst, "pkg", "conf", "cacerts"), []byte("v1"), 0o444)) + assert.NoError(t, os.Chmod(filepath.Join(dst, "pkg", "conf"), 0o555)) + assert.NoError(t, os.Chmod(filepath.Join(dst, "pkg"), 0o555|os.ModeSetgid)) + t.Cleanup(func() { makeWritable(t, dst) }) + + err := client.Extract(t.Context(), bytes.NewReader([]byte("not a zstd stream")), dst, 0) + assert.Error(t, err) + + info, err := os.Stat(filepath.Join(dst, "pkg")) + assert.NoError(t, err) + assert.Equal(t, os.FileMode(0o555)|os.ModeSetgid, info.Mode()&(os.ModePerm|os.ModeSetgid)) + assertReadOnlyDir(t, filepath.Join(dst, "pkg", "conf")) +} + +func assertReadOnlyDir(t *testing.T, path string) { + t.Helper() + info, err := os.Stat(path) + assert.NoError(t, err) + assert.Equal(t, os.FileMode(0o555), info.Mode().Perm()) +} + +// makeWritable re-adds owner-write under root so t.TempDir cleanup can remove it. +func makeWritable(t *testing.T, root string) { + t.Helper() + _ = filepath.WalkDir(root, func(path string, d os.DirEntry, err error) error { + if err == nil && d.IsDir() { + _ = os.Chmod(path, 0o755) + } + return nil + }) +} + func TestOpenIfNoneMatch(t *testing.T) { srv := newFakeServer(nil) defer srv.Close()