Skip to content

Commit 2c5efd2

Browse files
committed
[BUG] Check what curl_global_init returns (#4434)
Ensure HttpCurlGlobalInitializer checks the return value of curl_global_init. If initialization fails, log an error, avoid calling curl_global_cleanup, and cause HttpClient and HttpClientSync to report SessionState::CreateFailed. Fixes #4434 Signed-off-by: Varun <v08pandey@gmail.com>
1 parent 3dc106f commit 2c5efd2

3 files changed

Lines changed: 149 additions & 12 deletions

File tree

‎ext/include/opentelemetry/ext/http/client/curl/http_client_curl.h‎

Lines changed: 32 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -51,10 +51,24 @@ class HttpCurlGlobalInitializer
5151

5252
HttpCurlGlobalInitializer();
5353

54+
friend class HttpClientTestPeer;
55+
56+
// Test constructor to simulate init failure without calling curl_global_init
57+
HttpCurlGlobalInitializer(CURLcode code, bool is_initialized)
58+
: init_code_{code}, is_initialized_{is_initialized}
59+
{}
60+
61+
CURLcode init_code_{CURLE_OK};
62+
bool is_initialized_{false};
63+
5464
public:
5565
~HttpCurlGlobalInitializer();
5666

5767
static nostd::shared_ptr<HttpCurlGlobalInitializer> GetInstance();
68+
69+
// added accessors here
70+
bool IsValid() const noexcept { return is_initialized_; }
71+
CURLcode GetStatus() const noexcept { return init_code_; }
5872
};
5973

6074
class Request : public opentelemetry::ext::http::client::Request
@@ -253,6 +267,11 @@ class HttpClientSync : public opentelemetry::ext::http::client::HttpClientSync
253267
const opentelemetry::ext::http::client::Headers &headers,
254268
const opentelemetry::ext::http::client::Compression &compression) noexcept override
255269
{
270+
if (!curl_global_initializer_ || !curl_global_initializer_->IsValid())
271+
{
272+
return opentelemetry::ext::http::client::Result(
273+
nullptr, opentelemetry::ext::http::client::SessionState::CreateFailed);
274+
}
256275
opentelemetry::ext::http::client::Body body;
257276

258277
HttpOperation curl_operation(opentelemetry::ext::http::client::Method::Get, url.data(),
@@ -283,6 +302,11 @@ class HttpClientSync : public opentelemetry::ext::http::client::HttpClientSync
283302
const opentelemetry::ext::http::client::Headers &headers,
284303
const opentelemetry::ext::http::client::Compression &compression) noexcept override
285304
{
305+
if (!curl_global_initializer_ || !curl_global_initializer_->IsValid())
306+
{
307+
return opentelemetry::ext::http::client::Result(
308+
nullptr, opentelemetry::ext::http::client::SessionState::CreateFailed);
309+
}
286310
HttpOperation curl_operation(opentelemetry::ext::http::client::Method::Post, url.data(),
287311
ssl_options, nullptr, headers, body, compression);
288312
curl_operation.SendSync();
@@ -308,6 +332,7 @@ class HttpClientSync : public opentelemetry::ext::http::client::HttpClientSync
308332
~HttpClientSync() override {}
309333

310334
private:
335+
friend class HttpClientTestPeer;
311336
nostd::shared_ptr<HttpCurlGlobalInitializer> curl_global_initializer_;
312337
};
313338

@@ -325,6 +350,12 @@ class HttpClient : public opentelemetry::ext::http::client::HttpClient
325350

326351
~HttpClient() override;
327352

353+
inline bool IsValid() const noexcept
354+
{
355+
return curl_global_initializer_ && curl_global_initializer_->IsValid() &&
356+
multi_handle_ != nullptr;
357+
}
358+
328359
std::shared_ptr<opentelemetry::ext::http::client::Session> CreateSession(
329360
nostd::string_view url) noexcept override;
330361

@@ -366,6 +397,7 @@ class HttpClient : public opentelemetry::ext::http::client::HttpClient
366397
bool doRetrySessions(bool report_all);
367398
void resetMultiHandle();
368399

400+
nostd::shared_ptr<HttpCurlGlobalInitializer> curl_global_initializer_;
369401
std::mutex multi_handle_m_;
370402
CURLM *multi_handle_;
371403
std::atomic<uint64_t> next_session_id_{0};
@@ -387,8 +419,6 @@ class HttpClient : public opentelemetry::ext::http::client::HttpClient
387419

388420
std::chrono::milliseconds background_thread_wait_for_;
389421
std::atomic<bool> is_shutdown_{false};
390-
391-
nostd::shared_ptr<HttpCurlGlobalInitializer> curl_global_initializer_;
392422
};
393423

394424
} // namespace curl

‎ext/src/http/client/curl/http_client_curl.cc‎

