From df93c5e86307c1656babc3f157fa9947e4ad652c Mon Sep 17 00:00:00 2001 From: Brian Bockelman Date: Sat, 14 Dec 2024 14:54:36 -0600 Subject: [PATCH 1/2] Use case-insensitive search for ETag header Refactors the ETag parsing to be done within the request command itself instead of in the filesystem layer. The code still needs to be replaced with proper header parsing. However, for now, we settle for switching to case-insensitive parsing and adding checks for return codes. --- src/S3Commands.cc | 22 ++++++++++++++++++++++ src/S3Commands.hh | 6 +++++- src/S3File.cc | 33 ++++++++++++--------------------- 3 files changed, 39 insertions(+), 22 deletions(-) diff --git a/src/S3Commands.cc b/src/S3Commands.cc index 4a6ab9a..bcf2a9e 100644 --- a/src/S3Commands.cc +++ b/src/S3Commands.cc @@ -535,6 +535,28 @@ bool AmazonS3SendMultipartPart::SendRequest(const std::string_view payload, return SendS3Request(payload, payloadSize, final, true); } +bool AmazonS3SendMultipartPart::GetEtag(std::string &result) { + if (!m_etag.empty()) { + result = m_etag; + return true; + } + auto resultString = getResultString(); + static const std::string etag = "etag: \""; + auto iter = std::search( + resultString.begin(), resultString.end(), etag.begin(), etag.end(), + [](char a, char b) { return std::tolower(a) == std::tolower(b); }); + if (iter == resultString.end()) { + return false; + } + std::size_t startPos = std::distance(resultString.begin(), iter); + std::size_t endPos = resultString.find("\"", startPos + 7); + if (endPos == std::string::npos) { + return false; + } + m_etag = result = resultString.substr(startPos + 7, endPos - startPos - 7); + return true; +} + // --------------------------------------------------------------------------- AmazonS3Download::~AmazonS3Download() {} diff --git a/src/S3Commands.hh b/src/S3Commands.hh index a86dfe3..7b520e9 100644 --- a/src/S3Commands.hh +++ b/src/S3Commands.hh @@ -247,7 +247,11 @@ class AmazonS3SendMultipartPart final : public AmazonRequest { const std::string &partNumber, const std::string &uploadId, size_t payloadSize, bool final); - protected: + // Retrieve the ETag header from the returned headers; + bool GetEtag(std::string &result); + + private: + std::string m_etag; }; class AmazonS3Download : public AmazonRequest { diff --git a/src/S3File.cc b/src/S3File.cc index feb7779..67a7f39 100644 --- a/src/S3File.cc +++ b/src/S3File.cc @@ -289,11 +289,14 @@ ssize_t S3File::SendPartStreaming() { return -EIO; } else { m_log.Log(LogMask::Debug, "SendPart", "upload.SendRequest() succeeded"); - std::string resultString = upload_part_request.getResultString(); - std::size_t startPos = resultString.find("ETag:"); - std::size_t endPos = resultString.find("\"", startPos + 7); - eTags.push_back( - resultString.substr(startPos + 7, endPos - startPos - 7)); + std::string etag; + if (!upload_part_request.GetEtag(etag)) { + m_log.Log( + LogMask::Debug, "SendPart", + "upload.SendRequest() response missing an eTag in response"); + return -EIO; + } + eTags.push_back(etag); partNumber++; m_streaming_buffer.clear(); } @@ -333,26 +336,14 @@ ssize_t S3File::ContinueSendPart(const void *buffer, size_t size) { if (is_final) { m_part_written = 0; m_part_size = 0; - auto &resultString = m_write_op->getResultString(); - std::size_t startPos = resultString.find("ETag:"); - if (startPos == std::string::npos) { - m_log.Emsg("Write", "Result from S3 does not include ETag:", - resultString.c_str()); - m_write_op.reset(); - m_write_offset = -1; - return -EIO; - } - std::size_t endPos = resultString.find('"', startPos + 7); - if (startPos == std::string::npos) { - m_log.Emsg("Write", - "Result from S3 does not include ETag end-character:", - resultString.c_str()); + std::string etag; + if (!m_write_op->GetEtag(etag)) { + m_log.Emsg("Write", "Result from S3 does not include ETag"); m_write_op.reset(); m_write_offset = -1; return -EIO; } - eTags.push_back( - resultString.substr(startPos + 7, endPos - startPos - 7)); + eTags.push_back(etag); m_write_op.reset(); partNumber++; } From 74da355eff8152636c39bc1777968f95db32c06e Mon Sep 17 00:00:00 2001 From: Brian Bockelman Date: Sun, 15 Dec 2024 15:25:21 -0600 Subject: [PATCH 2/2] Add unit test for case-insensitive etag parsing --- src/HTTPCommands.cc | 1 + src/HTTPCommands.hh | 4 ++++ src/S3Commands.hh | 2 +- test/s3_unit_tests.cc | 51 +++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 57 insertions(+), 1 deletion(-) diff --git a/src/HTTPCommands.cc b/src/HTTPCommands.cc index 7e24305..d43a10c 100644 --- a/src/HTTPCommands.cc +++ b/src/HTTPCommands.cc @@ -705,6 +705,7 @@ bool HTTPRequest::Fail(const std::string &ecode, const std::string &emsg) { void HTTPRequest::Notify() { std::lock_guard lk(m_mtx); m_result_ready = true; + modifyResponse(m_result); m_cv.notify_one(); } diff --git a/src/HTTPCommands.hh b/src/HTTPCommands.hh index 3e33f64..47a3d6a 100644 --- a/src/HTTPCommands.hh +++ b/src/HTTPCommands.hh @@ -162,6 +162,10 @@ class HTTPRequest { // call to send more data. bool Timeout() const { return m_timeout; } + // Function that can be overridden by test cases, allowing modification + // of the server response + virtual void modifyResponse(std::string &) {} + typedef std::map AttributeValueMap; AttributeValueMap query_parameters; AttributeValueMap headers; diff --git a/src/S3Commands.hh b/src/S3Commands.hh index 7b520e9..271dd5e 100644 --- a/src/S3Commands.hh +++ b/src/S3Commands.hh @@ -217,7 +217,7 @@ class AmazonS3CompleteMultipartUpload : public AmazonRequest { protected: }; -class AmazonS3SendMultipartPart final : public AmazonRequest { +class AmazonS3SendMultipartPart : public AmazonRequest { using AmazonRequest::SendRequest; public: diff --git a/test/s3_unit_tests.cc b/test/s3_unit_tests.cc index 8fcf1cb..0922997 100644 --- a/test/s3_unit_tests.cc +++ b/test/s3_unit_tests.cc @@ -112,6 +112,11 @@ s3.end } public: + std::unique_ptr GetFS() { + return std::unique_ptr( + new S3FileSystem(&m_log, m_configfn.c_str(), nullptr)); + } + void WritePattern(const std::string &name, const off_t writeSize, const unsigned char chunkByte, const size_t chunkSize, bool known_size) { @@ -260,6 +265,8 @@ s3.end rv = fh->Close(); ASSERT_EQ(rv, 0); } + + XrdSysLogger m_log; }; // Upload a single byte into S3 @@ -576,6 +583,50 @@ TEST_F(FileSystemS3Fixture, StressGet) { } } +class AmazonS3SendMultipartPartLowercase : public AmazonS3SendMultipartPart { + protected: + virtual void modifyResponse(std::string &resp) override { + std::transform(resp.begin(), resp.end(), resp.begin(), + [](unsigned char c) { return std::tolower(c); }); + } +}; + +TEST_F(FileSystemS3Fixture, Etag) { + // Determine the S3 info. + auto oss = GetFS(); + std::string exposedPath, object; + std::string path = "/test/etag_casesensitive_test"; + ASSERT_EQ(oss->parsePath(path.c_str(), exposedPath, object), 0); + + auto ai = oss->getS3AccessInfo(exposedPath, object); + ASSERT_NE(ai.get(), nullptr); + ASSERT_NE(ai->getS3BucketName(), ""); + ASSERT_NE(object, ""); + + // Start an upload. + XrdSysLogger log; + XrdSysError err(&log, "test"); + AmazonS3CreateMultipartUpload startUpload(*ai, object, err); + ASSERT_TRUE(startUpload.SendRequest()); + std::string uploadId, errMsg; + startUpload.Results(uploadId, errMsg); + + // Upload an etag. + AmazonS3SendMultipartPart upload_part_request(*ai, object, err); + std::string streaming_buffer = "aaaa"; + ASSERT_TRUE(upload_part_request.SendRequest(streaming_buffer, + std::to_string(1), uploadId, + streaming_buffer.size(), true)); + std::string etag; + ASSERT_TRUE(upload_part_request.GetEtag(etag)); + std::vector eTags; + eTags.push_back(etag); + + // Finalize the object + AmazonS3CompleteMultipartUpload complete_upload_request(*ai, object, err); + ASSERT_TRUE(complete_upload_request.SendRequest(eTags, 2, uploadId)); +} + int main(int argc, char **argv) { ::testing::InitGoogleTest(&argc, argv); return RUN_ALL_TESTS();