1. docker
  2. Commit 0306a41909175b467d08ebc97d4c5136ca9e7ebd
Browse code

Correctly close pipe after error in tarsum verification

This addresses a subtle deadlock where an error during a copy prevented pipe
closure to propagate correctly. By closing down the read end of the pipe rather
than the write end, the waiting writer is properly signaled. A nice side-effect
of this change is that errors encountered by io.Copy are now propagated to the
verifier's Write method.

A test to ensure validation errors for unsupported digest types has been added,
as well.

Signed-off-by: Stephen J Day <stephen.day@docker.com>

Stephen J Day authored on 2015/03/19 10:44:15
Showing 5 changed files
Dockerfile
History View file @ 0306a41
... ... 
@@ -108,7 +108,7 @@ RUN go get golang.org/x/tools/cmd/cover
108 108 
 RUN gem install --no-rdoc --no-ri fpm --version 1.3.2
109 109
110 110 
 # Install registry
111 
-ENV REGISTRY_COMMIT 0c130dff5baf3168f2c85630c6d2344b81261269
111 
+ENV REGISTRY_COMMIT d957768537c5af40e4f4cd96871f7b2bde9e2923
112 112 
 RUN set -x \
113 113 
 	&& git clone https://github.com/docker/distribution.git /go/src/github.com/docker/distribution \
114 114 
 	&& (cd /go/src/github.com/docker/distribution && git checkout -q $REGISTRY_COMMIT) \
hack/vendor.sh
History View file @ 0306a41
... ... 
@@ -69,7 +69,7 @@ if [ "$1" = '--go' ]; then
69 69 
 fi
70 70
71 71 
 # get digest package from distribution
72 
-clone git github.com/docker/distribution 0c130dff5baf3168f2c85630c6d2344b81261269
72 
+clone git github.com/docker/distribution d957768537c5af40e4f4cd96871f7b2bde9e2923
73 73 
 mv src/github.com/docker/distribution/digest tmp-digest
74 74 
 rm -rf src/github.com/docker/distribution
75 75 
 mkdir -p src/github.com/docker/distribution
vendor/src/github.com/docker/distribution/digest/digest.go
History View file @ 0306a41
... ... 
@@ -58,7 +58,7 @@ var (
58 58 
 	// ErrDigestInvalidFormat returned when digest format invalid.
59 59 
 	ErrDigestInvalidFormat = fmt.Errorf("invalid checksum digest format")
60 60
61 
-	// ErrDigestUnsupported returned when the digest algorithm is unsupported by registry.
61 
+	// ErrDigestUnsupported returned when the digest algorithm is unsupported.
62 62 
 	ErrDigestUnsupported = fmt.Errorf("unsupported digest algorithm")
63 63 
 )
