Merge pull request #274 from stevvooe/tarsum-verification-deadlock

digest: correctly close pipe after error in tarsum verification

digest/digest.go

@@ -58,7 +58,7 @@ var (
 	// ErrDigestInvalidFormat returned when digest format invalid.
 	ErrDigestInvalidFormat = fmt.Errorf("invalid checksum digest format")
 
-	// ErrDigestUnsupported returned when the digest algorithm is unsupported by registry.
+	// ErrDigestUnsupported returned when the digest algorithm is unsupported.
 	ErrDigestUnsupported = fmt.Errorf("unsupported digest algorithm")
 )
 

digest/verifiers.go

@@ -56,8 +56,11 @@ func NewDigestVerifier(d Digest) (Verifier, error) {
 		// TODO(sday): Ick! A goroutine per digest verification? We'll have to
 		// get the tarsum library to export an io.Writer variant.
 		go func() {
-			io.Copy(ioutil.Discard, ts)
-			pw.Close()
+			if _, err := io.Copy(ioutil.Discard, ts); err != nil {
+				pr.CloseWithError(err)
+			} else {
+				pr.Close()
+			}
 		}()
 
 		return &tarsumVerifier{

digest/verifiers_test.go

@@ -3,8 +3,10 @@ package digest
 import (
 	"bytes"
 	"crypto/rand"
+	"encoding/base64"
 	"io"
 	"os"
+	"strings"
 	"testing"
 
 	"github.com/docker/distribution/testutil"
@@ -67,6 +69,87 @@ func TestDigestVerifier(t *testing.T) {
 	}
 }
 
+// TestVerifierUnsupportedDigest ensures that unsupported digest validation is
+// flowing through verifier creation.
+func TestVerifierUnsupportedDigest(t *testing.T) {
+	unsupported := Digest("bean:0123456789abcdef")
+
+	_, err := NewDigestVerifier(unsupported)
+	if err == nil {
+		t.Fatalf("expected error when creating verifier")
+	}
+
+	if err != ErrDigestUnsupported {
+		t.Fatalf("incorrect error for unsupported digest: %v %p %p", err, ErrDigestUnsupported, err)
+	}
+}
+
+// TestJunkNoDeadlock ensures that junk input into a digest verifier properly
+// returns errors from the tarsum library. Specifically, we pass in a file
+// with a "bad header" and should see the error from the io.Copy to verifier.
+// This has been seen with gzipped tarfiles, mishandled by the tarsum package,
+// but also on junk input, such as html.
+func TestJunkNoDeadlock(t *testing.T) {
+	expected := Digest("tarsum.dev+sha256:62e15750aae345f6303469a94892e66365cc5e3abdf8d7cb8b329f8fb912e473")
+	junk := bytes.Repeat([]byte{'a'}, 1024)
+
+	verifier, err := NewDigestVerifier(expected)
+	if err != nil {
+		t.Fatalf("unexpected error creating verifier: %v", err)
+	}
+
+	rd := bytes.NewReader(junk)
+	if _, err := io.Copy(verifier, rd); err == nil {
+		t.Fatalf("unexpected error verifying input data: %v", err)
+	}
+}
+
+// TestBadTarNoDeadlock runs a tar with a "bad" tar header through digest
+// verifier, ensuring that the verifier returns an error properly.
+func TestBadTarNoDeadlock(t *testing.T) {
+	// TODO(stevvooe): This test is exposing a bug in tarsum where if we pass
+	// a gzipped tar file into tarsum, the library returns an error. This
+	// should actually work. When the tarsum package is fixed, this test will
+	// fail and we can remove this test or invert it.
+
+	// This tarfile was causing deadlocks in verifiers due mishandled copy error.
+	// This is a gzipped tar, which we typically don't see but should handle.
+	//
+	// From https://registry-1.docker.io/v2/library/ubuntu/blobs/tarsum.dev+sha256:62e15750aae345f6303469a94892e66365cc5e3abdf8d7cb8b329f8fb912e473
+	const badTar = `
+H4sIAAAJbogA/0otSdZnoDEwMDAxMDc1BdJggE6D2YZGJobGBmbGRsZAdYYGBkZGDAqmtHYYCJQW
+lyQWAZ1CqTnonhsiAAAAAP//AsV/YkEJTdMAGfFvZmA2Gv/0AAAAAAD//4LFf3F+aVFyarFeTmZx
+CbXtAOVnMxMTXPFvbGpmjhb/xobmwPinSyCO8PgHAAAA///EVU9v2z4MvedTEMihl9a5/26/YTkU
+yNKiTTDsKMt0rE0WDYmK628/ym7+bFmH2DksQACbIB/5+J7kObwiQsXc/LdYVGibLObRccw01Qv5
+19EZ7hbbZudVgWtiDFCSh4paYII4xOVxNgeHLXrYow+GXAAqgSuEQhzlTR5ZgtlsVmB+aKe8rswe
+zzsOjwtoPGoTEGplHHhMCJqxSNUPwesbEGbzOXxR34VCHndQmjfhUKhEq/FURI0FqJKFR5q9NE5Z
+qbaoBGoglAB+5TSK0sOh3c3UPkRKE25dEg8dDzzIWmqN2wG3BNY4qRL1VFFAoJJb5SXHU90n34nk
+SUS8S0AeGwqGyXdZel1nn7KLGhPO0kDeluvN48ty9Q2269ft8/PTy2b5GfKuh9/2LBIWo6oz+N8G
+uodmWLETg0mW4lMP4XYYCL4+rlawftpIO40SA+W6Yci9wRZE1MNOjmyGdhBQRy9OHpqOdOGh/wT7
+nZdOkHZ650uIK+WrVZdkgErJfnNEJysLnI5FSAj4xuiCQNpOIoNWmhyLByVHxEpLf3dkr+k9KMsV
+xV0FhiVB21hgD3V5XwSqRdOmsUYr7oNtZXTVzyTHc2/kqokBy2ihRMVRTN+78goP5Ur/aMhz+KOJ
+3h2UsK43kdwDo0Q9jfD7ie2RRur7MdpIrx1Z3X4j/Q1qCswN9r/EGCvXiUy0fI4xeSknnH/92T/+
+fgIAAP//GkWjYBSMXAAIAAD//2zZtzAAEgAA`
+	expected := Digest("tarsum.dev+sha256:62e15750aae345f6303469a94892e66365cc5e3abdf8d7cb8b329f8fb912e473")
+
+	verifier, err := NewDigestVerifier(expected)
+	if err != nil {
+		t.Fatalf("unexpected error creating verifier: %v", err)
+	}
+
+	rd := base64.NewDecoder(base64.StdEncoding, strings.NewReader(badTar))
+
+	if _, err := io.Copy(verifier, rd); err == nil {
+		t.Fatalf("unexpected error verifying input data: %v", err)
+	}
+
+	if verifier.Verified() {
+		// For now, we expect an error, since tarsum library cannot handle
+		// compressed tars (!!!).
+		t.Fatalf("no error received after invalid tar")
+	}
+}
+
 // TODO(stevvooe): Add benchmarks to measure bytes/second throughput for
 // DigestVerifier. We should be tarsum/gzip limited for common cases but we
 // want to verify this.