Don't silently ignore errors determining size in TryReuseBlob

When looking for inexact matches, this will cause the matches to be skipped.
When checking for an exact match, this will cause an upload failure;
we don't have any other way to handle pre-existing blobs on the destination.

Signed-off-by: Miloslav Trmač <mitr@redhat.com>

Author: mtrmac

docker/docker_client.go

@@ -998,20 +998,33 @@ func (c *dockerClient) getExternalBlob(ctx context.Context, urls []string) (io.R
 			resp.Body.Close()
 			continue
 		}
-		return resp.Body, getBlobSize(resp), nil
+
+		size, err := getBlobSize(resp)
+		if err != nil {
+			size = -1
+		}
+		return resp.Body, size, nil
 	}
 	if remoteErrors == nil {
 		return nil, 0, nil // fallback to non-external blob
 	}
 	return nil, 0, fmt.Errorf("failed fetching external blob from all urls: %w", multierr.Format("", ", ", "", remoteErrors))
 }
 
-func getBlobSize(resp *http.Response) int64 {
-	size, err := strconv.ParseInt(resp.Header.Get("Content-Length"), 10, 64)
-	if err != nil {
-		size = -1
+func getBlobSize(resp *http.Response) (int64, error) {
+	hdrs := resp.Header.Values("Content-Length")
+	if len(hdrs) == 0 {
+		return -1, errors.New(`Missing "Content-Length" header in response`)
 	}
-	return size
+	hdr := hdrs[0] // Equivalent to resp.Header.Get(…)
+	size, err := strconv.ParseInt(hdr, 10, 64)
+	if err != nil { // Go’s response reader should already reject such values.
+		return -1, err
+	}
+	if size < 0 { // '-' is not a valid character in Content-Length, so negative values are invalid. Go’s response reader should already reject such values.
+		return -1, fmt.Errorf(`Invalid negative "Content-Length" %q`, hdr)
+	}
+	return size, nil
 }
 
 // getBlob returns a stream for the specified blob in ref, and the blob’s size (or -1 if unknown).
@@ -1042,7 +1055,10 @@ func (c *dockerClient) getBlob(ctx context.Context, ref dockerReference, info ty
 		return nil, 0, fmt.Errorf("fetching blob: %w", err)
 	}
 	cache.RecordKnownLocation(ref.Transport(), bicTransportScope(ref), info.Digest, newBICLocationReference(ref))
-	blobSize := getBlobSize(res)
+	blobSize, err := getBlobSize(res)
+	if err != nil {
+		blobSize = -1
+	}
 
 	reconnectingReader, err := newBodyReader(ctx, c, path, res.Body)
 	if err != nil {

docker/docker_client_test.go

@@ -338,6 +338,41 @@ func TestParseRegistryWarningHeader(t *testing.T) {
 	}
 }
 
+func TestGetBlobSize(t *testing.T) {
+	for _, c := range []struct {
+		headers  []string
+		expected int64 // -1 if error expected
+	}{
+		{[]string{}, -1},
+		{[]string{"0"}, 0},
+		{[]string{"1"}, 1},
+		{[]string{"0777"}, 777},  // Not interpreted as octal
+		{[]string{"x"}, -1},      // Not a number: Go's response reader rejects such responses.
+		{[]string{"1", "2"}, -1}, // Ambiguous: Go's response reader rejects such responses.
+		{[]string{""}, -1},       // Empty header: Go's response reader rejects such responses.
+		{[]string{"-1"}, -1},     // Negative: Go's response reader rejects such responses.
+	} {
+		var buf bytes.Buffer
+		buf.WriteString("HTTP/1.1 200 OK\r\n")
+		for _, v := range c.headers {
+			buf.WriteString("Content-Length: " + v + "\r\n")
+		}
+		buf.WriteString("\r\n")
+		resp, err := http.ReadResponse(bufio.NewReader(&buf), nil)
+		if err != nil {
+			assert.Equal(t, int64(-1), c.expected)
+		} else {
+			res, err := getBlobSize(resp)
+			if c.expected == -1 {
+				assert.Error(t, err, c.headers)
+			} else {
+				require.NoError(t, err, c.headers)
+				assert.Equal(t, c.expected, res)
+			}
+		}
+	}
+}
+
 func TestIsManifestUnknownError(t *testing.T) {
 	// Mostly a smoke test; we can add more registries here if they need special handling.
 

docker/docker_image_dest.go

@@ -243,8 +243,12 @@ func (d *dockerImageDestination) blobExists(ctx context.Context, repo reference.
 	defer res.Body.Close()
 	switch res.StatusCode {
 	case http.StatusOK:
+		size, err := getBlobSize(res)
+		if err != nil {
+			return false, -1, fmt.Errorf("determining size of blob %s in %s: %w", digest, repo.Name(), err)
+		}
 		logrus.Debugf("... already exists")
-		return true, getBlobSize(res), nil
+		return true, size, nil
 	case http.StatusUnauthorized:
 		logrus.Debugf("... not authorized")
 		return false, -1, fmt.Errorf("checking whether a blob %s exists in %s: %w", digest, repo.Name(), registryHTTPResponseToError(res))