Lines changed: 37 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -48,13 +48,21 @@ namespace curl
4848
{
4949

5050
HttpCurlGlobalInitializer::HttpCurlGlobalInitializer()
51+
: init_code_(curl_global_init(CURL_GLOBAL_ALL)), is_initialized_(init_code_ == CURLE_OK)
5152
{
52-
curl_global_init(CURL_GLOBAL_ALL);
53+
if (!is_initialized_)
54+
{
55+
OTEL_INTERNAL_LOG_ERROR("[HTTP Client Curl] curl_global_init failed with error code: "
56+
<< static_cast<int>(init_code_));
57+
}
5358
}
5459

5560
HttpCurlGlobalInitializer::~HttpCurlGlobalInitializer()
5661
{
57-
curl_global_cleanup();
62+
if (is_initialized_)
63+
{
64+
curl_global_cleanup();
65+
}
5866
}
5967

6068
nostd::shared_ptr<HttpCurlGlobalInitializer> HttpCurlGlobalInitializer::GetInstance()
@@ -141,6 +149,18 @@ static int deflateInPlace(z_stream *strm, unsigned char *buf, uint32_t len, uint
141149
void Session::SendRequest(
142150
std::shared_ptr<opentelemetry::ext::http::client::EventHandler> callback) noexcept
143151
{
152+
// Returning early before MaybeSpawnBackgroundThread() ensures the background IO
153+
// loop never starts with an uninitialized or null multi_handle_.
154+
if (!http_client_.IsValid())
155+
{
156+
if (callback)
157+
{
158+
callback->OnEvent(opentelemetry::ext::http::client::SessionState::CreateFailed,
159+
"curl initialization failed");
160+
}
161+
is_session_active_.store(false, std::memory_order_release);
162+
return;
163+
}
144164
is_session_active_.store(true, std::memory_order_release);
145165
const auto &url = host_ + http_request_->uri_;
146166
auto callback_ptr = callback.get();
@@ -271,24 +291,28 @@ void Session::FinishOperation()
271291
}
272292

273293
HttpClient::HttpClient()
274-
: multi_handle_(curl_multi_init()),
294+
: curl_global_initializer_(HttpCurlGlobalInitializer::GetInstance()),
295+
multi_handle_(curl_global_initializer_ && curl_global_initializer_->IsValid()
296+
? curl_multi_init()
297+
: nullptr),
275298
next_session_id_{0},
276299
max_sessions_per_connection_{8},
277300
background_thread_instrumentation_(nullptr),
278301
scheduled_delay_milliseconds_{std::chrono::milliseconds(256)},
279-
background_thread_wait_for_{std::chrono::minutes{1}},
280-
curl_global_initializer_(HttpCurlGlobalInitializer::GetInstance())
302+
background_thread_wait_for_{std::chrono::minutes{1}}
281303
{}
282304

283305
HttpClient::HttpClient(
284306
const std::shared_ptr<sdk::common::ThreadInstrumentation> &thread_instrumentation)
285-
: multi_handle_(curl_multi_init()),
307+
: curl_global_initializer_(HttpCurlGlobalInitializer::GetInstance()),
308+
multi_handle_(curl_global_initializer_ && curl_global_initializer_->IsValid()
309+
? curl_multi_init()
310+
: nullptr),
286311
next_session_id_{0},
287312
max_sessions_per_connection_{8},
288313
background_thread_instrumentation_(thread_instrumentation),
289314
scheduled_delay_milliseconds_{std::chrono::milliseconds(256)},
290-
background_thread_wait_for_{std::chrono::minutes{1}},
291-
curl_global_initializer_(HttpCurlGlobalInitializer::GetInstance())
315+
background_thread_wait_for_{std::chrono::minutes{1}}
292316
{}
293317

294318
HttpClient::~HttpClient()
@@ -317,7 +341,11 @@ HttpClient::~HttpClient()
317341
}
318342
{
319343
std::lock_guard<std::mutex> lock_guard{multi_handle_m_};
320-
curl_multi_cleanup(multi_handle_);
344+
if (multi_handle_)
345+
{
346+
curl_multi_cleanup(multi_handle_);
347+
multi_handle_ = nullptr;
348+
}
321349
}
322350
}
323351

‎ext/test/http/curl_http_test.cc‎

Lines changed: 80 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,11 @@
11
// Copyright The OpenTelemetry Authors
22
// SPDX-License-Identifier: Apache-2.0
33

4+
#include <curl/curl.h>
45
#include <curl/curlver.h>
56
#include "gtest/gtest.h"
67

78
#ifdef ENABLE_OTLP_RETRY_PREVIEW
8-
# include <curl/curl.h>
99
# include "gmock/gmock.h"
1010
#endif // ENABLE_OTLP_RETRY_PREVIEW
1111

