cache/s3: fix a panic, an off-by-one and a discarded abort in the touch path

cache/remotecache/s3/s3.go

@@ -25,6 +25,7 @@ import (
 	cacheimporttypes "github.com/moby/buildkit/cache/remotecache/v1/types"
 	"github.com/moby/buildkit/session"
 	"github.com/moby/buildkit/solver"
+	"github.com/moby/buildkit/util/bklog"
 	"github.com/moby/buildkit/util/compression"
 	"github.com/moby/buildkit/util/progress"
 	"github.com/moby/buildkit/worker"
@@ -52,6 +53,10 @@ const (
 	attrRetryMode             = "retry_mode"
 	attrRetryMaxAttempts      = "retry_max_attempts"
 	maxCopyObjectSize         = 5 * 1024 * 1024 * 1024
+	// abortMultipartTimeout bounds the best-effort cleanup of a failed
+	// multipart upload, which runs on a context detached from the failed
+	// operation.
+	abortMultipartTimeout = 30 * time.Second
 )
 
 type Config struct {
@@ -283,7 +288,13 @@ func (e *exporter) Finalize(ctx context.Context) (map[string]string, error) {
 				}
 				if exists != nil {
 					if time.Since(*exists) > e.config.TouchRefresh {
-						err = e.s3Client.touch(groupCtx, key, size)
+						// Some S3-compatible endpoints omit Content-Length from
+						// HeadObject responses, and dereferencing it
+						// unconditionally took the daemon down.
+						if size == nil {
+							return errors.Errorf("failed to touch %s: object has no content length", key)
+						}
+						err = e.s3Client.touch(groupCtx, key, *size)
 						if err != nil {
 							return errors.Wrapf(err, "failed to touch file")
 						}
@@ -562,19 +573,20 @@ func (s3Client *s3Client) exists(ctx context.Context, key string) (*time.Time, *
 
 func buildCopySourceRange(start int64, objectSize int64) string {
 	end := start + maxCopyObjectSize - 1
-	if end > objectSize {
+	// The range is inclusive, so the last addressable byte is objectSize-1.
+	if end >= objectSize {
 		end = objectSize - 1
 	}
 	startRange := strconv.FormatInt(start, 10)
 	stopRange := strconv.FormatInt(end, 10)
 	return "bytes=" + startRange + "-" + stopRange
 }
 
-func (s3Client *s3Client) touch(ctx context.Context, key string, size *int64) (err error) {
+func (s3Client *s3Client) touch(ctx context.Context, key string, size int64) (err error) {
 	copySource := fmt.Sprintf("%s/%s", s3Client.bucket, key)
 
 	// CopyObject does not support files > 5GB
-	if *size < maxCopyObjectSize {
+	if size < maxCopyObjectSize {
 		cp := &s3.CopyObjectInput{
 			Bucket:            &s3Client.bucket,
 			CopySource:        &copySource,
@@ -598,22 +610,31 @@ func (s3Client *s3Client) touch(ctx context.Context, key string, size *int64) (e
 	}
 
 	defer func() {
+		if err == nil {
+			return
+		}
 		abortIn := s3.AbortMultipartUploadInput{
 			Bucket:   &s3Client.bucket,
 			Key:      &key,
 			UploadId: output.UploadId,
 		}
-		if err != nil {
-			s3Client.AbortMultipartUpload(ctx, &abortIn)
+		// By the time we get here the operation context is usually already
+		// cancelled, which would fail the abort and leave the upload to accrue
+		// storage charges. Abort on a detached context, and report a failure
+		// rather than discarding it.
+		abortCtx, cancel := context.WithTimeoutCause(context.WithoutCancel(ctx), abortMultipartTimeout, errors.WithStack(context.DeadlineExceeded))
+		defer cancel()
+		if _, abortErr := s3Client.AbortMultipartUpload(abortCtx, &abortIn); abortErr != nil {
+			bklog.G(ctx).Errorf("failed to abort multipart upload for %s, it may keep incurring storage costs until a bucket lifecycle rule removes it: %v", key, abortErr)
 		}
 	}()
 
 	var currentPartNumber int32 = 1
 	var currentPosition int64
 	var completedParts []s3types.CompletedPart
 
-	for currentPosition < *size {
-		copyRange := buildCopySourceRange(currentPosition, *size)
+	for currentPosition < size {
+		copyRange := buildCopySourceRange(currentPosition, size)
 		partInput := s3.UploadPartCopyInput{
 			Bucket:          &s3Client.bucket,
 			CopySource:      &copySource,

cache/remotecache/s3/touch_test.go

@@ -0,0 +1,59 @@
+package s3
+
+import (
+	"fmt"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+)
+
+// TestBuildCopySourceRange checks the ranges used to refresh an object larger
+// than the CopyObject limit. A copy source range is inclusive, so the last
+// byte it may name is objectSize-1.
+func TestBuildCopySourceRange(t *testing.T) {
+	tests := []struct {
+		name       string
+		start      int64
+		objectSize int64
+		want       string
+	}{
+		{
+			name:       "first part of a larger object",
+			start:      0,
+			objectSize: 3 * maxCopyObjectSize,
+			want:       "bytes=0-5368709119",
+		},
+		{
+			name:       "final short part",
+			start:      maxCopyObjectSize,
+			objectSize: maxCopyObjectSize + 100,
+			want:       "bytes=5368709120-5368709219",
+		},
+		{
+			name:       "remainder ends exactly on the part size",
+			start:      0,
+			objectSize: maxCopyObjectSize,
+			want:       "bytes=0-5368709119",
+		},
+		{
+			name:       "remainder one byte short of the part size",
+			start:      0,
+			objectSize: maxCopyObjectSize - 1,
+			want:       "bytes=0-5368709118",
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := buildCopySourceRange(tt.start, tt.objectSize)
+			require.Equal(t, tt.want, got)
+
+			// The range must never address a byte past the end of the object.
+			var start, end int64
+			_, err := fmt.Sscanf(got, "bytes=%d-%d", &start, &end)
+			require.NoError(t, err)
+			require.Less(t, end, tt.objectSize)
+			require.LessOrEqual(t, start, end)
+		})
+	}
+}