Skip to content

Commit ec35054

Browse files
dgoffredodgoffredo
authored
prevent leaking of CURL handles (#36)
1 parent 338a87e commit ec35054

File tree

2 files changed

+173
-83
lines changed

2 files changed

+173
-83
lines changed

‎src/datadog/curl.cpp‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -506,6 +506,7 @@ void CurlImpl::run() {
506506
}
507507

508508
log_on_error(curl_.multi_remove_handle(multi_handle_, handle));
509+
curl_.easy_cleanup(handle);
509510
}
510511

511512
request_handles_.clear();

‎test/test_curl.cpp‎

Lines changed: 172 additions & 83 deletions
Original file line numberDiff line numberDiff line change
@@ -9,103 +9,131 @@
99
#include <chrono>
1010
#include <exception>
1111
#include <system_error>
12+
#include <unordered_set>
1213

1314
#include "mocks/loggers.h"
1415
#include "test.h"
1516

1617
using namespace datadog::tracing;
1718

18-
TEST_CASE("parse response headers and body") {
19-
class MockCurlLibrary : public CurlLibrary {
20-
void *user_data_on_header_ = nullptr;
21-
HeaderCallback on_header_ = nullptr;
22-
void *user_data_on_write_ = nullptr;
23-
WriteCallback on_write_ = nullptr;
24-
CURL *handle_ = nullptr;
25-
CURLMsg message_;
26-
27-
CURLcode easy_getinfo_response_code(CURL *, long *code) override {
28-
*code = 200;
29-
return CURLE_OK;
30-
}
31-
CURLcode easy_setopt_headerdata(CURL *, void *data) override {
32-
user_data_on_header_ = data;
33-
return CURLE_OK;
34-
}
35-
CURLcode easy_setopt_headerfunction(CURL *,
36-
HeaderCallback on_header) override {
37-
on_header_ = on_header;
38-
return CURLE_OK;
39-
}
40-
CURLcode easy_setopt_writedata(CURL *, void *data) override {
41-
user_data_on_write_ = data;
42-
return CURLE_OK;
43-
}
19+
class SingleRequestMockCurlLibrary : public CurlLibrary {
20+
public:
21+
void *user_data_on_header_ = nullptr;
22+
HeaderCallback on_header_ = nullptr;
23+
void *user_data_on_write_ = nullptr;
24+
WriteCallback on_write_ = nullptr;
25+
CURL *added_handle_ = nullptr;
26+
CURLMsg message_;
27+
// Since `SingleRequestMockCurlLibrary` supports at most one request,
28+
// `created_handles_` and `destroyed_handles_` will have size zero or one.
29+
std::unordered_multiset<CURL *> created_handles_;
30+
std::unordered_multiset<CURL *> destroyed_handles_;
31+
// `message_result_` is the success/error code associated with the "done"
32+
// message sent to the event loop when the request has finished.
33+
CURLcode message_result_ = CURLE_OK;
34+
// `delay_message_` is used to prevent the immediate dispatch of a "done"
35+
// message to the event loop. This allows races to be explored between request
36+
// registration and `Curl` shutdown.
37+
bool delay_message_ = false;
38+
39+
void easy_cleanup(CURL *handle) override {
40+
destroyed_handles_.insert(handle);
41+
CurlLibrary::easy_cleanup(handle);
42+
}
4443

45-
CURLcode easy_setopt_writefunction(CURL *,
46-
WriteCallback on_write) override {
47-
on_write_ = on_write;
48-
return CURLE_OK;
49-
}
50-
CURLMcode multi_add_handle(CURLM *, CURL *easy_handle) override {
51-
handle_ = easy_handle;
52-
return CURLM_OK;
53-
}
54-
CURLMsg *multi_info_read(CURLM *, int *msgs_in_queue) override {
55-
*msgs_in_queue = handle_ != nullptr;
56-
if (*msgs_in_queue == 0) {
57-
return nullptr;
58-
}
59-
message_.msg = CURLMSG_DONE;
60-
message_.easy_handle = handle_;
61-
message_.data.result = CURLE_OK;
62-
return &message_;
63-
}
64-
CURLMcode multi_perform(CURLM *, int *running_handles) override {
65-
if (!handle_) {
66-
*running_handles = 0;
67-
return CURLM_OK;
68-
}
44+
CURL *easy_init() override {
45+
CURL *handle = CurlLibrary::easy_init();
46+
created_handles_.insert(handle);
47+
return handle;
48+
}
6949

70-
// If any of these `REQUIRE`s fail, an exception will be thrown and the
71-
// test will abort. The runtime will print the exception first, though.
72-
REQUIRE(on_header_);
73-
REQUIRE(user_data_on_header_);
74-
*running_handles = 1;
75-
std::string header = "200 OK";
76-
REQUIRE(on_header_(header.data(), 1, header.size(),
77-
user_data_on_header_) == header.size());
78-
header = "Foo-Bar: baz";
79-
REQUIRE(on_header_(header.data(), 1, header.size(),
80-
user_data_on_header_) == header.size());
81-
header = "BOOM-BOOM: boom, boom, boom, boom ";
82-
REQUIRE(on_header_(header.data(), 1, header.size(),
83-
user_data_on_header_) == header.size());
84-
header = "BOOM-boom: ignored";
85-
REQUIRE(on_header_(header.data(), 1, header.size(),
86-
user_data_on_header_) == header.size());
87-
88-
REQUIRE(on_write_);
89-
REQUIRE(user_data_on_write_);
90-
std::string body = "{\"message\": \"Dogs don't know it's not libcurl!\"}";
91-
// Send the body in two pieces.
92-
REQUIRE(on_write_(body.data(), 1, body.size() / 2, user_data_on_write_) ==
93-
body.size() / 2);
94-
const auto remaining = body.size() - (body.size() / 2);
95-
REQUIRE(on_write_(body.data() + body.size() / 2, 1, remaining,
96-
user_data_on_write_) == remaining);
50+
CURLcode easy_getinfo_response_code(CURL *, long *code) override {
51+
*code = 200;
52+
return CURLE_OK;
53+
}
54+
CURLcode easy_setopt_headerdata(CURL *, void *data) override {
55+
user_data_on_header_ = data;
56+
return CURLE_OK;
57+
}
58+
CURLcode easy_setopt_headerfunction(CURL *,
59+
HeaderCallback on_header) override {
60+
on_header_ = on_header;
61+
return CURLE_OK;
62+
}
63+
CURLcode easy_setopt_writedata(CURL *, void *data) override {
64+
user_data_on_write_ = data;
65+
return CURLE_OK;
66+
}
9767

98-
return CURLM_OK;
68+
CURLcode easy_setopt_writefunction(CURL *, WriteCallback on_write) override {
69+
on_write_ = on_write;
70+
return CURLE_OK;
71+
}
72+
CURLMcode multi_add_handle(CURLM *, CURL *easy_handle) override {
73+
added_handle_ = easy_handle;
74+
return CURLM_OK;
75+
}
76+
CURLMsg *multi_info_read(CURLM *, int *msgs_in_queue) override {
77+
if (delay_message_) {
78+
*msgs_in_queue = 0;
79+
return nullptr;
9980
}
100-
CURLMcode multi_remove_handle(CURLM *, CURL *easy_handle) override {
101-
REQUIRE(easy_handle == handle_);
102-
handle_ = nullptr;
81+
82+
*msgs_in_queue = added_handle_ != nullptr;
83+
if (*msgs_in_queue == 0) {
84+
return nullptr;
85+
}
86+
message_.msg = CURLMSG_DONE;
87+
message_.easy_handle = added_handle_;
88+
message_.data.result = message_result_;
89+
return &message_;
90+
}
91+
CURLMcode multi_perform(CURLM *, int *running_handles) override {
92+
if (!added_handle_) {
93+
*running_handles = 0;
10394
return CURLM_OK;
10495
}
105-
};
10696

97+
// If any of these `REQUIRE`s fail, an exception will be thrown and the
98+
// test will abort. The runtime will print the exception first, though.
99+
REQUIRE(on_header_);
100+
REQUIRE(user_data_on_header_);
101+
*running_handles = 1;
102+
std::string header = "200 OK";
103+
REQUIRE(on_header_(header.data(), 1, header.size(), user_data_on_header_) ==
104+
header.size());
105+
header = "Foo-Bar: baz";
106+
REQUIRE(on_header_(header.data(), 1, header.size(), user_data_on_header_) ==
107+
header.size());
108+
header = "BOOM-BOOM: boom, boom, boom, boom ";
109+
REQUIRE(on_header_(header.data(), 1, header.size(), user_data_on_header_) ==
110+
header.size());
111+
header = "BOOM-boom: ignored";
112+
REQUIRE(on_header_(header.data(), 1, header.size(), user_data_on_header_) ==
113+
header.size());
114+
115+
REQUIRE(on_write_);
116+
REQUIRE(user_data_on_write_);
117+
std::string body = "{\"message\": \"Dogs don't know it's not libcurl!\"}";
118+
// Send the body in two pieces.
119+
REQUIRE(on_write_(body.data(), 1, body.size() / 2, user_data_on_write_) ==
120+
body.size() / 2);
121+
const auto remaining = body.size() - (body.size() / 2);
122+
REQUIRE(on_write_(body.data() + body.size() / 2, 1, remaining,
123+
user_data_on_write_) == remaining);
124+
125+
return CURLM_OK;
126+
}
127+
CURLMcode multi_remove_handle(CURLM *, CURL *easy_handle) override {
128+
REQUIRE(easy_handle == added_handle_);
129+
added_handle_ = nullptr;
130+
return CURLM_OK;
131+
}
132+
};
133+
134+
TEST_CASE("parse response headers and body") {
107135
const auto logger = std::make_shared<MockLogger>();
108-
MockCurlLibrary library;
136+
SingleRequestMockCurlLibrary library;
109137
const auto client = std::make_shared<Curl>(logger, library);
110138

111139
SECTION("in the tracer") {
@@ -207,6 +235,7 @@ TEST_CASE("fail to allocate request handle") {
207235
// Each call to `Curl::post` allocates a new "easy handle." If that fails,
208236
// then `post` immediately returns an error.
209237
class MockCurlLibrary : public CurlLibrary {
238+
public:
210239
CURL *easy_init() override { return nullptr; }
211240
};
212241

@@ -344,5 +373,65 @@ TEST_CASE("setopt failures") {
344373
REQUIRE(result.error().code == Error::CURL_REQUEST_SETUP_FAILED);
345374
}
346375

376+
TEST_CASE("handles are always cleaned up") {
377+
const auto logger = std::make_shared<MockLogger>();
378+
SingleRequestMockCurlLibrary library;
379+
auto client = std::make_shared<Curl>(logger, library);
380+
381+
SECTION("when the response is delivered") {
382+
Optional<Error> post_error;
383+
std::exception_ptr exception;
384+
const HTTPClient::URL url = {"http", "whatever", ""};
385+
const auto result = client->post(
386+
url, [](const auto &) {}, "whatever",
387+
[&](int status, const DictReader & /*headers*/, std::string body) {
388+
try {
389+
REQUIRE(status == 200);
390+
REQUIRE(body ==
391+
"{\"message\": \"Dogs don't know it's not libcurl!\"}");
392+
} catch (...) {
393+
exception = std::current_exception();
394+
}
395+
},
396+
[&](const Error &error) { post_error = error; });
397+
398+
REQUIRE(result);
399+
client->drain(std::chrono::steady_clock::now() + std::chrono::seconds(1));
400+
if (exception) {
401+
std::rethrow_exception(exception);
402+
}
403+
REQUIRE_FALSE(post_error);
404+
}
405+
406+
SECTION("when an error occurs") {
407+
Optional<Error> post_error;
408+
const HTTPClient::URL url = {"http", "whatever", ""};
409+
const auto ignore = [](auto &&...) {};
410+
library.message_result_ = CURLE_COULDNT_CONNECT; // any error would do
411+
const auto result =
412+
client->post(url, ignore, "whatever", ignore,
413+
[&](const Error &error) { post_error = error; });
414+
415+
REQUIRE(result);
416+
client->drain(std::chrono::steady_clock::now() + std::chrono::seconds(1));
417+
REQUIRE(post_error);
418+
}
419+
420+
SECTION("when we shut down while a request is in flight") {
421+
const HTTPClient::URL url = {"http", "whatever", ""};
422+
const auto ignore = [](auto &&...) {};
423+
library.delay_message_ = true;
424+
const auto result = client->post(url, ignore, "whatever", ignore, ignore);
425+
426+
REQUIRE(result);
427+
// Destroy the `Curl` object.
428+
client.reset();
429+
}
430+
431+
// Here are the checks relevant to this test.
432+
REQUIRE(library.created_handles_.size() == 1);
433+
REQUIRE(library.created_handles_ == library.destroyed_handles_);
434+
}
435+
347436
// TODO: "multi_*" failures
348437
// TODO: "getinfo" failures

0 commit comments

Comments
 (0)