diff --git a/internal/storage/blob.go b/internal/storage/blob.go index 97e50f3f..4a4bbc52 100644 --- a/internal/storage/blob.go +++ b/internal/storage/blob.go @@ -22,11 +22,19 @@ import ( const osWindows = "windows" +// attrsExt is fileblob's sidecar suffix, kept only to clear sidecars an +// earlier version wrote. +const attrsExt = ".attrs" + // Blob implements Storage using gocloud.dev/blob. // Supports local filesystem (file://) and S3 (s3://) URLs. type Blob struct { bucket *blob.Bucket url string + + // fileRoot is the directory backing a file:// bucket, empty for cloud + // backends. Used only to clear sidecars an earlier version wrote. + fileRoot string } // OpenBucket opens a blob bucket from a URL. @@ -47,6 +55,8 @@ func OpenBucket(ctx context.Context, urlStr string) (Storage, error) { return OpenGCS(ctx, urlStr) } + var fileRoot string + // Handle file:// URLs specially to create the directory if strings.HasPrefix(urlStr, "file://") { path := strings.TrimPrefix(urlStr, "file://") @@ -74,6 +84,8 @@ func OpenBucket(ctx context.Context, urlStr string) (Storage, error) { return nil, fmt.Errorf("resolving path: %w", err) } + fileRoot = absPath + // Convert back to URL format with forward slashes urlPath := filepath.ToSlash(absPath) if runtime.GOOS == osWindows { @@ -87,7 +99,14 @@ func OpenBucket(ctx context.Context, urlStr string) (Storage, error) { // This avoids "invalid cross-device link" errors from os.Rename when // the bucket directory and os.TempDir are on different filesystems // (e.g. Docker volume mounts). - urlStr += "?no_tmp_dir=true" + // + // Do not write fileblob's ".attrs" sidecar. It is rewritten with + // os.Create, truncating in place outside the atomic rename that + // protects the blob, so a read overlapping a write can decode a + // partial file; a missing one defaults cleanly, a truncated one does + // not. Nothing in the proxy needs it: Store sets no ContentType, and + // Size reads os.Stat via Attributes. + urlStr += "?no_tmp_dir=true&metadata=skip" } bucket, err := blob.OpenBucket(ctx, urlStr) @@ -95,10 +114,38 @@ func OpenBucket(ctx context.Context, urlStr string) (Storage, error) { return nil, fmt.Errorf("opening bucket: %w", err) } - return &Blob{bucket: bucket, url: urlStr}, nil + return &Blob{bucket: bucket, url: urlStr, fileRoot: fileRoot}, nil +} + +// legacySidecarPath gives the ".attrs" path an earlier version wrote for key, +// or "" when the mapping is not certain. +// +// fileblob maps keys with an unexported escapeKey, so this derives it. +// escapeKey is the identity for a plain key and parts from one only for keys +// that are not valid local paths, which is what filepath.Localize rejects. +// Declining those keeps the removal inside fileRoot too: a key holding ".." +// would otherwise resolve outside the cache. A control character is the one +// case Localize accepts and escapeKey does not, where removal simply misses. +func (b *Blob) legacySidecarPath(key string) string { + if b.fileRoot == "" { + return "" + } + rel, err := filepath.Localize(key) + if err != nil { + return "" + } + return filepath.Join(b.fileRoot, rel) + attrsExt } func (b *Blob) Store(ctx context.Context, path string, r io.Reader) (int64, string, error) { + // Drop any sidecar an earlier version left for this key. Nothing rewrites + // one now, so a partial sidecar from an interrupted write would fail every + // read of the key for good. Removal is atomic where the rewrite was not, + // so a concurrent reader gets the whole old file or nothing. + if sidecar := b.legacySidecarPath(path); sidecar != "" { + _ = os.Remove(sidecar) + } + // Compute hash while writing h := sha256.New() tee := io.TeeReader(r, h) diff --git a/internal/storage/blob_test.go b/internal/storage/blob_test.go index 3e5bf65b..bef61d4d 100644 --- a/internal/storage/blob_test.go +++ b/internal/storage/blob_test.go @@ -6,11 +6,17 @@ import ( "encoding/hex" "errors" "io" + "io/fs" + "os" "path/filepath" "runtime" "strings" + "sync" + "sync/atomic" "testing" "time" + + "gocloud.dev/blob" ) func TestOpenBucket(t *testing.T) { @@ -293,3 +299,267 @@ func fileURLFromPath(path string) string { } return "file://" + path } + +func TestOpenBucketWritesNoAttrsSidecar(t *testing.T) { + dir := t.TempDir() + ctx := context.Background() + + b, err := OpenBucket(ctx, fileURLFromPath(dir)) + if err != nil { + t.Fatalf("OpenBucket failed: %v", err) + } + defer func() { _ = b.Close() }() + + if _, _, err := b.Store(ctx, "pkg/thing-1.0.0.tgz", strings.NewReader("content")); err != nil { + t.Fatalf("Store failed: %v", err) + } + + sidecars, err := filepath.Glob(filepath.Join(dir, "*", "*.attrs")) + if err != nil { + t.Fatalf("Glob failed: %v", err) + } + if len(sidecars) != 0 { + t.Errorf("got sidecar files %v, want none: a truncated sidecar fails reads that overlap a write", sidecars) + } +} + +// A read overlapping a write to the same key must not fail. fileblob rewrote +// its ".attrs" sidecar in place, so a reader decoding it mid-write saw a +// partial file, which the proxy served as a 502 on an artifact it held. +func TestConcurrentReadsSurviveWritesToSameKey(t *testing.T) { + const ( + key = "pkg/thing-1.0.0.tgz" + readers = 4 + readsPerRead = 500 + ) + dir := t.TempDir() + ctx := context.Background() + + b, err := OpenBucket(ctx, fileURLFromPath(dir)) + if err != nil { + t.Fatalf("OpenBucket failed: %v", err) + } + defer func() { _ = b.Close() }() + + payload := strings.Repeat("x", 4096) + if _, _, err := b.Store(ctx, key, strings.NewReader(payload)); err != nil { + t.Fatalf("seeding Store failed: %v", err) + } + + done := make(chan struct{}) + var writers sync.WaitGroup + writers.Add(1) + go func() { + defer writers.Done() + for { + select { + case <-done: + return + default: + } + if _, _, err := b.Store(ctx, key, strings.NewReader(payload)); err != nil { + return + } + } + }() + + var failures atomic.Int64 + var reading sync.WaitGroup + for range readers { + reading.Add(1) + go func() { + defer reading.Done() + for range readsPerRead { + r, err := b.Open(ctx, key) + if err != nil { + failures.Add(1) + continue + } + if _, err := io.Copy(io.Discard, r); err != nil { + failures.Add(1) + } + _ = r.Close() + } + }() + } + reading.Wait() + close(done) + writers.Wait() + + if got := failures.Load(); got != 0 { + t.Errorf("%d of %d reads failed while one writer rewrote the same key, want 0", got, readers*readsPerRead) + } +} + +// seedLegacySidecar stores key through a bucket that still writes sidecars, as +// an earlier version did, and returns the path fileblob actually used. It is +// discovered rather than assumed, so callers test the real mapping. +func seedLegacySidecar(t *testing.T, dir, key, payload string) string { + t.Helper() + ctx := context.Background() + + legacy, err := blob.OpenBucket(ctx, fileURLFromPath(dir)+"?no_tmp_dir=true") + if err != nil { + t.Fatalf("opening legacy bucket: %v", err) + } + if err := legacy.WriteAll(ctx, key, []byte(payload), nil); err != nil { + t.Fatalf("legacy WriteAll: %v", err) + } + if err := legacy.Close(); err != nil { + t.Fatalf("closing legacy bucket: %v", err) + } + + var found []string + walkErr := filepath.WalkDir(dir, func(path string, d fs.DirEntry, err error) error { + if err != nil { + return err + } + if !d.IsDir() && strings.HasSuffix(path, ".attrs") { + found = append(found, path) + } + return nil + }) + if walkErr != nil { + t.Fatalf("walking %s: %v", dir, walkErr) + } + if len(found) != 1 { + t.Fatalf("got sidecars %v, want exactly one", found) + } + return found[0] +} + +// An interrupted setAttrs leaves a partial sidecar that fails every read of the +// key, and nothing rewrites one now, so a store has to clear it. +// +// One key per storage path the proxy builds: ArtifactPath across ecosystems, +// metadata blobs, and the Gradle build cache. Scoped npm names, Go's "!" case +// escaping and the ":" in OCI digests and Debian epochs are the characters +// most likely to part fileblob's mapping from a plain path join. +func TestStoreClearsLegacyAttrsSidecar(t *testing.T) { + keys := []string{ + "npm/@babel/core/7.24.0/core-7.24.0.tgz", + "maven/org.apache.commons/commons-lang3/3.14.0/commons-lang3-3.14.0.jar", + "golang/github.com/!burnt!sushi/toml/v1.3.2/v1.3.2.zip", + "oci/library/nginx/sha256:abc123def456/manifest", + "debian/tzdata/1:2024a-1/tzdata_2024a-1_all.deb", + "pypi/requests/2.31.0/requests-2.31.0-py3-none-any.whl", + "cargo/serde/1.0.197/serde-1.0.197.crate", + "julia/Example/a1b2c3/a1b2c3.tar.gz", + "conda/numpy/1.26.4/numpy-1.26.4-py311.conda", + "_metadata/npm/@babel/core/metadata", + "_gradle/http-build-cache/0a1b2c3d4e5f", + } + + for _, key := range keys { + t.Run(key, func(t *testing.T) { + assertStoreClearsSidecar(t, key) + }) + } +} + +func assertStoreClearsSidecar(t *testing.T, key string) { + t.Helper() + const payload = "payload" + ctx := context.Background() + dir := t.TempDir() + + sidecar := seedLegacySidecar(t, dir, key, payload) + if err := os.WriteFile(sidecar, []byte(`{"user.content_type":"appl`), 0o600); err != nil { + t.Fatalf("corrupting sidecar: %v", err) + } + + b := openFileBlob(t, dir) + if _, err := b.Open(ctx, key); err == nil { + t.Fatal("corrupt sidecar did not fail the read, so it is not the file fileblob reads for this key") + } + + derived := b.legacySidecarPath(key) + if _, _, err := b.Store(ctx, key, strings.NewReader(payload)); err != nil { + t.Fatalf("Store failed: %v", err) + } + + if derived == "" { + assertSidecarKept(t, sidecar) + return + } + if derived != sidecar { + t.Fatalf("derived %q, but fileblob wrote %q", derived, sidecar) + } + if _, err := os.Stat(sidecar); !os.IsNotExist(err) { + t.Errorf("sidecar still present after Store, stat err = %v", err) + } + assertReadsBack(t, b, key, payload) +} + +// Windows rejects ":" in a local path and fileblob escapes it, so for those +// keys the mapping is not certain and the sidecar is left alone. +func assertSidecarKept(t *testing.T, sidecar string) { + t.Helper() + if runtime.GOOS != osWindows { + t.Fatalf("declined a key that is a plain local path on %s", runtime.GOOS) + } + if _, err := os.Stat(sidecar); err != nil { + t.Errorf("declined key should keep its sidecar, stat err = %v", err) + } +} + +func assertReadsBack(t *testing.T, b *Blob, key, want string) { + t.Helper() + r, err := b.Open(context.Background(), key) + if err != nil { + t.Fatalf("read still failing after Store cleared the sidecar: %v", err) + } + defer func() { _ = r.Close() }() + got, err := io.ReadAll(r) + if err != nil { + t.Fatalf("ReadAll failed: %v", err) + } + if string(got) != want { + t.Errorf("got %q, want %q", got, want) + } +} + +func openFileBlob(t *testing.T, dir string) *Blob { + t.Helper() + s, err := OpenBucket(context.Background(), fileURLFromPath(dir)) + if err != nil { + t.Fatalf("OpenBucket failed: %v", err) + } + t.Cleanup(func() { _ = s.Close() }) + b, ok := s.(*Blob) + if !ok { + t.Fatalf("got %T, want *Blob", s) + } + return b +} + +// fileblob escapes a non-local key in a way this cannot reproduce, and one +// holding ".." resolves outside the cache directory. Removal declines both +// rather than delete the wrong file. +func TestLegacySidecarPathDeclinesNonLocalKeys(t *testing.T) { + b := &Blob{fileRoot: filepath.FromSlash("/var/cache/proxy")} + + for _, key := range []string{ + "npm/pkg//1.0.0/x.tgz", + "npm/pkg/../../../../etc/passwd", + "npm/pkg/1.0.0/", + "/etc/passwd", + "", + } { + if got := b.legacySidecarPath(key); got != "" { + t.Errorf("legacySidecarPath(%q) = %q, want \"\"", key, got) + } + } + + if got := b.legacySidecarPath("npm/pkg/1.0.0/x.tgz"); got == "" { + t.Error("a plain key must still map to a sidecar path") + } +} + +// Cloud backends have no local directory, so nothing is removed for them. +func TestLegacySidecarPathEmptyForCloudBackends(t *testing.T) { + b := &Blob{} + if got := b.legacySidecarPath("npm/pkg/1.0.0/x.tgz"); got != "" { + t.Errorf("legacySidecarPath = %q, want \"\" when there is no file root", got) + } +}