diff --git a/include/storage/object_store.h b/include/storage/object_store.h index 8f556d66..147c42eb 100644 --- a/include/storage/object_store.h +++ b/include/storage/object_store.h @@ -9,6 +9,7 @@ #include #include #include +#include #include #include #include @@ -287,6 +288,15 @@ class AsyncHttpManager std::vector *infos, std::string *next_continuation_token = nullptr) const; + // Parse the ETag value from a single HTTP response header line. The header + // name is matched case-insensitively -- HTTP header names are + // case-insensitive and HTTP/2 delivers them lowercased ("etag:"), so a + // byte-exact "ETag:" match would miss it on HTTP/2 endpoints (e.g. GCS). + // Returns the unquoted value, or nullopt if the line is not an ETag header + // or carries no value. + static std::optional ParseETagHeader( + std::string_view header_line); + private: void CleanupTaskResources(ObjectStore::Task *task); bool SetupUploadRequest(ObjectStore::UploadTask *task, CURL *easy); diff --git a/src/storage/object_store.cpp b/src/storage/object_store.cpp index 4d7067a3..b786c864 100644 --- a/src/storage/object_store.cpp +++ b/src/storage/object_store.cpp @@ -2,6 +2,7 @@ #include #include +#include #include #include @@ -905,6 +906,55 @@ size_t AsyncHttpManager::WriteCallback(void *contents, return total; } +std::optional AsyncHttpManager::ParseETagHeader( + std::string_view header_line) +{ + // Match the header name case-insensitively. HTTP header names are + // case-insensitive and HTTP/2 delivers them lowercased ("etag:"); a + // byte-exact "ETag:" compare misses them on HTTP/2 endpoints (e.g. GCS), + // leaving etag_ empty and degrading the term-file CAS to an unconditional + // PUT (silent loss of split-brain fencing). + constexpr std::string_view etag_prefix = "ETag:"; + if (header_line.size() < etag_prefix.size() || + strncasecmp( + header_line.data(), etag_prefix.data(), etag_prefix.size()) != 0) + { + return std::nullopt; + } + + // Skip whitespace after the colon. + size_t value_start = etag_prefix.size(); + while ( + value_start < header_line.size() && + (header_line[value_start] == ' ' || header_line[value_start] == '\t')) + { + ++value_start; + } + + // Value runs to end of line. + size_t value_end = value_start; + while (value_end < header_line.size() && header_line[value_end] != '\r' && + header_line[value_end] != '\n') + { + ++value_end; + } + + if (value_end <= value_start) + { + return std::nullopt; + } + + std::string_view etag_value = + header_line.substr(value_start, value_end - value_start); + // Strip surrounding quotes if present. + if (etag_value.size() >= 2 && etag_value.front() == '"' && + etag_value.back() == '"') + { + etag_value = etag_value.substr(1, etag_value.size() - 2); + } + return std::string(etag_value); +} + size_t AsyncHttpManager::HeaderCallback(char *buffer, size_t size, size_t nitems, @@ -916,41 +966,10 @@ size_t AsyncHttpManager::HeaderCallback(char *buffer, return size * nitems; } - // Extract ETag header: "ETag: "value"\r\n" std::string_view header_line(buffer, size * nitems); - constexpr std::string_view etag_prefix = "ETag:"; - if (header_line.size() >= etag_prefix.size() && - header_line.substr(0, etag_prefix.size()) == etag_prefix) - { - // Find the value after "ETag: " - size_t value_start = etag_prefix.size(); - while (value_start < header_line.size() && - (header_line[value_start] == ' ' || - header_line[value_start] == '\t')) - { - ++value_start; - } - - // Extract value (may be quoted) - size_t value_end = value_start; - while (value_end < header_line.size() && - header_line[value_end] != '\r' && header_line[value_end] != '\n') - { - ++value_end; - } - - if (value_end > value_start) - { - std::string_view etag_value = - header_line.substr(value_start, value_end - value_start); - // Remove quotes if present - if (etag_value.size() >= 2 && etag_value.front() == '"' && - etag_value.back() == '"') - { - etag_value = etag_value.substr(1, etag_value.size() - 2); - } - task->etag_ = std::string(etag_value); - } + if (std::optional etag = ParseETagHeader(header_line)) + { + task->etag_ = std::move(*etag); } return size * nitems; diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 52fa8003..5e1827a9 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -47,6 +47,7 @@ set(UTEST_SOURCES iouring_tail_scratch.cpp segment_allocator.cpp data_page_cache.cpp + object_store_etag.cpp ) string( REPLACE ".cpp" "" BASENAMES_UTEST "${UTEST_SOURCES}" ) diff --git a/tests/object_store_etag.cpp b/tests/object_store_etag.cpp new file mode 100644 index 00000000..7bc61dc1 --- /dev/null +++ b/tests/object_store_etag.cpp @@ -0,0 +1,40 @@ +#include +#include +#include + +#include "storage/object_store.h" + +// The ETag response header must be matched case-insensitively: HTTP header +// names are case-insensitive and HTTP/2 (e.g. GCS) delivers them lowercased. +// A byte-exact "ETag:" match would leave etag_ empty on HTTP/2, silently +// downgrading the term-file compare-and-swap to an unconditional PUT. +TEST_CASE("ParseETagHeader matches the header name case-insensitively", + "[object_store]") +{ + using eloqstore::AsyncHttpManager; + + // HTTP/1.1 style (capitalized, quoted). + auto h1 = AsyncHttpManager::ParseETagHeader("ETag: \"abc123\"\r\n"); + REQUIRE(h1.has_value()); + REQUIRE(*h1 == "abc123"); + + // HTTP/2 style (lowercased) -- the case this fix targets. + auto h2 = AsyncHttpManager::ParseETagHeader("etag: \"abc123\"\r\n"); + REQUIRE(h2.has_value()); + REQUIRE(*h2 == "abc123"); + + // Mixed case, tab separator, unquoted weak validator. + auto h3 = AsyncHttpManager::ParseETagHeader("Etag:\tW/xyz\r\n"); + REQUIRE(h3.has_value()); + REQUIRE(*h3 == "W/xyz"); + + // Non-ETag header is ignored. + REQUIRE_FALSE( + AsyncHttpManager::ParseETagHeader("Content-Length: 5\r\n").has_value()); + + // ETag header with no value. + REQUIRE_FALSE(AsyncHttpManager::ParseETagHeader("etag:\r\n").has_value()); + + // Shorter than the prefix. + REQUIRE_FALSE(AsyncHttpManager::ParseETagHeader("eta").has_value()); +}