fix: nil-pointer guards in metadata, retry-ID and mod-time handling - #880
Open
terryrankine wants to merge 1 commit into
Open
terryrankine wants to merge 1 commit into
terryrankine wants to merge 1 commit into
Conversation
- storage/s3: Copy() wrote the retry ID into a nil Metadata map when --no-such-upload-retry-count > 0, panicking with "assignment to entry in nil map". Initialise the map first. - storage/s3: user-defined metadata replaced the whole Metadata map in Copy() and Put(), silently dropping the retry ID set just above it. Merge into the existing map instead. - storage/s3: Stat() and retryOnNoSuchUpload() dereferenced the retry-ID metadata value without a nil check. - command/cp: --if-source-newer dereferenced ModTime without a nil check. Treat a missing modification time as "source is newer" so the copy proceeds. Comparison extracted to isSourceNewer() so it can be tested. - parallel: Run() before Init() panicked with a bare nil-pointer dereference. Panic with an explicit message instead. - command/validation: the "expected at most N arguments" error reported min instead of max. - e2e: setup() assigned opts.accessKeyID to region instead of opts.region, so tests with a custom region silently used the access key ID as the region. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
3 of 7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Context
A handful of small, independent nil-pointer and correctness fixes found while auditing the codebase. Each has a unit test that fails without it.
Fixes
storage/s3.goCopy():input.Metadatawas nil when--no-such-upload-retry-countadded the retry ID → "assignment to entry in nil map" panic. Also, user-defined metadata (--metadata) replaced the map and dropped the retry ID; it is now merged.storage/s3.goStat() / retryOnNoSuchUpload(): guard*retryIDagainst a nil map value.command/cp.goshouldOverride():--if-source-newerdereferencedModTimewithout a nil check. ExtractedisSourceNewer(); a missing mod time is treated as "copy".command/validation.go: the "expected at most N arguments" message printedmininstead ofmax.parallel/global.go:Run()beforeInit()now panics with a clear message instead of a nil-pointer dereference.e2e/util_test.go:setup()assignedopts.accessKeyIDto the region.Tests
TestCheckNumberOfArguments,TestRunPanicsWithoutInit,TestIsSourceNewer,TestS3CopyRetryIDWithUserMetadata,TestS3PutRetryIDWithUserMetadata,TestS3StatNilRetryID. Each was verified to fail with the corresponding fix reverted.Linux, Go 1.24,
go test -count=1 -race ./...: all packagesok.gofmt,go vet,staticcheck,unparam,semgrep: clean.🤖 Generated with Claude Code