From eb2bd586a3040022d12b6444e08c446c385bf621 Mon Sep 17 00:00:00 2001 From: Scott Hart Date: Wed, 25 Sep 2024 15:58:32 -0400 Subject: [PATCH] fix(rest): promote buffer curl reads from to member variable --- google/cloud/internal/curl_impl.cc | 43 ++++++----------------------- google/cloud/internal/curl_impl.h | 5 ++++ google/cloud/internal/curl_writev.h | 1 + 3 files changed, 14 insertions(+), 35 deletions(-) diff --git a/google/cloud/internal/curl_impl.cc b/google/cloud/internal/curl_impl.cc index b1db29c57ad9b..ba47998b2df4e 100644 --- a/google/cloud/internal/curl_impl.cc +++ b/google/cloud/internal/curl_impl.cc @@ -114,12 +114,6 @@ static std::size_t ReadFunction( // NOLINT(misc-use-anonymous-namespace) return writev->MoveTo(absl::MakeSpan(buffer, size * nitems)); } -// Instead of trying to send any more bytes from userdata, aborts. -static std::size_t ReadFunctionAbort( // NOLINT(misc-use-anonymous-namespace) - char*, std::size_t, std::size_t, void*) { - return CURL_READFUNC_ABORT; -} - static int SeekFunction( // NOLINT(misc-use-anonymous-namespace) void* userdata, curl_off_t offset, int origin) { auto* const writev = reinterpret_cast(userdata); @@ -366,8 +360,8 @@ Status CurlImpl::MakeRequest(HttpMethod method, RestContext& context, } if (method == HttpMethod::kPost) { - WriteVector writev{std::move(request)}; - curl_off_t const size = writev.size(); + writev_ = WriteVector{std::move(request)}; + curl_off_t const size = writev_.size(); status = handle_.SetOption(CURLOPT_POSTFIELDS, nullptr); if (!status.ok()) return OnTransferError(context, std::move(status)); status = handle_.SetOption(CURLOPT_POST, 1L); @@ -376,26 +370,26 @@ Status CurlImpl::MakeRequest(HttpMethod method, RestContext& context, if (!status.ok()) return OnTransferError(context, std::move(status)); status = handle_.SetOption(CURLOPT_READFUNCTION, &ReadFunction); if (!status.ok()) return OnTransferError(context, std::move(status)); - status = handle_.SetOption(CURLOPT_READDATA, &writev); + status = handle_.SetOption(CURLOPT_READDATA, &writev_); if (!status.ok()) return OnTransferError(context, std::move(status)); status = handle_.SetOption(CURLOPT_SEEKFUNCTION, &SeekFunction); if (!status.ok()) return OnTransferError(context, std::move(status)); - status = handle_.SetOption(CURLOPT_SEEKDATA, &writev); + status = handle_.SetOption(CURLOPT_SEEKDATA, &writev_); if (!status.ok()) return OnTransferError(context, std::move(status)); SetHeader("Expect:"); return MakeRequestImpl(context); } if (method == HttpMethod::kPut || method == HttpMethod::kPatch) { - WriteVector writev{std::move(request)}; - curl_off_t const size = writev.size(); + writev_ = WriteVector{std::move(request)}; + curl_off_t const size = writev_.size(); status = handle_.SetOption(CURLOPT_READFUNCTION, &ReadFunction); if (!status.ok()) return OnTransferError(context, std::move(status)); - status = handle_.SetOption(CURLOPT_READDATA, &writev); + status = handle_.SetOption(CURLOPT_READDATA, &writev_); if (!status.ok()) return OnTransferError(context, std::move(status)); status = handle_.SetOption(CURLOPT_SEEKFUNCTION, &SeekFunction); if (!status.ok()) return OnTransferError(context, std::move(status)); - status = handle_.SetOption(CURLOPT_SEEKDATA, &writev); + status = handle_.SetOption(CURLOPT_SEEKDATA, &writev_); if (!status.ok()) return OnTransferError(context, std::move(status)); status = handle_.SetOption(CURLOPT_UPLOAD, 1L); if (!status.ok()) return OnTransferError(context, std::move(status)); @@ -487,22 +481,6 @@ std::size_t CurlImpl::HeaderCallback(absl::Span response) { response.size()); } -class CurlImpl::ReadFunctionAbortGuard { - public: - explicit ReadFunctionAbortGuard(CurlImpl& impl) : impl_(impl) {} - ~ReadFunctionAbortGuard() { - // If curl_closed_ is true, then the handle has already been recycled and - // attempting to set an option on it will error. - if (!impl_.curl_closed_) { - impl_.handle_.SetOptionUnchecked(CURLOPT_READFUNCTION, - &ReadFunctionAbort); - } - } - - private: - CurlImpl& impl_; -}; - Status CurlImpl::MakeRequestImpl(RestContext& context) { TRACE_STATE() << ", url_=" << url_; @@ -525,11 +503,6 @@ Status CurlImpl::MakeRequestImpl(RestContext& context) { handle_.SetOptionUnchecked(CURLOPT_HTTP_VERSION, VersionToCurlCode(http_version_)); - // All data in the WriteVector should be written after ReadImpl returns unless - // an error, typically a timeout, has occurred. Use ReadFunctionAbortGuard to - // leverage RAII to instruct curl to not attempt to send anymore data on this - // handle regardless if an error or exception is encountered. - ReadFunctionAbortGuard guard(*this); auto error = curl_multi_add_handle(multi_.get(), handle_.handle_.get()); // This indicates that we are using the API incorrectly. The application diff --git a/google/cloud/internal/curl_impl.h b/google/cloud/internal/curl_impl.h index ce9c8a18a994b..0faca01a2a096 100644 --- a/google/cloud/internal/curl_impl.h +++ b/google/cloud/internal/curl_impl.h @@ -18,6 +18,7 @@ #include "google/cloud/internal/curl_handle.h" #include "google/cloud/internal/curl_handle_factory.h" #include "google/cloud/internal/curl_wrappers.h" +#include "google/cloud/internal/curl_writev.h" #include "google/cloud/internal/rest_context.h" #include "google/cloud/internal/rest_request.h" #include "google/cloud/internal/rest_response.h" @@ -171,6 +172,10 @@ class CurlImpl { // Track when status and headers from the response are received. bool all_headers_received_ = false; + // writev_ is a member variable in order to ensure that its lifetime remains + // valid even if libcurl is interrupted when trying to send data. + WriteVector writev_; + // Track the unused portion of the output buffer provided to Read(). absl::Span avail_; diff --git a/google/cloud/internal/curl_writev.h b/google/cloud/internal/curl_writev.h index 00adba2853173..51c26114c7325 100644 --- a/google/cloud/internal/curl_writev.h +++ b/google/cloud/internal/curl_writev.h @@ -28,6 +28,7 @@ GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN // Vector of data chunks to satisfy requests from libcurl. class WriteVector { public: + WriteVector() = default; explicit WriteVector(std::vector> v) : original_(std::move(v)), writev_(original_.begin(), original_.end()) {}