Skip to content

Commit

Permalink
cleanup: fix UBSAN errors
Browse files Browse the repository at this point in the history
  • Loading branch information
coryan committed Jul 12, 2024
1 parent dccf837 commit 3db9092
Show file tree
Hide file tree
Showing 9 changed files with 61 additions and 24 deletions.
3 changes: 3 additions & 0 deletions .bazelrc
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,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
Expand Down
17 changes: 12 additions & 5 deletions ci/cloudbuild/notifiers/alerts/function/function.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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<curl_slist, decltype(&curl_slist_free_all)>;
auto const headers =
Headers{curl_slist_append(nullptr, kContentType), curl_slist_free_all};
using CurlHandle = std::unique_ptr<CURL, decltype(&curl_easy_cleanup)>;
auto curl = CurlHandle(curl_easy_init(), curl_easy_cleanup);
using Headers = std::unique_ptr<curl_slist, CurlSListFreeAll>;
auto const headers = Headers{curl_slist_append(nullptr, kContentType)};
using CurlHandle = std::unique_ptr<CURL, CurlPtrCleanup>;
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());
Expand Down
18 changes: 13 additions & 5 deletions examples/grpc_credential_types.cc
Original file line number Diff line number Diff line change
Expand Up @@ -41,18 +41,26 @@ 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<std::string> HttpGet(std::string const& url,
std::string const& token) {
static auto const kCurlInit = [] {
return curl_global_init(CURL_GLOBAL_ALL);
}();
(void)kCurlInit;
auto const authorization = "Authorization: Bearer " + token;
using Headers = std::unique_ptr<curl_slist, decltype(&curl_slist_free_all)>;
auto const headers = Headers{
curl_slist_append(nullptr, authorization.c_str()), curl_slist_free_all};
using CurlHandle = std::unique_ptr<CURL, decltype(&curl_easy_cleanup)>;
auto curl = CurlHandle(curl_easy_init(), curl_easy_cleanup);
using Headers = std::unique_ptr<curl_slist, CurlSListFreeAll>;
auto const headers =
Headers{curl_slist_append(nullptr, authorization.c_str())};
using CurlHandle = std::unique_ptr<CURL, CurlPtrCleanup>;
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());
Expand Down
4 changes: 2 additions & 2 deletions google/cloud/internal/curl_handle.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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);
}
Expand Down
5 changes: 2 additions & 3 deletions google/cloud/internal/curl_handle.h
Original file line number Diff line number Diff line change
Expand Up @@ -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<int>(s.length())),
&curl_free);
return CurlString(curl_easy_escape(handle_.get(), s.data(),
static_cast<int>(s.length())));
}

template <typename T>
Expand Down
4 changes: 2 additions & 2 deletions google/cloud/internal/curl_handle_factory.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
1 change: 0 additions & 1 deletion google/cloud/internal/curl_impl.cc
Original file line number Diff line number Diff line change
Expand Up @@ -169,7 +169,6 @@ CurlImpl::CurlImpl(CurlHandle handle,
std::shared_ptr<CurlHandleFactory> factory,
Options const& options)
: factory_(std::move(factory)),
request_headers_(nullptr, &curl_slist_free_all),
handle_(std::move(handle)),
multi_(factory_->CreateMultiHandle()) {
CurlInitializeOnce(options);
Expand Down
2 changes: 1 addition & 1 deletion google/cloud/internal/curl_wrappers.cc
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Expand Down
31 changes: 26 additions & 5 deletions google/cloud/internal/curl_wrappers.h
Original file line number Diff line number Diff line change
Expand Up @@ -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<CURL, decltype(&curl_easy_cleanup)>;
using CurlPtr = std::unique_ptr<CURL, CurlPtrCleanup>;

/// 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<CURLM, decltype(&curl_multi_cleanup)>;
using CurlMulti = std::unique_ptr<CURLM, CurlMultiPtrCleanup>;

/// Hold a character string created by CURL use correct deleter.
using CurlString = std::unique_ptr<char, decltype(&curl_free)>;
using CurlString = std::unique_ptr<char, CurlFree>;

using CurlHeaders = std::unique_ptr<curl_slist, decltype(&curl_slist_free_all)>;
using CurlHeaders = std::unique_ptr<curl_slist, CurlSListFreeAll>;

using CurlReceivedHeaders = std::multimap<std::string, std::string>;
std::size_t CurlAppendHeaderData(CurlReceivedHeaders& received_headers,
Expand All @@ -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<CURLSH, decltype(&curl_share_cleanup)>;
using CurlShare = std::unique_ptr<CURLSH, CurlShareCleanup>;

/// Returns true if the SSL locking callbacks are installed.
bool SslLockingCallbacksInstalled();
Expand Down

0 comments on commit 3db9092

Please sign in to comment.