Skip to content

Commit

Permalink
Merge 'Handle S3 partial read overflows' from Pavel Emelyanov
Browse files Browse the repository at this point in the history
The test case that validates upload-sink works does this by getting several random ranges from the uploaded object and checks that the content is what it should be. The range boundaries are generated like this:

```
    uint64_t len = random(1, chunk_size);
    uint64_t offset = random(file_size) - len;
```

The 2nd line is not correct, if random number happens less than the len the offset befomes "negative", i.e. -- very large 64-bit unsigned value.

Next, this offset:len gets into s3 client's get_object_contiguous() helper which in turn converts them into http range header's bytes-specifier format which is "first_bytet-last_byte" one. The math here is

```
    first_byte = offset;
    last_byte = offset + len - 1;
```

Here the overflow of the offset thing results in underflow of the last_byte -- it becomes less than the first_byte. According to RFC this range-specifier is invalid and (!) can be ignored by the server. This is what minio does -- it ignores invalid range and returns back full object.

But that's not all. When returning object portion the http request status code is PartialContent, but when the range is ignored and full object is returned, the status is OK. This makes s3 client's request fail with unexpected_status_error in the middle of the test. Then the object is removed with deferred action and actual error is printed into logs. In the end of the day logs look as if deletion of an object failed with OK status %)

fixes: #16133

Closes #16324

* github.com:scylladb/scylladb:
  test/s3: Avoid object range overflow
  s3/client: Handle GET-with-Range overflows correctly
  • Loading branch information
denesb committed Dec 18, 2023
2 parents 081f30d + 76705b6 commit a6200e9
Show file tree
Hide file tree
Showing 2 changed files with 9 additions and 2 deletions.
2 changes: 1 addition & 1 deletion test/boost/s3_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -142,7 +142,7 @@ void do_test_client_multipart_upload(bool with_copy_upload) {
testlog.info("Checking correctness\n");
for (int samples = 0; samples < 7; samples++) {
uint64_t len = tests::random::get_int(1u, chunk_size);
uint64_t off = tests::random::get_int(object_size) - len;
uint64_t off = tests::random::get_int(object_size - len);

auto s_buf = cln->get_object_contiguous(name, s3::range{ off, len }).get0();
unsigned align = off % chunk_size;
Expand Down
9 changes: 8 additions & 1 deletion utils/s3/client.cc
Original file line number Diff line number Diff line change
Expand Up @@ -358,7 +358,14 @@ future<temporary_buffer<char>> client::get_object_contiguous(sstring object_name
auto req = http::request::make("GET", _host, object_name);
http::reply::status_type expected = http::reply::status_type::ok;
if (range) {
auto range_header = format("bytes={}-{}", range->off, range->off + range->len - 1);
if (range->len == 0) {
co_return temporary_buffer<char>();
}
auto end_bytes = range->off + range->len - 1;
if (end_bytes < range->off) {
throw std::overflow_error("End of the range exceeds 64-bits");
}
auto range_header = format("bytes={}-{}", range->off, end_bytes);
s3l.trace("GET {} contiguous range='{}'", object_name, range_header);
req._headers["Range"] = std::move(range_header);
expected = http::reply::status_type::partial_content;
Expand Down

0 comments on commit a6200e9

Please sign in to comment.