blob/memblob: fix Copy aliasing attributes between source and destination#3727
Conversation
Copy was assigning the source *blobEntry pointer directly to the destination key. Two bugs follow from that. First, the src and dst entries shared the same *driver.Attributes pointer and the same Metadata map. Any in-place mutation of the attributes or metadata through one key would silently corrupt the other. Second, when copying onto an existing key, the destination's original CreateTime was replaced with the source's CreateTime. The reference implementation (fileblob) routes all copies through NewTypedWriter, which preserves the existing CreateTime of any blob being overwritten. memblob did not do the same. The fix deep-copies the Attributes struct and Metadata map for the new destination entry, and carries forward the destination's previous CreateTime when it already exists. Two new unit tests cover both cases: TestCopyPreservesDestinationCreateTime and TestCopyDoesNotAliasEntry.
|
|
||
| // Preserve the original CreateTime of the destination blob if it already | ||
| // exists. This matches the behavior of fileblob (which goes through | ||
| // NewTypedWriter) and real object stores like GCS. |
There was a problem hiding this comment.
Are you sure about this? I tried adding a drivertest for this; the existing TestCopy already mostly does the setup here (e.g., copying onto an existing key). See #3729.
As you suggest in this PR, it fails for memblob and passes for fileblob, but interestingly it also passes for s3blob but fails for gcsblob. This seems to imply that this behavior is not consistent across blob providers and shouldn't be relied on.
…rivertest Per review, copying onto an existing key does not preserve the destination's CreateTime consistently across providers, so Copy no longer tries to preserve it; it now only deep-copies the source's attributes and metadata so the two entries are independent. The memblob-specific tests are replaced by a drivertest subtest under testCopy that makes a blob, copies it, mutates the source's attributes through the driver, and verifies the copy is unchanged. It fails for memblob without the fix and passes with it. It is commented out for now because enabling it requires regenerating the record/replay files for providers like S3 and GCS.
|
Thanks for testing this across providers, and for putting together #3729. I dropped the CreateTime preservation from Copy since the behavior clearly isn't consistent enough to rely on, and removed both memblob-specific tests. The aliasing check is now a drivertest subtest under testCopy. It writes a blob with metadata, copies it, mutates the source's attributes through the driver, and verifies the copy is unchanged. I confirmed locally that it fails for memblob without the fix and passes with it, and that it passes for fileblob in both conformance configurations (the metadata mutation is guarded because the MetadataDontWrite configuration returns no metadata map). Per your note about the S3 and GCS record/replay files, the subtest is commented out in drivertest.go. I also updated the PR description to match. |
memblob.Copy was assigning the source *blobEntry pointer directly to the destination key. After Copy(dst, src), both keys store the same *blobEntry, which holds a *driver.Attributes pointer and a Metadata map. Mutating the attributes of one key (the driver's Attributes method returns the stored pointer) silently changes the other.
The fix makes Copy deep-copy the Attributes struct and Metadata map into a fresh blobEntry for the destination.
The aliasing is covered by a new drivertest subtest, TestCopy/DoesNotAliasAttributes. It writes a blob with metadata, copies it, mutates the source's attributes through the driver, and verifies the copy is unchanged. It fails for memblob without this fix and passes with it, and it passes for fileblob in both conformance configurations. The subtest is commented out in drivertest.go for now since enabling it requires regenerating the record/replay files for providers like S3 and GCS.
An earlier version of this PR also made Copy preserve the destination's CreateTime when copying onto an existing key. That was dropped after review since the behavior is not consistent across providers.