From d703a86a642d605d516ffe635cd0888c80dd1ce0 Mon Sep 17 00:00:00 2001
From: Stephen J Day <stephen.day@docker.com>
Date: Fri, 5 Dec 2014 11:46:41 -0800
Subject: [PATCH] Add checks for ReadStream offset boundary conditions

Several checks for ReadStream with offset around boundary conditions were
missing. The new checks ensure negative offsets are detected and io.EOF is
returned properly when trying to read past the end of a file. The filesystem
and inmemory driver have been updated accordingly.

An outline of missing checks for List are also part of this commit. Action will
be taken here based on discussion in issue #819.
---
 storagedriver/filesystem/driver.go     |  4 +++
 storagedriver/inmemory/driver.go       |  4 +++
 storagedriver/testsuites/testsuites.go | 43 ++++++++++++++++++++++++++
 3 files changed, 51 insertions(+)

diff --git a/storagedriver/filesystem/driver.go b/storagedriver/filesystem/driver.go
index 6fb56891b..05ec6175b 100644
--- a/storagedriver/filesystem/driver.go
+++ b/storagedriver/filesystem/driver.go
@@ -82,6 +82,10 @@ func (d *Driver) PutContent(subPath string, contents []byte) error {
 // ReadStream retrieves an io.ReadCloser for the content stored at "path" with a
 // given byte offset.
 func (d *Driver) ReadStream(path string, offset int64) (io.ReadCloser, error) {
+	if offset < 0 {
+		return nil, storagedriver.InvalidOffsetError{Path: path, Offset: offset}
+	}
+
 	file, err := os.OpenFile(d.fullPath(path), os.O_RDONLY, 0644)
 	if err != nil {
 		if os.IsNotExist(err) {
diff --git a/storagedriver/inmemory/driver.go b/storagedriver/inmemory/driver.go
index b6bdc2588..0b68e0210 100644
--- a/storagedriver/inmemory/driver.go
+++ b/storagedriver/inmemory/driver.go
@@ -83,6 +83,10 @@ func (d *Driver) ReadStream(path string, offset int64) (io.ReadCloser, error) {
 	d.mutex.RLock()
 	defer d.mutex.RUnlock()
 
+	if offset < 0 {
+		return nil, storagedriver.InvalidOffsetError{Path: path, Offset: offset}
+	}
+
 	path = d.normalize(path)
 	found := d.root.find(path)
 
diff --git a/storagedriver/testsuites/testsuites.go b/storagedriver/testsuites/testsuites.go
index 6a51cd19c..0967e2dbe 100644
--- a/storagedriver/testsuites/testsuites.go
+++ b/storagedriver/testsuites/testsuites.go
@@ -2,6 +2,7 @@ package testsuites
 
 import (
 	"bytes"
+	"io"
 	"io/ioutil"
 	"math/rand"
 	"os"
@@ -209,6 +210,43 @@ func (suite *DriverSuite) TestReadStreamWithOffset(c *check.C) {
 	readContents, err = ioutil.ReadAll(reader)
 	c.Assert(err, check.IsNil)
 	c.Assert(readContents, check.DeepEquals, contentsChunk3)
+
+	// Ensure we get invalid offest for negative offsets.
+	reader, err = suite.StorageDriver.ReadStream(filename, -1)
+	c.Assert(err, check.FitsTypeOf, storagedriver.InvalidOffsetError{})
+	c.Assert(err.(storagedriver.InvalidOffsetError).Offset, check.Equals, int64(-1))
+	c.Assert(err.(storagedriver.InvalidOffsetError).Path, check.Equals, filename)
+	c.Assert(reader, check.IsNil)
+
+	// Read past the end of the content and make sure we get a reader that
+	// returns 0 bytes and io.EOF
+	reader, err = suite.StorageDriver.ReadStream(filename, chunkSize*3)
+	c.Assert(err, check.IsNil)
+	defer reader.Close()
+
+	buf := make([]byte, chunkSize)
+	n, err := reader.Read(buf)
+	c.Assert(err, check.Equals, io.EOF)
+	c.Assert(n, check.Equals, 0)
+
+	// Check the N-1 boundary condition, ensuring we get 1 byte then io.EOF.
+	reader, err = suite.StorageDriver.ReadStream(filename, chunkSize*3-1)
+	c.Assert(err, check.IsNil)
+	defer reader.Close()
+
+	n, err = reader.Read(buf)
+	c.Assert(n, check.Equals, 1)
+
+	// We don't care whether the io.EOF comes on the this read or the first
+	// zero read, but the only error acceptable here is io.EOF.
+	if err != nil {
+		c.Assert(err, check.Equals, io.EOF)
+	}
+
+	// Any more reads should result in zero bytes and io.EOF
+	n, err = reader.Read(buf)
+	c.Assert(n, check.Equals, 0)
+	c.Assert(err, check.Equals, io.EOF)
 }
 
 // TestContinueStreamAppend tests that a stream write can be appended to without
@@ -329,6 +367,11 @@ func (suite *DriverSuite) TestList(c *check.C) {
 
 	sort.Strings(keys)
 	c.Assert(keys, check.DeepEquals, childFiles)
+
+	// A few checks to add here (check out #819 for more discussion on this):
+	// 1. Ensure that all paths are absolute.
+	// 2. Ensure that listings only include direct children.
+	// 3. Ensure that we only respond to directory listings that end with a slash (maybe?).
 }
 
 // TestMove checks that a moved object no longer exists at the source path and