From 0dc9cdd968eacc2b6e60fc2eb9ab551bec8c6ec2 Mon Sep 17 00:00:00 2001 From: Carlos O'Ryan Date: Fri, 12 Jul 2024 18:41:32 +0000 Subject: [PATCH] cleanup: fix UBSAN errors --- .bazelrc | 3 ++ .../notifiers/alerts/function/function.cc | 17 +++++++--- examples/grpc_credential_types.cc | 18 ++++++++--- google/cloud/internal/curl_handle.cc | 4 +-- google/cloud/internal/curl_handle.h | 5 ++- google/cloud/internal/curl_handle_factory.cc | 4 +-- google/cloud/internal/curl_impl.cc | 1 - google/cloud/internal/curl_wrappers.cc | 2 +- google/cloud/internal/curl_wrappers.h | 31 ++++++++++++++++--- 9 files changed, 61 insertions(+), 24 deletions(-) diff --git a/.bazelrc b/.bazelrc index 58ddbc9077436..f90f9311b93e1 100644 --- a/.bazelrc +++ b/.bazelrc @@ -97,6 +97,9 @@ build:tsan --action_env=TSAN_OPTIONS=halt_on_error=1:second_deadlock_stack=1:rep build:ubsan --config=sanitizer build:ubsan --config=libcxx build:ubsan --copt=-fsanitize=undefined +# By default libcurl calls functions through pointers not matching the original +# type. +build:ubsan --copt=-DCURL_STRICTER build:ubsan --linkopt=-fsanitize=undefined build:ubsan --linkopt=-fsanitize-link-c++-runtime build:ubsan --action_env=UBSAN_OPTIONS=halt_on_error=1:print_stacktrace=1 diff --git a/ci/cloudbuild/notifiers/alerts/function/function.cc b/ci/cloudbuild/notifiers/alerts/function/function.cc index 8edb923de54be..96f3e265d1f93 100644 --- a/ci/cloudbuild/notifiers/alerts/function/function.cc +++ b/ci/cloudbuild/notifiers/alerts/function/function.cc @@ -60,13 +60,20 @@ nlohmann::json MakeChatPayload(BuildStatus const& bs) { return nlohmann::json{{"text", std::move(text)}}; } +struct CurlPtrCleanup { + void operator()(CURL* arg) const { return curl_easy_cleanup(arg); } +}; + +struct CurlSListFreeAll { + void operator()(curl_slist* arg) const { return curl_slist_free_all(arg); } +}; + void HttpPost(std::string const& url, std::string const& data) { static constexpr auto kContentType = "Content-Type: application/json"; - using Headers = std::unique_ptr; - auto const headers = - Headers{curl_slist_append(nullptr, kContentType), curl_slist_free_all}; - using CurlHandle = std::unique_ptr; - auto curl = CurlHandle(curl_easy_init(), curl_easy_cleanup); + using Headers = std::unique_ptr; + auto const headers = Headers{curl_slist_append(nullptr, kContentType)}; + using CurlHandle = std::unique_ptr; + auto curl = CurlHandle(curl_easy_init()); if (!curl) throw std::runtime_error("Failed to create CurlHandle"); curl_easy_setopt(curl.get(), CURLOPT_URL, url.c_str()); curl_easy_setopt(curl.get(), CURLOPT_HTTPHEADER, headers.get()); diff --git a/examples/grpc_credential_types.cc b/examples/grpc_credential_types.cc index 9f6d7ccc9a7e5..e0da5f889ac76 100644 --- a/examples/grpc_credential_types.cc +++ b/examples/grpc_credential_types.cc @@ -41,6 +41,14 @@ extern "C" size_t CurlOnWriteData(char* ptr, size_t size, size_t nmemb, return size * nmemb; } +struct CurlPtrCleanup { + void operator()(CURL* arg) const { return curl_easy_cleanup(arg); } +}; + +struct CurlSListFreeAll { + void operator()(curl_slist* arg) const { return curl_slist_free_all(arg); } +}; + google::cloud::StatusOr HttpGet(std::string const& url, std::string const& token) { static auto const kCurlInit = [] { @@ -48,11 +56,11 @@ google::cloud::StatusOr HttpGet(std::string const& url, }(); (void)kCurlInit; auto const authorization = "Authorization: Bearer " + token; - using Headers = std::unique_ptr; - auto const headers = Headers{ - curl_slist_append(nullptr, authorization.c_str()), curl_slist_free_all}; - using CurlHandle = std::unique_ptr; - auto curl = CurlHandle(curl_easy_init(), curl_easy_cleanup); + using Headers = std::unique_ptr; + auto const headers = + Headers{curl_slist_append(nullptr, authorization.c_str())}; + using CurlHandle = std::unique_ptr; + auto curl = CurlHandle(curl_easy_init()); if (!curl) throw std::runtime_error("Failed to create CurlHandle"); std::string buffer; curl_easy_setopt(curl.get(), CURLOPT_URL, url.c_str()); diff --git a/google/cloud/internal/curl_handle.cc b/google/cloud/internal/curl_handle.cc index a59005eb39f50..2739613ef1d26 100644 --- a/google/cloud/internal/curl_handle.cc +++ b/google/cloud/internal/curl_handle.cc @@ -123,13 +123,13 @@ CurlHandle CurlHandle::MakeFromPool(CurlHandleFactory& factory) { } void CurlHandle::ReturnToPool(CurlHandleFactory& factory, CurlHandle h) { - CurlPtr tmp(nullptr, curl_easy_cleanup); + CurlPtr tmp; h.handle_.swap(tmp); factory.CleanupHandle(std::move(tmp), HandleDisposition::kKeep); } void CurlHandle::DiscardFromPool(CurlHandleFactory& factory, CurlHandle h) { - CurlPtr tmp(nullptr, curl_easy_cleanup); + CurlPtr tmp; h.handle_.swap(tmp); factory.CleanupHandle(std::move(tmp), HandleDisposition::kDiscard); } diff --git a/google/cloud/internal/curl_handle.h b/google/cloud/internal/curl_handle.h index 1025f7cca2a85..b6d2b5ff96a26 100644 --- a/google/cloud/internal/curl_handle.h +++ b/google/cloud/internal/curl_handle.h @@ -72,9 +72,8 @@ class CurlHandle { /// URL-escapes a string. CurlString MakeEscapedString(std::string const& s) { - return CurlString( - curl_easy_escape(handle_.get(), s.data(), static_cast(s.length())), - &curl_free); + return CurlString(curl_easy_escape(handle_.get(), s.data(), + static_cast(s.length()))); } template diff --git a/google/cloud/internal/curl_handle_factory.cc b/google/cloud/internal/curl_handle_factory.cc index 0eaaaa764ff5e..4fca68ee34d0d 100644 --- a/google/cloud/internal/curl_handle_factory.cc +++ b/google/cloud/internal/curl_handle_factory.cc @@ -65,7 +65,7 @@ void DefaultCurlHandleFactory::CleanupHandle(CurlPtr h, HandleDisposition) { } CurlMulti DefaultCurlHandleFactory::CreateMultiHandle() { - return CurlMulti(curl_multi_init(), &curl_multi_cleanup); + return CurlMulti(curl_multi_init()); } void DefaultCurlHandleFactory::CleanupMultiHandle(CurlMulti m, @@ -161,7 +161,7 @@ CurlMulti PooledCurlHandleFactory::CreateMultiHandle() { } ++active_multi_handles_; lk.unlock(); - return CurlMulti(curl_multi_init(), &curl_multi_cleanup); + return CurlMulti(curl_multi_init()); } void PooledCurlHandleFactory::CleanupMultiHandle(CurlMulti m, diff --git a/google/cloud/internal/curl_impl.cc b/google/cloud/internal/curl_impl.cc index dc21e8b99056f..4842729afa219 100644 --- a/google/cloud/internal/curl_impl.cc +++ b/google/cloud/internal/curl_impl.cc @@ -169,7 +169,6 @@ CurlImpl::CurlImpl(CurlHandle handle, std::shared_ptr factory, Options const& options) : factory_(std::move(factory)), - request_headers_(nullptr, &curl_slist_free_all), handle_(std::move(handle)), multi_(factory_->CreateMultiHandle()) { CurlInitializeOnce(options); diff --git a/google/cloud/internal/curl_wrappers.cc b/google/cloud/internal/curl_wrappers.cc index 7e9aef6137728..04b7a3b865e8b 100644 --- a/google/cloud/internal/curl_wrappers.cc +++ b/google/cloud/internal/curl_wrappers.cc @@ -231,7 +231,7 @@ bool SslLockingCallbacksInstalled() { } CurlPtr MakeCurlPtr() { - auto handle = CurlPtr(curl_easy_init(), &curl_easy_cleanup); + auto handle = CurlPtr(curl_easy_init()); // We get better performance using a slightly larger buffer (128KiB) than the // default buffer size set by libcurl (16KiB). We ignore errors because // failing to set this parameter just affects performance by a small amount. diff --git a/google/cloud/internal/curl_wrappers.h b/google/cloud/internal/curl_wrappers.h index 5ae3cb27bca0c..537025e048fe1 100644 --- a/google/cloud/internal/curl_wrappers.h +++ b/google/cloud/internal/curl_wrappers.h @@ -32,19 +32,40 @@ GOOGLE_CLOUD_CPP_INLINE_NAMESPACE_BEGIN (LIBCURL_VERSION_NUM >= ((((Ma) << 16) | ((Mi) << 8)) | (Pa))) #endif // CURL_AT_LEAST_VERSION +// Wrappers around the `curl_*` functions we need for cleanups. +struct CurlPtrCleanup { + void operator()(CURL* arg) const { return curl_easy_cleanup(arg); } +}; + +struct CurlMultiPtrCleanup { + void operator()(CURLM* arg) const { (void)curl_multi_cleanup(arg); } +}; + +struct CurlFree { + void operator()(char* arg) const { return curl_free(arg); } +}; + +struct CurlSListFreeAll { + void operator()(curl_slist* arg) const { return curl_slist_free_all(arg); } +}; + +struct CurlShareCleanup { + void operator()(CURLSH* arg) const { (void)curl_share_cleanup(arg); } +}; + /// Hold a CURL* handle and automatically clean it up. -using CurlPtr = std::unique_ptr; +using CurlPtr = std::unique_ptr; /// Create a new (wrapped) CURL* with one-time configuration options set. CurlPtr MakeCurlPtr(); /// Hold a CURLM* handle and automatically clean it up. -using CurlMulti = std::unique_ptr; +using CurlMulti = std::unique_ptr; /// Hold a character string created by CURL use correct deleter. -using CurlString = std::unique_ptr; +using CurlString = std::unique_ptr; -using CurlHeaders = std::unique_ptr; +using CurlHeaders = std::unique_ptr; using CurlReceivedHeaders = std::multimap; std::size_t CurlAppendHeaderData(CurlReceivedHeaders& received_headers, @@ -56,7 +77,7 @@ std::string DebugSendHeader(char const* data, std::size_t size); std::string DebugInData(char const* data, std::size_t size); std::string DebugOutData(char const* data, std::size_t size); -using CurlShare = std::unique_ptr; +using CurlShare = std::unique_ptr; /// Returns true if the SSL locking callbacks are installed. bool SslLockingCallbacksInstalled();