@@ -56,6 +56,35 @@ class HttpClientTestPeer
5656
{
5757
public:
5858
static void ResetMultiHandle(HttpClient &client) { client.resetMultiHandle(); }
59+
// mock initializer
60+
static nostd::shared_ptr<HttpCurlGlobalInitializer> CreateMockInitializer(CURLcode code,
61+
bool is_valid)
62+
{
63+
return nostd::shared_ptr<HttpCurlGlobalInitializer>(
64+
new HttpCurlGlobalInitializer(code, is_valid));
65+
}
66+
// injects the mock initializer into an HttpClient
67+
static void SetInitializer(HttpClient &client,
68+
const nostd::shared_ptr<HttpCurlGlobalInitializer> &initializer)
69+
{
70+
client.curl_global_initializer_ = initializer;
71+
if (!initializer || !initializer->IsValid())
72+
{
73+
std::lock_guard<std::mutex> lk(client.multi_handle_m_);
74+
if (client.multi_handle_)
75+
{
76+
curl_multi_cleanup(client.multi_handle_);
77+
client.multi_handle_ = nullptr;
78+
}
79+
}
80+
}
81+
82+
// injects the mock initializer into an HttpClientSync
83+
static void SetSyncInitializer(HttpClientSync &client,
84+
const nostd::shared_ptr<HttpCurlGlobalInitializer> &initializer)
85+
{
86+
client.curl_global_initializer_ = initializer;
87+
}
5988
};
6089
} // namespace curl
6190
} // namespace client
@@ -77,6 +106,10 @@ class CustomEventHandler : public http_client::EventHandler
77106
{
78107
switch (state)
79108
{
109+
// CreateFailed occurs when initialization or request preparation fails before
110+
// network dispatch (e.g., if curl_global_init failed or session setup failed).
111+
// Marking is_called_ ensures tests expecting early failure notifications register it.
112+
case http_client::SessionState::CreateFailed:
80113
case http_client::SessionState::ConnectFailed:
81114
case http_client::SessionState::SendFailed: {
82115
is_called_.store(true, std::memory_order_release);
@@ -1095,6 +1128,52 @@ TEST_F(BasicCurlHttpTests, GzipIncompressibleData)
10951128
session_manager->CancelAllSessions();
10961129
session_manager->FinishAllSessions();
10971130
}
1131+
10981132
#endif // ENABLE_OTLP_COMPRESSION_PREVIEW
1133+
// 2 new tests
1134+
TEST_F(BasicCurlHttpTests, GlobalInitFailureHttpClient)
1135+
{
1136+
curl::HttpClient client;
1137+
auto mock_init =
1138+
http_client::curl::HttpClientTestPeer::CreateMockInitializer(CURLE_FAILED_INIT, false);
1139+
http_client::curl::HttpClientTestPeer::SetInitializer(client, mock_init);
1140+
1141+
EXPECT_FALSE(client.IsValid());
1142+
1143+
auto session = client.CreateSession("http://127.0.0.1:19000/get/");
1144+
auto request = session->CreateRequest();
1145+
request->SetMethod(http_client::Method::Get);
1146+
request->SetUri("get/");
1147+
1148+
auto handler = std::make_shared<CustomEventHandler>();
1149+
session->SendRequest(handler);
1150+
1151+
EXPECT_FALSE(session->IsSessionActive());
1152+
EXPECT_TRUE(handler->is_called_.load(std::memory_order_acquire));
1153+
EXPECT_FALSE(handler->got_response_.load(std::memory_order_acquire));
1154+
1155+
EXPECT_TRUE(client.CancelAllSessions());
1156+
EXPECT_TRUE(client.FinishAllSessions());
1157+
}
1158+
1159+
TEST_F(BasicCurlHttpTests, GlobalInitFailureHttpClientSync)
1160+
{
1161+
curl::HttpClientSync sync_client;
1162+
auto mock_init =
1163+
http_client::curl::HttpClientTestPeer::CreateMockInitializer(CURLE_FAILED_INIT, false);
1164+
http_client::curl::HttpClientTestPeer::SetSyncInitializer(sync_client, mock_init);
1165+
1166+
http_client::HttpSslOptions ssl_opts;
1167+
auto get_result =
1168+
sync_client.Get("http://127.0.0.1:19000/get/", ssl_opts, {}, http_client::Compression::kNone);
1169+
EXPECT_FALSE(get_result);
1170+
EXPECT_EQ(get_result.GetSessionState(), http_client::SessionState::CreateFailed);
1171+
1172+
http_client::Body body;
1173+
auto post_result = sync_client.Post("http://127.0.0.1:19000/post/", ssl_opts, body, {},
1174+
http_client::Compression::kNone);
1175+
EXPECT_FALSE(post_result);
1176+
EXPECT_EQ(post_result.GetSessionState(), http_client::SessionState::CreateFailed);
1177+
}
10991178

11001179
} // namespace

0 commit comments

Comments
 (0)