From 7b323b1402f60088ec2f366112b83a8df7baf1e7 Mon Sep 17 00:00:00 2001 From: Akihiro Suda Date: Thu, 22 Mar 2018 15:57:39 +0900 Subject: [PATCH 1/2] services/content: fix reading a blob which is smaller than the read buffer. The newly added test fails without this fix in services/content/service.go: $ go test -c . && sudo ./containerd.test -test.v -test.root -test.run TestContentClient ... --- FAIL: TestContentClient/SmallBlob (0.02s) provideringester.go:62: rpc error: code = OutOfRange desc = read past object length 6 bytes helpers.go:67: drwx------ 4096 /tmp/content-suite-ContentClient-286788688 FAIL Signed-off-by: Akihiro Suda --- content/testsuite/provideringester.go | 76 +++++++++++++++++++++++++++ content/testsuite/testsuite.go | 4 ++ services/content/service.go | 12 +++-- 3 files changed, 87 insertions(+), 5 deletions(-) create mode 100644 content/testsuite/provideringester.go diff --git a/content/testsuite/provideringester.go b/content/testsuite/provideringester.go new file mode 100644 index 000000000..713cb066d --- /dev/null +++ b/content/testsuite/provideringester.go @@ -0,0 +1,76 @@ +/* + Copyright The containerd Authors. + + Licensed under the Apache License, Version 2.0 (the "License"); + you may not use this file except in compliance with the License. + You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + + Unless required by applicable law or agreed to in writing, software + distributed under the License is distributed on an "AS IS" BASIS, + WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + See the License for the specific language governing permissions and + limitations under the License. +*/ + +package testsuite + +import ( + "context" + "io" + "io/ioutil" + "testing" + + "github.com/containerd/containerd/content" + digest "github.com/opencontainers/go-digest" +) + +// ProviderIngester consists of provider and ingester. +// This interface is used for test cases that does not require +// complete content.Store implementation. +type ProviderIngester interface { + content.Provider + content.Ingester +} + +// TestSmallBlob tests reading a blob which is smaller than the read size. +// This test is exposed so that it can be used for incomplete content.Store implementation. +func TestSmallBlob(ctx context.Context, t *testing.T, store ProviderIngester) { + blob := []byte(`foobar`) + blobSize := int64(len(blob)) + blobDigest := digest.FromBytes(blob) + // test write + w, err := store.Writer(ctx, t.Name(), blobSize, blobDigest) + if err != nil { + t.Fatal(err) + } + if _, err := w.Write(blob); err != nil { + t.Fatal(err) + } + if err := w.Commit(ctx, blobSize, blobDigest); err != nil { + t.Fatal(err) + } + if err := w.Close(); err != nil { + t.Fatal(err) + } + // test read. + readSize := blobSize + 1 + ra, err := store.ReaderAt(ctx, blobDigest) + if err != nil { + t.Fatal(err) + } + r := io.NewSectionReader(ra, 0, readSize) + b, err := ioutil.ReadAll(r) + if err != nil { + t.Fatal(err) + } + if err := ra.Close(); err != nil { + t.Fatal(err) + } + d := digest.FromBytes(b) + if blobDigest != d { + t.Fatalf("expected %s (%q), got %s (%q)", blobDigest, string(blob), + d, string(b)) + } +} diff --git a/content/testsuite/testsuite.go b/content/testsuite/testsuite.go index 0f77f59b2..21247fa37 100644 --- a/content/testsuite/testsuite.go +++ b/content/testsuite/testsuite.go @@ -50,6 +50,10 @@ func ContentSuite(t *testing.T, name string, storeFn func(ctx context.Context, r t.Run("CrossNamespaceAppend", makeTest(t, name, storeFn, checkCrossNSAppend)) t.Run("CrossNamespaceShare", makeTest(t, name, storeFn, checkCrossNSShare)) + + t.Run("SmallBlob", makeTest(t, name, storeFn, func(ctx context.Context, t *testing.T, cs content.Store) { + TestSmallBlob(ctx, t, cs) + })) } // ContextWrapper is used to decorate new context used inside the test diff --git a/services/content/service.go b/services/content/service.go index d809dab1e..9038e168a 100644 --- a/services/content/service.go +++ b/services/content/service.go @@ -187,7 +187,9 @@ func (s *service) Read(req *api.ReadContentRequest, session api.Content_ReadServ var ( offset = req.Offset - size = req.Size_ + // size is read size, not the expected size of the blob (oi.Size), which the caller might not be aware of. + // offset+size can be larger than oi.Size. + size = req.Size_ // TODO(stevvooe): Using the global buffer pool. At 32KB, it is probably // little inefficient for work over a fast network. We can tune this later. @@ -199,12 +201,12 @@ func (s *service) Read(req *api.ReadContentRequest, session api.Content_ReadServ offset = 0 } - if size <= 0 { - size = oi.Size - offset + if offset > oi.Size { + return status.Errorf(codes.OutOfRange, "read past object length %v bytes", oi.Size) } - if offset+size > oi.Size { - return status.Errorf(codes.OutOfRange, "read past object length %v bytes", oi.Size) + if size <= 0 || offset+size > oi.Size { + size = oi.Size - offset } _, err = io.CopyBuffer( From a76f230984b7d6d351325291042402da6c793f67 Mon Sep 17 00:00:00 2001 From: Stephen J Day Date: Fri, 23 Mar 2018 13:56:45 -0700 Subject: [PATCH 2/2] content/testsuite: include small blob test in standard suite Signed-off-by: Stephen J Day --- content/testsuite/provideringester.go | 76 --------------------------- content/testsuite/testsuite.go | 45 ++++++++++++++-- 2 files changed, 41 insertions(+), 80 deletions(-) delete mode 100644 content/testsuite/provideringester.go diff --git a/content/testsuite/provideringester.go b/content/testsuite/provideringester.go deleted file mode 100644 index 713cb066d..000000000 --- a/content/testsuite/provideringester.go +++ /dev/null @@ -1,76 +0,0 @@ -/* - Copyright The containerd Authors. - - Licensed under the Apache License, Version 2.0 (the "License"); - you may not use this file except in compliance with the License. - You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - - Unless required by applicable law or agreed to in writing, software - distributed under the License is distributed on an "AS IS" BASIS, - WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. - See the License for the specific language governing permissions and - limitations under the License. -*/ - -package testsuite - -import ( - "context" - "io" - "io/ioutil" - "testing" - - "github.com/containerd/containerd/content" - digest "github.com/opencontainers/go-digest" -) - -// ProviderIngester consists of provider and ingester. -// This interface is used for test cases that does not require -// complete content.Store implementation. -type ProviderIngester interface { - content.Provider - content.Ingester -} - -// TestSmallBlob tests reading a blob which is smaller than the read size. -// This test is exposed so that it can be used for incomplete content.Store implementation. -func TestSmallBlob(ctx context.Context, t *testing.T, store ProviderIngester) { - blob := []byte(`foobar`) - blobSize := int64(len(blob)) - blobDigest := digest.FromBytes(blob) - // test write - w, err := store.Writer(ctx, t.Name(), blobSize, blobDigest) - if err != nil { - t.Fatal(err) - } - if _, err := w.Write(blob); err != nil { - t.Fatal(err) - } - if err := w.Commit(ctx, blobSize, blobDigest); err != nil { - t.Fatal(err) - } - if err := w.Close(); err != nil { - t.Fatal(err) - } - // test read. - readSize := blobSize + 1 - ra, err := store.ReaderAt(ctx, blobDigest) - if err != nil { - t.Fatal(err) - } - r := io.NewSectionReader(ra, 0, readSize) - b, err := ioutil.ReadAll(r) - if err != nil { - t.Fatal(err) - } - if err := ra.Close(); err != nil { - t.Fatal(err) - } - d := digest.FromBytes(b) - if blobDigest != d { - t.Fatalf("expected %s (%q), got %s (%q)", blobDigest, string(blob), - d, string(b)) - } -} diff --git a/content/testsuite/testsuite.go b/content/testsuite/testsuite.go index 21247fa37..2cba07f52 100644 --- a/content/testsuite/testsuite.go +++ b/content/testsuite/testsuite.go @@ -46,14 +46,11 @@ func ContentSuite(t *testing.T, name string, storeFn func(ctx context.Context, r t.Run("ResumeCopy", makeTest(t, name, storeFn, checkResume(resumeCopy))) t.Run("ResumeCopySeeker", makeTest(t, name, storeFn, checkResume(resumeCopySeeker))) t.Run("ResumeCopyReaderAt", makeTest(t, name, storeFn, checkResume(resumeCopyReaderAt))) + t.Run("SmallBlob", makeTest(t, name, storeFn, checkSmallBlob)) t.Run("Labels", makeTest(t, name, storeFn, checkLabels)) t.Run("CrossNamespaceAppend", makeTest(t, name, storeFn, checkCrossNSAppend)) t.Run("CrossNamespaceShare", makeTest(t, name, storeFn, checkCrossNSShare)) - - t.Run("SmallBlob", makeTest(t, name, storeFn, func(ctx context.Context, t *testing.T, cs content.Store) { - TestSmallBlob(ctx, t, cs) - })) } // ContextWrapper is used to decorate new context used inside the test @@ -527,6 +524,46 @@ func resumeCopyReaderAt(ctx context.Context, w content.Writer, b []byte, _, size return errors.Wrap(content.Copy(ctx, w, r, size, dgst), "copy failed") } +// checkSmallBlob tests reading a blob which is smaller than the read size. +func checkSmallBlob(ctx context.Context, t *testing.T, store content.Store) { + blob := []byte(`foobar`) + blobSize := int64(len(blob)) + blobDigest := digest.FromBytes(blob) + // test write + w, err := store.Writer(ctx, t.Name(), blobSize, blobDigest) + if err != nil { + t.Fatal(err) + } + if _, err := w.Write(blob); err != nil { + t.Fatal(err) + } + if err := w.Commit(ctx, blobSize, blobDigest); err != nil { + t.Fatal(err) + } + if err := w.Close(); err != nil { + t.Fatal(err) + } + // test read. + readSize := blobSize + 1 + ra, err := store.ReaderAt(ctx, blobDigest) + if err != nil { + t.Fatal(err) + } + r := io.NewSectionReader(ra, 0, readSize) + b, err := ioutil.ReadAll(r) + if err != nil { + t.Fatal(err) + } + if err := ra.Close(); err != nil { + t.Fatal(err) + } + d := digest.FromBytes(b) + if blobDigest != d { + t.Fatalf("expected %s (%q), got %s (%q)", blobDigest, string(blob), + d, string(b)) + } +} + func checkCrossNSShare(ctx context.Context, t *testing.T, cs content.Store) { wrap, ok := ctx.Value(wrapperKey{}).(ContextWrapper) if !ok {