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!