From b0b4d61f6e42b16e709fb2ab1c6a30309d21756f Mon Sep 17 00:00:00 2001 From: Michael Hoglan Date: Fri, 6 Mar 2015 16:35:51 -0600 Subject: [PATCH 1/4] Added guard protections for requests / responses Add guard to handle if nil path is requested Add guard to handle if content-length from response is -1 which represents that the response is chunked transfer encoding or EOF close response --- getter.go | 7 +++++++ s3gof3r.go | 4 ++++ 2 files changed, 11 insertions(+) diff --git a/getter.go b/getter.go index d0e54b0..9df1e11 100644 --- a/getter.go +++ b/getter.go @@ -77,6 +77,13 @@ func newGetter(getURL url.URL, c *Config, b *Bucket) (io.ReadCloser, http.Header if resp.StatusCode != 200 { return nil, nil, newRespError(resp) } + + // Golang changes content-length to -1 when chunked transfer encoding / EOF close response detected + if resp.ContentLength == -1 { + return nil, nil, fmt.Errorf("Retrieving objects with undefined content-length " + + " responses (chunked transfer encoding / EOF close) is not supported") + } + g.contentLen = resp.ContentLength g.chunkTotal = int((g.contentLen + g.bufsz - 1) / g.bufsz) // round up, integer division logger.debugPrintf("object size: %3.2g MB", float64(g.contentLen)/float64((1*mb))) diff --git a/s3gof3r.go b/s3gof3r.go index aee7c1d..1084e85 100644 --- a/s3gof3r.go +++ b/s3gof3r.go @@ -3,6 +3,7 @@ package s3gof3r import ( + "errors" "fmt" "io" "io/ioutil" @@ -84,6 +85,9 @@ func (s3 *S3) Bucket(name string) *Bucket { // Header data from the downloaded object is also returned, useful for reading object metadata. // DefaultConfig is used if c is nil func (b *Bucket) GetReader(path string, c *Config) (r io.ReadCloser, h http.Header, err error) { + if path == "" { + return nil, nil, errors.New("Empty path requested") + } if c == nil { c = b.conf() } From 75198037a7c414a7d110818e0b004b2df1bb4d33 Mon Sep 17 00:00:00 2001 From: Michael Hoglan Date: Fri, 6 Mar 2015 13:41:51 -0600 Subject: [PATCH 2/4] Fix OBOE when comparing cIdx and rChunk.size Remove the -1 occurring when checking if the current index (cIdx) was equal or greater than size of chunk (rChunk.size) being processed. Both values are 1-based and there should not be the need to offset the size by 1; cIdx starts at 0 initially, but is replaced by number of bytes read which is 1-based as it is a count and not an index into the range of bytes being read. --- getter.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/getter.go b/getter.go index 9df1e11..5d63a35 100644 --- a/getter.go +++ b/getter.go @@ -223,7 +223,7 @@ func (g *getter) Read(p []byte) (int, error) { nw += n g.bytesRead += int64(n) - if g.cIdx >= g.rChunk.size-1 { // chunk complete + if g.cIdx >= g.rChunk.size { // chunk complete g.sp.give <- g.rChunk.b g.chunkID++ g.rChunk = nil From 39d8e0fde8febc0cf598666de1a78388648c613b Mon Sep 17 00:00:00 2001 From: Michael Hoglan Date: Fri, 6 Mar 2015 16:26:34 -0600 Subject: [PATCH 3/4] Add guard to handle if all chunks are read Add guard to handle if bytesRead is greater than content-length. Should not occur as golang uses a LimitedReader up to the content-length, but if for some reason the bytesRead was greater than content-length, the getter will hang as it keeps trying to read the nextChunk (which does not exist) --- getter.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/getter.go b/getter.go index 5d63a35..55f4ea9 100644 --- a/getter.go +++ b/getter.go @@ -209,7 +209,19 @@ func (g *getter) Read(p []byte) (int, error) { for nw < len(p) { if g.bytesRead == g.contentLen { return nw, io.EOF + } else if g.bytesRead > g.contentLen { + // Here for robustness / completeness + // Should not occur as golang uses LimitedReader up to content-length + return nw, fmt.Errorf("Expected %d bytes, received %d (too many bytes)", + g.contentLen, g.bytesRead) } + + // If for some reason no more chunks to be read and bytes are off, error, incomplete result + if g.chunkID >= g.chunkTotal { + return nw, fmt.Errorf("Expected %d bytes, received %d and chunkID %d >= chunkTotal %d (no more chunks remaining)", + g.contentLen, g.bytesRead, g.chunkID, g.chunkTotal) + } + if g.rChunk == nil { g.rChunk, err = g.nextChunk() if err != nil { From ee13edc6af5e98efb87371b5b599fd789cb826d0 Mon Sep 17 00:00:00 2001 From: Randall McPherson Date: Mon, 9 Mar 2015 15:01:13 -0400 Subject: [PATCH 4/4] add nil path test --- s3gof3r.go | 2 +- s3gof3r_test.go | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/s3gof3r.go b/s3gof3r.go index 1084e85..72e9dc3 100644 --- a/s3gof3r.go +++ b/s3gof3r.go @@ -86,7 +86,7 @@ func (s3 *S3) Bucket(name string) *Bucket { // DefaultConfig is used if c is nil func (b *Bucket) GetReader(path string, c *Config) (r io.ReadCloser, h http.Header, err error) { if path == "" { - return nil, nil, errors.New("Empty path requested") + return nil, nil, errors.New("empty path requested") } if c == nil { c = b.conf() diff --git a/s3gof3r_test.go b/s3gof3r_test.go index 24390ed..4594df3 100644 --- a/s3gof3r_test.go +++ b/s3gof3r_test.go @@ -40,6 +40,7 @@ func TestGetReader(t *testing.T) { {"t1.test", nil, 1 * kb, nil}, {"no-md5", &Config{Scheme: "https", Client: ClientWithTimeout(clientTimeout), Md5Check: false}, 1, nil}, {"NoKey", nil, -1, &RespError{StatusCode: 404, Message: "The specified key does not exist."}}, + {"", nil, -1, fmt.Errorf("empty path requested")}, {"6_mb_test", &Config{Concurrency: 3, PartSize: 1 * mb, NTry: 2, Md5Check: true, Scheme: "https", Client: ClientWithTimeout(3 * time.Second)}, 6 * mb,