cache/s3: fix io.ReaderAt contract violations in the import read path

[ernetas]

The s3 cache backend reads blobs back through a ReaderAt built on ranged GetObject calls. There are three bugs in it.

A short read comes back with no error

This is the loop that fills the buffer:

nn, err = hrs.rc.Read(p)
n += nn
p = p[nn:]
if nn == len(p) || err != nil {
	break
}

nn is what was just read. len(p) after the reslice is what is still wanted. Those are different things, and when they happen to be equal the loop stops early and returns a short count with a nil error.

Ask for 100 bytes from a stream that gives back 50 at a time and you get 50 bytes and no error. One byte at a time gives you 99. io.ReaderAt says a read shorter than the buffer must report an error, and the content package relies on that, so it does not read again. The layer is quietly truncated.

The body of an S3 response is an HTTP stream, so short reads are normal, not an edge case.

The offset is tracked from the wrong place

hrs.offset += int64(n) adds to wherever the reader thought it was, not to the offset it was actually asked for.

After a read at a new offset the tracked position no longer matches the stream. The next read that continues from there looks like a seek, so the object is reopened for nothing. Worse, a later read whose offset happens to equal the stale value looks like a continuation, and gets served from the wrong part of the object.

The io.ReaderAt fast path could not have worked

The stream is opened with a bytes=off- range, so byte 0 of the stream is byte off of the object. The cached reader would then apply the same offset again and read from twice as far in.

It never ran, because an HTTP response body is not an io.ReaderAt. But it was cached in a field and reused for every later call, so if it ever became one the reads would be wrong from then on.

The fix

Use io.ReadFull, which returns io.ErrUnexpectedEOF when it cannot fill the buffer. Track the offset from the one that was requested. Drop the fast path. A zero length read now returns without opening the object at all.

Tests

There were no tests for this file. This adds seven, against a fake stream that hands back a fixed number of bytes per read and records the offsets it was opened at.

Five of them fail on the current code:

• TestReaderAtShortReads at chunk sizes 1 and 50
• TestReaderAtSequentialReadsReuseStream
• TestReaderAtOffsetAccounting
• TestReaderAtTruncatedObject
• TestReaderAtEmptyRead

The other two cover Close and concurrent reads and pass either way. They are there to keep the mutex added in b0e7d80 honest.

Notes

This is a standalone fix. It does not depend on anything else and nothing else in the s3 backend changes.