johannesboyne/gofakes3

Incorrect handling of ranges

jwoffindin opened this issue · 3 comments

First off, thanks for such an awesome project, has been super useful.

I been getting weird failures with s3manager.Downloader. Attempting to download a small file (700 odd-bytes) always returns 0 length content and no error.

This occurs with both directfs and memory backends (only two that I tested).

It turns out func (g *GoFakeS3) getObject() is always returns an ErrInvalidRange. Unfortunately this error appears to silently ignored by s3manager so it returns 0-length files.

Tracing through the code, it appears the problem is triggered by this logic because s3manager.Downloader defaults to using PartSize of 5M.

Ignoring this unexpected behaviour of s3manager, I think the logic should, rather than being:

	if start < 0 || length < 0 || start > size || start+length > size {
		return nil, ErrInvalidRange
	}

	return &ObjectRange{Start: start, Length: length}, nil

probably be something along the lines of:

	if start < 0 || length < 0 || start > size {
		return nil, ErrInvalidRange
	}

	if start+length > size {
		return &ObjectRange{Start: start, Length: size - start}, nil
	}

	return &ObjectRange{Start: start, Length: length}, nil

If you're okay with this I can raise a PR.

Thanks!

Great find @jwoffindin! Thank you! It looks like I didn't read the spec properly. From https://tools.ietf.org/html/rfc7233#section-2.1:

   A client can limit the number of bytes requested without knowing the
   size of the selected representation.  If the last-byte-pos value is
   absent, or if the value is greater than or equal to the current
   length of the representation data, the byte range is interpreted as
   the remainder of the representation (i.e., the server replaces the
   value of last-byte-pos with a value that is one less than the current
   length of the selected representation).

If you'd like to submit a PR, that'd be great! There's a half-decent test case in range_test.go for this that'll also need an update.

A PR would be awesome! And as @shabbyrobe wrote, great finding.

Pretty sure #37 got this all sewn up, so I'll close. Thanks @jwoffindin!