From 70e588b4b29b6e0e18138e064f1155b267c0175b Mon Sep 17 00:00:00 2001 From: Terry Rankine Date: Thu, 17 Sep 2026 21:04:03 +0200 Subject: [PATCH] fix: nil-pointer guards in metadata, retry-ID and mod-time handling - 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) --- command/cp.go | 19 +++- command/cp_test.go | 66 +++++++++++++ command/validation.go | 2 +- command/validation_test.go | 64 +++++++++++++ e2e/util_test.go | 2 +- parallel/global.go | 8 +- parallel/global_test.go | 27 ++++++ storage/s3.go | 18 ++-- storage/s3_test.go | 191 +++++++++++++++++++++++++++++++++++++ 9 files changed, 381 insertions(+), 16 deletions(-) create mode 100644 command/validation_test.go create mode 100644 parallel/global_test.go diff --git a/command/cp.go b/command/cp.go index 7ed06bfee..827339e6b 100644 --- a/command/cp.go +++ b/command/cp.go @@ -893,18 +893,27 @@ func (c Copy) shouldOverride(ctx context.Context, srcurl *url.URL, dsturl *url.U } if c.ifSourceNewer { - srcMod, dstMod := srcObj.ModTime, dstObj.ModTime - - if !srcMod.After(*dstMod) { - stickyErr = errorpkg.ErrObjectIsNewer - } else { + if isSourceNewer(srcObj, dstObj) { stickyErr = nil + } else { + stickyErr = errorpkg.ErrObjectIsNewer } } return stickyErr } +// isSourceNewer reports whether srcObj was modified after dstObj. If either +// modification time is unavailable, the source is treated as newer so that +// the copy proceeds. +func isSourceNewer(srcObj, dstObj *storage.Object) bool { + srcMod, dstMod := srcObj.ModTime, dstObj.ModTime + if srcMod == nil || dstMod == nil { + return true + } + return srcMod.After(*dstMod) +} + // prepareRemoteDestination will return a new destination URL for // remote->remote and local->remote copy operations. func prepareRemoteDestination( diff --git a/command/cp_test.go b/command/cp_test.go index ccac38ff3..97d06ab4b 100644 --- a/command/cp_test.go +++ b/command/cp_test.go @@ -4,7 +4,9 @@ import ( "io" "os" "testing" + "time" + "github.com/peak/s5cmd/v2/storage" "gotest.tools/v3/assert" ) @@ -77,3 +79,67 @@ func TestGuessContentType(t *testing.T) { os.Remove(f.Name()) } } + +func TestIsSourceNewer(t *testing.T) { + t.Parallel() + + now := time.Now() + earlier := now.Add(-time.Minute) + + testcases := []struct { + name string + src *time.Time + dst *time.Time + expected bool + }{ + { + name: "source newer than destination", + src: &now, + dst: &earlier, + expected: true, + }, + { + name: "source older than destination", + src: &earlier, + dst: &now, + expected: false, + }, + { + name: "same modification time", + src: &now, + dst: &now, + expected: false, + }, + { + name: "source modification time unavailable", + src: nil, + dst: &now, + expected: true, + }, + { + name: "destination modification time unavailable", + src: &now, + dst: nil, + expected: true, + }, + { + name: "both modification times unavailable", + src: nil, + dst: nil, + expected: true, + }, + } + + for _, tc := range testcases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + t.Parallel() + + src := &storage.Object{ModTime: tc.src} + dst := &storage.Object{ModTime: tc.dst} + + got := isSourceNewer(src, dst) + assert.Equal(t, got, tc.expected) + }) + } +} diff --git a/command/validation.go b/command/validation.go index 6530fe82f..7c2e10320 100644 --- a/command/validation.go +++ b/command/validation.go @@ -69,7 +69,7 @@ func checkNumberOfArguments(ctx *cli.Context, min, max int) error { return fmt.Errorf("expected at least %d arguments but was given %d: %q", min, l, ctx.Args().Slice()) } if max >= 0 && l > max { - return fmt.Errorf("expected at most %d arguments but was given %d: %q", min, l, ctx.Args().Slice()) + return fmt.Errorf("expected at most %d arguments but was given %d: %q", max, l, ctx.Args().Slice()) } return nil } diff --git a/command/validation_test.go b/command/validation_test.go new file mode 100644 index 000000000..4b48d4f3b --- /dev/null +++ b/command/validation_test.go @@ -0,0 +1,64 @@ +package command + +import ( + "flag" + "strings" + "testing" + + "github.com/urfave/cli/v2" +) + +func TestCheckNumberOfArguments(t *testing.T) { + tests := []struct { + name string + min int + max int + args []string + wantErr bool + errSubstr string + }{ + { + name: "too many args returns error with max", + min: 1, + max: 3, + args: []string{"a", "b", "c", "d", "e"}, + wantErr: true, + errSubstr: "3", + }, + { + name: "too few args for exact match", + min: 2, + max: 2, + args: []string{"a"}, + wantErr: true, + }, + { + name: "valid number of args", + min: 1, + max: 3, + args: []string{"a", "b"}, + wantErr: false, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + app := cli.NewApp() + set := flag.NewFlagSet("test", flag.ContinueOnError) + set.Parse(tt.args) + ctx := cli.NewContext(app, set, nil) + + err := checkNumberOfArguments(ctx, tt.min, tt.max) + if tt.wantErr { + if err == nil { + t.Errorf("checkNumberOfArguments() expected error but got nil") + } else if tt.errSubstr != "" && !strings.Contains(err.Error(), tt.errSubstr) { + t.Errorf("checkNumberOfArguments() error = %q, want substring %q", err.Error(), tt.errSubstr) + } + } else { + if err != nil { + t.Errorf("checkNumberOfArguments() unexpected error: %v", err) + } + } + }) + } +} diff --git a/e2e/util_test.go b/e2e/util_test.go index 31a8cd162..7b687ebbb 100644 --- a/e2e/util_test.go +++ b/e2e/util_test.go @@ -168,7 +168,7 @@ func setup(t *testing.T, options ...option) (*s3.S3, func(...string) icmd.Cmd) { region := "" if opts.region != "" { - region = opts.accessKeyID + region = opts.region } var cfg *credentialCfg diff --git a/parallel/global.go b/parallel/global.go index 61d117eff..e05f3d96e 100644 --- a/parallel/global.go +++ b/parallel/global.go @@ -20,4 +20,10 @@ func Close() { } // Run runs global ParallelManager. -func Run(task Task, waiter *Waiter) { global.Run(task, waiter) } +// Init must be called before Run, otherwise this will panic. +func Run(task Task, waiter *Waiter) { + if global == nil { + panic("parallel: Run called before Init") + } + global.Run(task, waiter) +} diff --git a/parallel/global_test.go b/parallel/global_test.go new file mode 100644 index 000000000..780a27ef5 --- /dev/null +++ b/parallel/global_test.go @@ -0,0 +1,27 @@ +package parallel + +import "testing" + +func TestRunPanicsWithoutInit(t *testing.T) { + // Ensure global is nil so Run will panic. + old := global + global = nil + defer func() { global = old }() + + defer func() { + r := recover() + if r == nil { + t.Fatal("Run() did not panic") + } + got, ok := r.(string) + if !ok { + t.Fatalf("Run() panicked with non-string value: %v", r) + } + want := "parallel: Run called before Init" + if got != want { + t.Errorf("Run() panic message = %q, want %q", got, want) + } + }() + + Run(nil, nil) +} diff --git a/storage/s3.go b/storage/s3.go index 3313be66e..5dda99211 100644 --- a/storage/s3.go +++ b/storage/s3.go @@ -144,7 +144,7 @@ func (s *S3) Stat(ctx context.Context, url *url.URL) (*Object, error) { } if s.noSuchUploadRetryCount > 0 { - if retryID, ok := output.Metadata[metadataKeyRetryID]; ok { + if retryID, ok := output.Metadata[metadataKeyRetryID]; ok && retryID != nil { obj.retryID = *retryID } } @@ -545,6 +545,9 @@ func (s *S3) Copy(ctx context.Context, from, to *url.URL, metadata Metadata) err // add retry ID to the object metadata if s.noSuchUploadRetryCount > 0 { + if input.Metadata == nil { + input.Metadata = make(map[string]*string) + } input.Metadata[metadataKeyRetryID] = generateRetryID() } @@ -557,11 +560,12 @@ func (s *S3) Copy(ctx context.Context, from, to *url.URL, metadata Metadata) err } if len(metadata.UserDefined) != 0 { - m := make(map[string]*string) + if input.Metadata == nil { + input.Metadata = make(map[string]*string) + } for k, v := range metadata.UserDefined { - m[k] = aws.String(v) + input.Metadata[k] = aws.String(v) } - input.Metadata = m } _, err := s.api.CopyObject(input) @@ -866,11 +870,9 @@ func (s *S3) Put( } if len(metadata.UserDefined) != 0 { - m := make(map[string]*string) for k, v := range metadata.UserDefined { - m[k] = aws.String(v) + input.Metadata[k] = aws.String(v) } - input.Metadata = m } uploaderOptsFn := func(u *s3manager.Uploader) { @@ -890,7 +892,7 @@ func (s *S3) retryOnNoSuchUpload(ctx aws.Context, to *url.URL, input *s3manager. err error, uploaderOpts ...func(*s3manager.Uploader), ) error { var expectedRetryID string - if ID, ok := input.Metadata[metadataKeyRetryID]; ok { + if ID, ok := input.Metadata[metadataKeyRetryID]; ok && ID != nil { expectedRetryID = *ID } diff --git a/storage/s3_test.go b/storage/s3_test.go index 11b52ebc3..2f721e171 100644 --- a/storage/s3_test.go +++ b/storage/s3_test.go @@ -754,6 +754,197 @@ func TestS3CopyEncryptionRequest(t *testing.T) { } } +func TestS3CopyRetryIDWithUserMetadata(t *testing.T) { + testcases := []struct { + name string + retryCount int + userDefined map[string]string + + expectRetryID bool + }{ + { + name: "retry ID without user metadata", + retryCount: 1, + + expectRetryID: true, + }, + { + name: "retry ID merged with user metadata", + retryCount: 1, + userDefined: map[string]string{"foo": "bar"}, + + expectRetryID: true, + }, + { + name: "user metadata without retry ID", + userDefined: map[string]string{"foo": "bar"}, + }, + } + + u, err := url.New("s3://bucket/key") + if err != nil { + t.Errorf("unexpected error: %v", err) + } + for _, tc := range testcases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + mockAPI := s3.New(unit.Session) + + mockAPI.Handlers.Unmarshal.Clear() + mockAPI.Handlers.UnmarshalMeta.Clear() + mockAPI.Handlers.UnmarshalError.Clear() + mockAPI.Handlers.Send.Clear() + + mockAPI.Handlers.Send.PushBack(func(r *request.Request) { + r.HTTPResponse = &http.Response{ + StatusCode: http.StatusOK, + Body: io.NopCloser(strings.NewReader("")), + } + + got := r.Params.(*s3.CopyObjectInput).Metadata + assertRequestMetadata(t, got, tc.userDefined, tc.expectRetryID) + }) + mockAPI.Handlers.Unmarshal.PushBack(func(r *request.Request) { + if r.Error != nil { + if awsErr, ok := r.Error.(awserr.Error); ok { + if awsErr.Code() == request.ErrCodeSerialization { + r.Error = nil + } + } + } + }) + + mockS3 := &S3{ + api: mockAPI, + noSuchUploadRetryCount: tc.retryCount, + } + + err = mockS3.Copy(context.Background(), u, u, Metadata{UserDefined: tc.userDefined}) + if err != nil { + t.Errorf("Expected %v, but received %q", nil, err) + } + }) + } +} + +func TestS3PutRetryIDWithUserMetadata(t *testing.T) { + testcases := []struct { + name string + retryCount int + userDefined map[string]string + + expectRetryID bool + }{ + { + name: "retry ID without user metadata", + retryCount: 1, + + expectRetryID: true, + }, + { + name: "retry ID merged with user metadata", + retryCount: 1, + userDefined: map[string]string{"foo": "bar"}, + + expectRetryID: true, + }, + { + name: "user metadata without retry ID", + userDefined: map[string]string{"foo": "bar"}, + }, + } + + u, err := url.New("s3://bucket/key") + if err != nil { + t.Errorf("unexpected error: %v", err) + } + for _, tc := range testcases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + mockAPI := s3.New(unit.Session) + + mockAPI.Handlers.Unmarshal.Clear() + mockAPI.Handlers.UnmarshalMeta.Clear() + mockAPI.Handlers.UnmarshalError.Clear() + mockAPI.Handlers.Send.Clear() + + mockAPI.Handlers.Send.PushBack(func(r *request.Request) { + r.HTTPResponse = &http.Response{ + StatusCode: http.StatusOK, + Body: io.NopCloser(strings.NewReader("")), + } + + got := r.Params.(*s3.PutObjectInput).Metadata + assertRequestMetadata(t, got, tc.userDefined, tc.expectRetryID) + }) + + mockS3 := &S3{ + uploader: s3manager.NewUploaderWithClient(mockAPI), + noSuchUploadRetryCount: tc.retryCount, + } + + err = mockS3.Put(context.Background(), bytes.NewReader([]byte("")), u, Metadata{UserDefined: tc.userDefined}, 1, 5242880) + if err != nil { + t.Errorf("Expected %v, but received %q", nil, err) + } + }) + } +} + +// assertRequestMetadata checks that the request metadata contains every +// user-defined key and, if expected, a non-empty retry ID. +func assertRequestMetadata(t *testing.T, got map[string]*string, userDefined map[string]string, expectRetryID bool) { + t.Helper() + + for k, v := range userDefined { + assert.Equal(t, aws.StringValue(got[k]), v) + } + + retryID, ok := got[metadataKeyRetryID] + if expectRetryID { + assert.Assert(t, ok, "retry ID missing from request metadata") + assert.Assert(t, aws.StringValue(retryID) != "", "retry ID is empty") + } else { + assert.Assert(t, !ok, "unexpected retry ID in request metadata") + } +} + +func TestS3StatNilRetryID(t *testing.T) { + u, err := url.New("s3://bucket/key") + if err != nil { + t.Errorf("unexpected error: %v", err) + } + + mockAPI := s3.New(unit.Session) + mockS3 := &S3{ + api: mockAPI, + noSuchUploadRetryCount: 1, + } + + mockAPI.Handlers.Send.Clear() + mockAPI.Handlers.Unmarshal.Clear() + mockAPI.Handlers.UnmarshalMeta.Clear() + mockAPI.Handlers.ValidateResponse.Clear() + + mockAPI.Handlers.Send.PushBack(func(r *request.Request) { + r.HTTPResponse = &http.Response{ + StatusCode: http.StatusOK, + Body: io.NopCloser(strings.NewReader("")), + } + }) + mockAPI.Handlers.Unmarshal.PushBack(func(r *request.Request) { + r.Data.(*s3.HeadObjectOutput).Metadata = map[string]*string{ + metadataKeyRetryID: nil, + } + }) + + obj, err := mockS3.Stat(context.Background(), u) + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + assert.Equal(t, obj.retryID, "") +} + func TestS3PutEncryptionRequest(t *testing.T) { testcases := []struct { name string