From 333189a20b7bb2b7440b083e151afd24bb5a61d4 Mon Sep 17 00:00:00 2001 From: Eugene Blikh Date: Fri, 7 Aug 2026 01:16:13 +0300 Subject: [PATCH] archive: restore trees with read-only directories; bump to 0.2.2 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit DecodeDir applied each archived mode as its entry landed, so a 0555 directory became read-only the moment it was created and the first file written into it failed with EACCES. That is not an edge case: the Go module cache is 0555 directories and 0444 files throughout, and it is the main thing dir download is pointed at. Build #295 died on the first cache HIT of the new gomod key, having passed on the miss. Directories are now created with the owner-write bit forced on and their recorded mode applied after extraction finishes, deepest-first. Files are written writable and chmod'ed after the copy, and a read-only file left by an earlier restore is replaced rather than treated as an error — re-restoring over a populated module cache is normal in CI. Both tests fail against the previous DecodeDir with the same 'permission denied' the build reported. --- CHANGELOG.md | 15 +++++ VERSION | 2 +- internal/archive/archive.go | 68 ++++++++++++++++++- internal/archive/archive_test.go | 110 +++++++++++++++++++++++++++++++ 4 files changed, 191 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 46eee202f1a0df4a30929acc8725bee151176c97..6dc9ff52ef18d43bd6cd7dcea92e6c6e09910d49 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,20 @@ project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). ## [Unreleased] +## [0.2.2] — 2026-08-07 + +### Fixed +- `dir download` could not restore a tree containing read-only + directories — which the Go module cache is throughout (0555 dirs, + 0444 files). Extraction applied each archived mode as the entry + landed, so the first file written into a restored 0555 directory + failed with `permission denied`. Directories are now created + writable and their recorded mode applied once extraction finishes, + files are written writable and chmod'ed afterwards, and a read-only + file left by an earlier restore is replaced instead of erroring. + Caught by this repo's own CI on the first cache HIT of the new + `gomod/*.tar.zst` key (build #295). + ## [0.2.1] — 2026-08-07 ### Fixed @@ -112,6 +126,7 @@ with a single static Go binary. must look in the same place. [Unreleased]: https://git.srht.bigb.es/~bigbes/ci-cacher/log/master +[0.2.2]: https://git.srht.bigb.es/~bigbes/ci-cacher/refs/v0.2.2 [0.2.1]: https://git.srht.bigb.es/~bigbes/ci-cacher/refs/v0.2.1 [0.2.0]: https://git.srht.bigb.es/~bigbes/ci-cacher/refs/v0.2.0 [0.1.2]: https://git.srht.bigb.es/~bigbes/ci-cacher/refs/v0.1.2 diff --git a/VERSION b/VERSION index 0c62199f16ac1e2d7f7ae75b420c1231325dff4e..ee1372d33a29e27945406f0527f8af8e6ee119c9 100644 --- a/VERSION +++ b/VERSION @@ -1 +1 @@ -0.2.1 +0.2.2 diff --git a/internal/archive/archive.go b/internal/archive/archive.go index 77fdb284b9838a164e3b908f6b406d8685870f42..6060e2beda73006b5dab97858532898e203019e3 100644 --- a/internal/archive/archive.go +++ b/internal/archive/archive.go @@ -108,6 +108,13 @@ func writeOne(tw *tar.Writer, root, p string) error { // DecodeDir extracts a tar.zst stream from r into dest. dest must exist. // Any entry whose normalized path escapes dest is rejected. +// +// Read-only entries are the normal case, not an exotic one: the Go module +// cache is 0555 directories and 0444 files throughout. So directories are +// created writable and their recorded mode is applied at the very end, +// and files are written writable and chmod'ed after the copy. Restoring +// the archived mode as we go would lock the extraction out of the tree it +// is still filling in. func DecodeDir(r io.Reader, dest string) error { absDest, err := filepath.Abs(dest) if err != nil { @@ -120,10 +127,14 @@ func DecodeDir(r io.Reader, dest string) error { defer zr.Close() tr := tar.NewReader(zr) + // Directory modes, in archive order, applied once everything is in + // place. Deepest-first on the way back out. + var dirs []dirMode + for { hdr, err := tr.Next() if errors.Is(err, io.EOF) { - return nil + return restoreDirModes(dirs) } if err != nil { return fmt.Errorf("tar next: %w", err) @@ -132,16 +143,18 @@ func DecodeDir(r io.Reader, dest string) error { if err != nil { return err } + mode := fs.FileMode(hdr.Mode) & 0o7777 switch hdr.Typeflag { case tar.TypeDir: - if err := os.MkdirAll(target, fs.FileMode(hdr.Mode)&0o7777); err != nil { + if err := os.MkdirAll(target, mode|0o700); err != nil { return fmt.Errorf("mkdir %s: %w", target, err) } + dirs = append(dirs, dirMode{path: target, mode: mode}) case tar.TypeReg, tar.TypeRegA: if err := os.MkdirAll(filepath.Dir(target), 0o755); err != nil { return fmt.Errorf("mkdir parent of %s: %w", target, err) } - f, err := os.OpenFile(target, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, fs.FileMode(hdr.Mode)&0o7777) + f, err := createFile(target, mode) if err != nil { return fmt.Errorf("create %s: %w", target, err) } @@ -152,6 +165,9 @@ func DecodeDir(r io.Reader, dest string) error { if err := f.Close(); err != nil { return err } + if err := os.Chmod(target, mode); err != nil { + return fmt.Errorf("chmod %s: %w", target, err) + } case tar.TypeSymlink: _ = os.Remove(target) if err := os.Symlink(hdr.Linkname, target); err != nil { @@ -163,6 +179,52 @@ func DecodeDir(r io.Reader, dest string) error { } } +// dirMode is a directory whose archived mode is applied after extraction. +type dirMode struct { + path string + mode fs.FileMode +} + +// restoreDirModes applies the archived modes deepest-first: entries arrive +// parent-before-child, so walking back tightens a directory only after +// everything under it is already done. +func restoreDirModes(dirs []dirMode) error { + for i := len(dirs) - 1; i >= 0; i-- { + if err := os.Chmod(dirs[i].path, dirs[i].mode); err != nil { + return fmt.Errorf("chmod %s: %w", dirs[i].path, err) + } + } + return nil +} + +// createFile opens target for writing, forcing the owner-write bit on so a +// read-only archived mode doesn't stop us writing the content. The caller +// chmods to the archived mode afterwards. A read-only file left behind by +// an earlier extraction is replaced rather than treated as an error — +// re-restoring over a populated module cache is a normal CI situation. +func createFile(target string, mode fs.FileMode) (*os.File, error) { + f, err := os.OpenFile(target, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, mode|0o200) + if err == nil || !errors.Is(err, fs.ErrPermission) { + return f, err + } + if rmErr := os.Remove(target); rmErr != nil { + // The parent directory may itself be read-only; widen it and + // retry once before giving up with the original error. + parent := filepath.Dir(target) + st, stErr := os.Stat(parent) + if stErr != nil { + return nil, err + } + if chErr := os.Chmod(parent, st.Mode().Perm()|0o700); chErr != nil { + return nil, err + } + if rmErr := os.Remove(target); rmErr != nil { + return nil, err + } + } + return os.OpenFile(target, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, mode|0o200) +} + // safeJoin returns base/sub, rejecting any sub that is absolute or contains // ".." segments that would resolve outside base. We reject rather than // silently re-root so malicious tarballs surface as an error. diff --git a/internal/archive/archive_test.go b/internal/archive/archive_test.go index 7e9e81d9c0ebeb093299a9afaca20e955081af71..d3ad43b7bb1b159be21f7323851741b0f3350ee8 100644 --- a/internal/archive/archive_test.go +++ b/internal/archive/archive_test.go @@ -2,6 +2,7 @@ package archive import ( "bytes" + "io/fs" "os" "path/filepath" "testing" @@ -43,6 +44,115 @@ func TestRoundTripDir(t *testing.T) { } } +// The Go module cache is 0555 directories holding 0444 files, and it is +// the single most common thing this package is pointed at. Restoring the +// archived modes as each entry lands locks the extraction out of the tree +// it is still filling in — CI build #295 died on exactly that. +func TestRoundTripReadOnlyTree(t *testing.T) { + src := t.TempDir() + pkg := filepath.Join(src, "mod", "example.com", "pkg@v1.0.0") + mustWrite(t, filepath.Join(pkg, "go.mod"), "module example.com/pkg") + mustWrite(t, filepath.Join(pkg, "doc.go"), "package pkg") + // Tighten from the leaves up, or the chmods can't reach the children. + for _, p := range []string{ + filepath.Join(pkg, "go.mod"), filepath.Join(pkg, "doc.go"), + } { + if err := os.Chmod(p, 0o444); err != nil { + t.Fatal(err) + } + } + for _, d := range []string{pkg, filepath.Dir(pkg), filepath.Join(src, "mod")} { + if err := os.Chmod(d, 0o555); err != nil { + t.Fatal(err) + } + } + unlockAfter(t, src) + + var buf bytes.Buffer + if err := EncodeDir(&buf, src); err != nil { + t.Fatalf("EncodeDir: %v", err) + } + + dest := t.TempDir() + unlockAfter(t, dest) + if err := DecodeDir(&buf, dest); err != nil { + t.Fatalf("DecodeDir into a fresh dir: %v", err) + } + + rel := "mod/example.com/pkg@v1.0.0" + got, err := os.ReadFile(filepath.Join(dest, rel, "doc.go")) + if err != nil { + t.Fatalf("read restored file: %v", err) + } + if string(got) != "package pkg" { + t.Errorf("doc.go = %q", got) + } + // The archived modes must survive, not just the bytes: `go` refuses to + // use a module cache it can write to. + assertMode(t, filepath.Join(dest, rel), 0o555) + assertMode(t, filepath.Join(dest, rel, "doc.go"), 0o444) +} + +// Restoring over a populated, read-only cache is normal in CI: the second +// extraction must replace the read-only files rather than fail on them. +func TestDecodeOverExistingReadOnlyTree(t *testing.T) { + src := t.TempDir() + mustWrite(t, filepath.Join(src, "sub/f.txt"), "content") + if err := os.Chmod(filepath.Join(src, "sub/f.txt"), 0o444); err != nil { + t.Fatal(err) + } + if err := os.Chmod(filepath.Join(src, "sub"), 0o555); err != nil { + t.Fatal(err) + } + unlockAfter(t, src) + + var first, second bytes.Buffer + if err := EncodeDir(&first, src); err != nil { + t.Fatalf("EncodeDir: %v", err) + } + second.Write(first.Bytes()) + + dest := t.TempDir() + unlockAfter(t, dest) + if err := DecodeDir(&first, dest); err != nil { + t.Fatalf("first DecodeDir: %v", err) + } + if err := DecodeDir(&second, dest); err != nil { + t.Fatalf("second DecodeDir over the restored tree: %v", err) + } + got, err := os.ReadFile(filepath.Join(dest, "sub/f.txt")) + if err != nil || string(got) != "content" { + t.Errorf("f.txt = %q, err=%v", got, err) + } + assertMode(t, filepath.Join(dest, "sub/f.txt"), 0o444) +} + +func assertMode(t *testing.T, path string, want os.FileMode) { + t.Helper() + st, err := os.Stat(path) + if err != nil { + t.Fatalf("stat %s: %v", path, err) + } + if got := st.Mode().Perm(); got != want { + t.Errorf("%s mode = %#o, want %#o", path, got, want) + } +} + +// unlockAfter makes a read-only tree removable again, so t.TempDir's +// cleanup doesn't fail on the very permissions the test is about. +func unlockAfter(t *testing.T, root string) { + t.Helper() + t.Cleanup(func() { + _ = filepath.WalkDir(root, func(p string, d fs.DirEntry, err error) error { + if err != nil { + return nil + } + _ = os.Chmod(p, 0o700) + return nil + }) + }) +} + func TestDecodeRejectsPathEscape(t *testing.T) { // Hand-craft a tarball with a "../evil" entry, then zstd it via EncodeDir // indirection isn't feasible (EncodeDir won't emit ..). Instead, write