cache/s3: fix a panic, an off-by-one and a discarded abort in the touch path - #7124
Closed
ernetas wants to merge 1 commit into
Closed
cache/s3: fix a panic, an off-by-one and a discarded abort in the touch path#7124ernetas wants to merge 1 commit into
ernetas wants to merge 1 commit into
Conversation
…ch path When a blob is already in the bucket the exporter refreshes its timestamp instead of uploading it again. Three things are wrong on that path. HeadObject's ContentLength was dereferenced without a check. It is a *int64 and the SDK leaves it nil when the response carries no Content-Length header, which several S3-compatible endpoints do. That panicked the daemon rather than failing the export. Check it at the call site and pass the size by value, so touch cannot be handed a nil. buildCopySourceRange clamped with end > objectSize. A copy source range is inclusive, so the last byte it may name is objectSize-1, and an end equal to objectSize addresses one byte past the object. It is reachable when the remaining bytes are exactly one part short of the part size. The abort of a failed multipart upload ran on the context that had just failed, which is usually already cancelled, so the abort failed too. Its error was discarded, so nothing said so. An upload left incomplete keeps its parts, and the bucket keeps charging for them until a lifecycle rule removes them. Abort on a detached context with its own timeout and log a failure. Add a test for the copy source range. The one byte short case fails on the old bound. Signed-off-by: Ernestas Lukoševičius <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When a blob is already in the bucket the exporter refreshes its timestamp instead of uploading it again. There are three separate problems on that path.
A nil dereference takes the daemon down
existsreturnshead.ContentLength, which is a*int64. The SDK leaves it nil when the response has noContent-Lengthheader.touchthen does:The SDK models the field as optional, so nothing guarantees it is set. When it is not, this is a nil dereference, and an export that should have failed with an error panics the daemon instead.
I have not reproduced a nil from a specific endpoint. The argument here is only that the code dereferences a pointer the SDK says may be absent, and that the cost of being wrong is the whole daemon.
Check it at the call site and pass the size by value, so
touchcannot be handed a nil in the first place.The copy range can name a byte past the end of the object
buildCopySourceRangeclamps like this:A copy source range is inclusive. For an object of
objectSizebytes the last byte you may name isobjectSize-1. So anendequal toobjectSizeis already one too far, and>lets it through.You need the remaining bytes to be exactly one part short of the part size to hit it, which is why nobody has. The bound should be
>=.A failed multipart upload is never actually aborted
The cleanup runs on the context that just failed:
If the export was cancelled, or the context timed out, that context is already dead and the abort fails immediately. The result is discarded, so there is no log line and no error either.
An incomplete multipart upload keeps the parts that were uploaded. They do not show up in a bucket listing, but they are billed until something removes them. If you do not have a lifecycle rule for
AbortIncompleteMultipartUpload, they stay forever.Abort on a context detached from the failed one, with its own 30 second timeout, and log it if that fails too.
Tests
Adds a table test for
buildCopySourceRange. Every case also asserts that the range never names a byte at or pastobjectSize, which is the property that was broken.The "remainder one byte short of the part size" case fails on the current bound.
The other two fixes are on paths that need a real bucket, so they are not covered here.
Notes
This is independent of #7123 and #7121. It only touches the touch path.