64 64
vendor/src/github.com/docker/distribution/digest/verifiers.go
History View file @ 0306a41
... ... 
@@ -56,8 +56,11 @@ func NewDigestVerifier(d Digest) (Verifier, error) {
56 56 
 		// TODO(sday): Ick! A goroutine per digest verification? We'll have to
57 57 
 		// get the tarsum library to export an io.Writer variant.
58 58 
 		go func() {
59 
-			io.Copy(ioutil.Discard, ts)
60 
-			pw.Close()
59 
+			if _, err := io.Copy(ioutil.Discard, ts); err != nil {
60 
+				pr.CloseWithError(err)
61 
+			} else {
62 
+				pr.Close()
63 
+			}
61 64 
 		}()
62 65
63 66 
 		return &tarsumVerifier{
vendor/src/github.com/docker/distribution/digest/verifiers_test.go
History View file @ 0306a41
... ... 
@@ -3,8 +3,10 @@ package digest
3 3 
 import (
4 4 
 	"bytes"
5 5 
 	"crypto/rand"
6 
+	"encoding/base64"
6 7 
 	"io"
7 8 
 	"os"
9 
+	"strings"
8 10 
 	"testing"
9 11
10 12 
 	"github.com/docker/distribution/testutil"
... ... 
@@ -67,6 +69,87 @@ func TestDigestVerifier(t *testing.T) {
67 67 
 	}
68 68 
 }
69 69
70 
+// TestVerifierUnsupportedDigest ensures that unsupported digest validation is
71 
+// flowing through verifier creation.
72 
+func TestVerifierUnsupportedDigest(t *testing.T) {
73 
+	unsupported := Digest("bean:0123456789abcdef")
74 
+
75 
+	_, err := NewDigestVerifier(unsupported)
76 
+	if err == nil {
77 
+		t.Fatalf("expected error when creating verifier")
78 
+	}
79 
+
80 
+	if err != ErrDigestUnsupported {
81 
+		t.Fatalf("incorrect error for unsupported digest: %v %p %p", err, ErrDigestUnsupported, err)
82 
+	}
83 
+}
84 
+
85 
+// TestJunkNoDeadlock ensures that junk input into a digest verifier properly
86 
+// returns errors from the tarsum library. Specifically, we pass in a file
87 
+// with a "bad header" and should see the error from the io.Copy to verifier.
88 
+// This has been seen with gzipped tarfiles, mishandled by the tarsum package,
89 
+// but also on junk input, such as html.
90 
+func TestJunkNoDeadlock(t *testing.T) {
91 
+	expected := Digest("tarsum.dev+sha256:62e15750aae345f6303469a94892e66365cc5e3abdf8d7cb8b329f8fb912e473")
92 
+	junk := bytes.Repeat([]byte{'a'}, 1024)
93 
+
94 
+	verifier, err := NewDigestVerifier(expected)
95 
+	if err != nil {
96 
+		t.Fatalf("unexpected error creating verifier: %v", err)
97 
+	}
98 
+
99 
+	rd := bytes.NewReader(junk)
100 
+	if _, err := io.Copy(verifier, rd); err == nil {
101 
+		t.Fatalf("unexpected error verifying input data: %v", err)
102 
+	}
103 
+}
104 
+
105 
+// TestBadTarNoDeadlock runs a tar with a "bad" tar header through digest
106 
+// verifier, ensuring that the verifier returns an error properly.
107 
+func TestBadTarNoDeadlock(t *testing.T) {
108 
+	// TODO(stevvooe): This test is exposing a bug in tarsum where if we pass
109 
+	// a gzipped tar file into tarsum, the library returns an error. This
110 
+	// should actually work. When the tarsum package is fixed, this test will
111 
+	// fail and we can remove this test or invert it.
112 
+
113 
+	// This tarfile was causing deadlocks in verifiers due mishandled copy error.
114 
+	// This is a gzipped tar, which we typically don't see but should handle.
115 
+	//
116 
+	// From https://registry-1.docker.io/v2/library/ubuntu/blobs/tarsum.dev+sha256:62e15750aae345f6303469a94892e66365cc5e3abdf8d7cb8b329f8fb912e473
117 
+	const badTar = `
118 
+H4sIAAAJbogA/0otSdZnoDEwMDAxMDc1BdJggE6D2YZGJobGBmbGRsZAdYYGBkZGDAqmtHYYCJQW
119 
+lyQWAZ1CqTnonhsiAAAAAP//AsV/YkEJTdMAGfFvZmA2Gv/0AAAAAAD//4LFf3F+aVFyarFeTmZx
120 
+CbXtAOVnMxMTXPFvbGpmjhb/xobmwPinSyCO8PgHAAAA///EVU9v2z4MvedTEMihl9a5/26/YTkU
121 
+yNKiTTDsKMt0rE0WDYmK628/ym7+bFmH2DksQACbIB/5+J7kObwiQsXc/LdYVGibLObRccw01Qv5
122 
+19EZ7hbbZudVgWtiDFCSh4paYII4xOVxNgeHLXrYow+GXAAqgSuEQhzlTR5ZgtlsVmB+aKe8rswe
123 
+zzsOjwtoPGoTEGplHHhMCJqxSNUPwesbEGbzOXxR34VCHndQmjfhUKhEq/FURI0FqJKFR5q9NE5Z
124 
+qbaoBGoglAB+5TSK0sOh3c3UPkRKE25dEg8dDzzIWmqN2wG3BNY4qRL1VFFAoJJb5SXHU90n34nk
125 
+SUS8S0AeGwqGyXdZel1nn7KLGhPO0kDeluvN48ty9Q2269ft8/PTy2b5GfKuh9/2LBIWo6oz+N8G
126 
+uodmWLETg0mW4lMP4XYYCL4+rlawftpIO40SA+W6Yci9wRZE1MNOjmyGdhBQRy9OHpqOdOGh/wT7
127 
+nZdOkHZ650uIK+WrVZdkgErJfnNEJysLnI5FSAj4xuiCQNpOIoNWmhyLByVHxEpLf3dkr+k9KMsV
128 
+xV0FhiVB21hgD3V5XwSqRdOmsUYr7oNtZXTVzyTHc2/kqokBy2ihRMVRTN+78goP5Ur/aMhz+KOJ
129 
+3h2UsK43kdwDo0Q9jfD7ie2RRur7MdpIrx1Z3X4j/Q1qCswN9r/EGCvXiUy0fI4xeSknnH/92T/+
130 
+fgIAAP//GkWjYBSMXAAIAAD//2zZtzAAEgAA`
131 
+	expected := Digest("tarsum.dev+sha256:62e15750aae345f6303469a94892e66365cc5e3abdf8d7cb8b329f8fb912e473")
132 
+
133 
+	verifier, err := NewDigestVerifier(expected)
134 
+	if err != nil {
135 
+		t.Fatalf("unexpected error creating verifier: %v", err)
136 
+	}
137 
+
138 
+	rd := base64.NewDecoder(base64.StdEncoding, strings.NewReader(badTar))
139 
+
140 
+	if _, err := io.Copy(verifier, rd); err == nil {
141 
+		t.Fatalf("unexpected error verifying input data: %v", err)
142 
+	}
143 
+
144 
+	if verifier.Verified() {
145 
+		// For now, we expect an error, since tarsum library cannot handle
146 
+		// compressed tars (!!!).
147 
+		t.Fatalf("no error received after invalid tar")
148 
+	}
149 
+}
150 
+
70 151 
 // TODO(stevvooe): Add benchmarks to measure bytes/second throughput for
71 152 
 // DigestVerifier. We should be tarsum/gzip limited for common cases but we
72 153 
 // want to verify this.