From 0b0ae2423af5a98e28d248c0e1de5f88760ff621 Mon Sep 17 00:00:00 2001 From: Feng Ruohang Date: Tue, 25 Aug 2026 08:20:23 +0800 Subject: [PATCH] fix: keep copy metadata consistent with a rewritten null version CopyObjectHandler recorded the source compression metadata whenever the copy was metadata-only, on the assumption that the object layer would then leave the stored bytes alone. That assumption does not hold. Both erasureServerPools.CopyObject and erasureSets.CopyObject only skip a data rewrite in three cases, and otherwise fall back to a full PutObject. The reachable gap is a copy whose source is a null version on a bucket that gained versioning after the object was written. Neither version ID is set, so the self-referential version branch is skipped, the data is rewritten as plaintext, and the preserved compression metadata then described bytes that no longer exist. A subsequent GET failed with "s2: corrupt input". Mirror the object layer's decision in copyRewritesObjectData and record the compression metadata from it, so the metadata always describes whichever bytes are finally stored. The source version selection that lets a versioned metadata-only copy add a self-referential version moves next to the same decision, since both depend on the effective metadata-only value. Signed-off-by: Feng Ruohang Co-Authored-By: Claude Opus 5 (1M context) --- cmd/object-copy-metadata_test.go | 119 +++++++++++++++++++++++++++++++ cmd/object-handlers.go | 46 ++++++++++-- 2 files changed, 158 insertions(+), 7 deletions(-) diff --git a/cmd/object-copy-metadata_test.go b/cmd/object-copy-metadata_test.go index 399f192ee..d3f81b774 100644 --- a/cmd/object-copy-metadata_test.go +++ b/cmd/object-copy-metadata_test.go @@ -202,3 +202,122 @@ func testAPICopyObjectSSECKeyRotationKeepsCompressionState(obj ObjectLayer, inst instanceType, response.Code, response.Body.Len(), len(data), response.Body.String()) } } + +// TestAPICopyObjectMetadataOnlyNullVersion covers the copy whose source is a +// null version on a bucket that gained versioning after the object was written. +// The object layer cannot reference such a version, so it rewrites the data and +// the recorded compression metadata has to describe the rewritten bytes. +func TestAPICopyObjectMetadataOnlyNullVersion(t *testing.T) { + defer DetectTestLeak(t)() + ExecObjectLayerAPITest(ExecObjectLayerAPITestArgs{ + t: t, + objAPITest: testAPICopyObjectMetadataOnlyNullVersion, + endpoints: []string{"CopyObject", "PutObject", "GetObject"}, + }) +} + +func testAPICopyObjectMetadataOnlyNullVersion(obj ObjectLayer, instanceType, bucketName string, + apiRouter http.Handler, credentials auth.Credentials, t *testing.T, +) { + restoreCompression := setCopyChecksumCompression(true) + compressionRestored := false + defer func() { + if !compressionRestored { + restoreCompression() + } + }() + + data := bytes.Repeat([]byte("null-version-metadata-copy-"), 64*1024) + want := mustChecksum(t, hash.ChecksumCRC32, data) + object := "copy-metadata/null-version.txt" + putCopyChecksumSource(t, apiRouter, credentials, bucketName, object, data, + map[string]string{xhttp.AmzChecksumCRC32: want}) + + before, err := obj.GetObjectInfo(t.Context(), bucketName, object, ObjectOptions{}) + if err != nil { + t.Fatal(err) + } + if !before.IsCompressed() || before.VersionID != "" { + t.Fatalf("%s: invalid null-version precondition: compressed=%v versionID=%q", + instanceType, before.IsCompressed(), before.VersionID) + } + + // Versioning is enabled after the write, so the object keeps a null version. + if _, err := globalBucketMetadataSys.Update(t.Context(), bucketName, + bucketVersioningConfig, enabledBucketVersioningConfig); err != nil { + t.Fatalf("%s: unable to enable versioning: %v", instanceType, err) + } + if !globalBucketVersioningSys.PrefixEnabled(bucketName, object) { + t.Fatalf("%s: versioning did not become enabled", instanceType) + } + + // Without compression the rewritten destination stores plaintext. + restoreCompression() + compressionRestored = true + + rec := copyChecksumRequest(t, apiRouter, credentials, bucketName, object, object, + map[string]string{xhttp.AmzMetadataDirective: "REPLACE"}) + if rec.Code != http.StatusOK { + t.Fatalf("%s: metadata-only CopyObject failed: %d %s", instanceType, rec.Code, rec.Body.String()) + } + + after := assertCopyChecksum(t, obj, bucketName, object, hash.ChecksumCRC32, data, false, nil) + if after.VersionID == "" { + t.Fatalf("%s: versioned copy did not create a new version", instanceType) + } + if got := readCopyChecksumObject(t, obj, bucketName, object, ObjectOptions{}); !bytes.Equal(got, data) { + t.Fatalf("%s: copied object body differs: got %d bytes, want %d", instanceType, len(got), len(data)) + } +} + +func TestCopyRewritesObjectData(t *testing.T) { + tests := []struct { + name string + metadataOnly bool + srcOpts ObjectOptions + dstOpts ObjectOptions + want bool + }{ + { + name: "data copy always rewrites", + want: true, + }, + { + name: "unversioned in-place metadata update", + metadataOnly: true, + }, + { + name: "addressed version updated in place", + metadataOnly: true, + srcOpts: ObjectOptions{VersionID: "v1"}, + dstOpts: ObjectOptions{VersionID: "v1"}, + }, + { + name: "versioned self referential version", + metadataOnly: true, + srcOpts: ObjectOptions{VersionID: "v1"}, + dstOpts: ObjectOptions{Versioned: true}, + }, + { + name: "versioned null source version cannot be referenced", + metadataOnly: true, + dstOpts: ObjectOptions{Versioned: true}, + want: true, + }, + { + name: "suspended destination with an addressed source version", + metadataOnly: true, + srcOpts: ObjectOptions{VersionID: "v1"}, + dstOpts: ObjectOptions{VersionSuspended: true, VersionID: nullVersionID}, + want: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := copyRewritesObjectData(tt.metadataOnly, tt.srcOpts, tt.dstOpts); got != tt.want { + t.Fatalf("copyRewritesObjectData() = %v, want %v", got, tt.want) + } + }) + } +} diff --git a/cmd/object-handlers.go b/cmd/object-handlers.go index 649136227..d25e99b5c 100644 --- a/cmd/object-handlers.go +++ b/cmd/object-handlers.go @@ -1179,6 +1179,31 @@ func isRemoteCallRequired(ctx context.Context, bucket string, objAPI ObjectLayer return false } +// copyRewritesObjectData reports whether the object layer stores new object data +// for this copy instead of updating metadata in place or adding a +// self-referential version. It mirrors the metadata-only decision taken by +// erasureServerPools.CopyObject and erasureSets.CopyObject. CopyObjectHandler +// has to predict that decision because the compression metadata it records must +// describe whichever bytes are finally stored. metadataOnly already excludes +// legacy sources, which the object layer always rewrites. +func copyRewritesObjectData(metadataOnly bool, srcOpts, dstOpts ObjectOptions) bool { + if !metadataOnly { + return true + } + switch { + case dstOpts.VersionID != "" && srcOpts.VersionID == dstOpts.VersionID: + // In-place update of the addressed version. + return false + case !dstOpts.Versioned && srcOpts.VersionID == "": + // In-place update of an unversioned object. + return false + case dstOpts.Versioned && srcOpts.VersionID != dstOpts.VersionID: + // A new version referencing the existing data. + return false + } + return true +} + // CopyObjectHandler - Copy Object // ---------- // This implementation of the PUT operation adds an object to a bucket @@ -1708,8 +1733,20 @@ func (api objectAPIHandlers) CopyObjectHandler(w http.ResponseWriter, r *http.Re srcInfo.UserDefined[ReservedMetadataPrefixLower+ReplicationStatus] = dsc.PendingStatus() srcInfo.UserDefined[ReservedMetadataPrefixLower+ReplicationTimestamp] = UTCNow().Format(time.RFC3339Nano) } - // Compression metadata must describe data that is actually rewritten. - if !srcInfo.metadataOnly || srcInfo.Legacy || dstOpts.WantServerSideChecksumType.IsSet() { + // srcInfo.metadataOnly is still cleared below for legacy sources and for + // server-side checksum recomputation; both of those rewrite the object data. + metadataOnly := srcInfo.metadataOnly && !srcInfo.Legacy && !dstOpts.WantServerSideChecksumType.IsSet() + + // Name the source version explicitly so a metadata-only copy into a + // versioned bucket adds a self-referential version instead of rewriting the + // object data. A null source version cannot be referenced this way. + copySrcOpts := srcOpts + if metadataOnly && dstOpts.Versioned && copySrcOpts.VersionID == "" { + copySrcOpts.VersionID = srcInfo.VersionID + } + + // Compression metadata must describe the bytes that are actually stored. + if copyRewritesObjectData(metadataOnly, copySrcOpts, dstOpts) { if isDstCompressed { maps.Copy(srcInfo.UserDefined, compressMetadata) } else { @@ -1808,11 +1845,6 @@ func (api objectAPIHandlers) CopyObjectHandler(w http.ResponseWriter, r *http.Re copyObjectFn := objectAPI.CopyObject - copySrcOpts := srcOpts - if srcInfo.metadataOnly && dstOpts.Versioned && copySrcOpts.VersionID == "" { - copySrcOpts.VersionID = srcInfo.VersionID - } - // Copy source object to destination, if source and destination // object is same then only metadata is updated. objInfo, err = copyObjectFn(ctx, srcBucket, srcObject, dstBucket, dstObject, srcInfo, copySrcOpts, dstOpts)