fix: nil-pointer guards in metadata, retry-ID and mod-time handling

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(

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)
+		})
+	}
+}

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
 }

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)
+				}
+			}
+		})
+	}
+}

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

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)
+}

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)
+}

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
 	}