Skip to content

Commit 3e98332

Browse files
dentinydentiny
committed
Check libcurl return value
1 parent 6f7c73f commit 3e98332

File tree

2 files changed

+49
-55
lines changed

2 files changed

+49
-55
lines changed

‎src/httpfs_curl_client.cpp‎

Lines changed: 48 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -7,9 +7,7 @@
77
#include <sys/stat.h>
88
#include "duckdb/common/exception/http_exception.hpp"
99

10-
#ifndef EMSCRIPTEN
11-
#include "httpfs_curl_client.hpp"
12-
#endif
10+
#define CHECK_CURL_OK(expr) D_ASSERT((expr) == CURLE_OK)
1311

1412
namespace duckdb {
1513

@@ -91,11 +89,11 @@ CURLHandle::CURLHandle(const string &token, const string &cert_path) {
9189
throw InternalException("Failed to initialize curl");
9290
}
9391
if (!token.empty()) {
94-
curl_easy_setopt(curl, CURLOPT_XOAUTH2_BEARER, token.c_str());
95-
curl_easy_setopt(curl, CURLOPT_HTTPAUTH, CURLAUTH_BEARER);
92+
CHECK_CURL_OK(curl_easy_setopt(curl, CURLOPT_XOAUTH2_BEARER, token.c_str()));
93+
CHECK_CURL_OK(curl_easy_setopt(curl, CURLOPT_HTTPAUTH, CURLAUTH_BEARER));
9694
}
9795
if (!cert_path.empty()) {
98-
curl_easy_setopt(curl, CURLOPT_CAINFO, cert_path.c_str());
96+
CHECK_CURL_OK(curl_easy_setopt(curl, CURLOPT_CAINFO, cert_path.c_str()));
9997
}
10098
}
10199

@@ -110,7 +108,7 @@ struct RequestInfo {
110108
std::vector<HTTPHeaders> header_collection;
111109
};
112110

113-
static idx_t httpfs_client_count = 0;
111+
static std::atomic<idx_t> httpfs_client_count {0};
114112

115113
class HTTPFSCurlClient : public HTTPClient {
116114
public:
@@ -129,45 +127,48 @@ class HTTPFSCurlClient : public HTTPClient {
129127

130128
// set curl options
131129
// follow redirects
132-
curl_easy_setopt(*curl, CURLOPT_FOLLOWLOCATION, 1L);
130+
CHECK_CURL_OK(curl_easy_setopt(*curl, CURLOPT_FOLLOWLOCATION, 1L));
133131

134132
// Curl re-uses connections by default
135133
if (!http_params.keep_alive) {
136-
curl_easy_setopt(*curl, CURLOPT_FORBID_REUSE, 1L);
134+
CHECK_CURL_OK(curl_easy_setopt(*curl, CURLOPT_FORBID_REUSE, 1L));
137135
}
138136

139137
if (http_params.enable_curl_server_cert_verification) {
140-
curl_easy_setopt(*curl, CURLOPT_SSL_VERIFYPEER, 1L); // Verify the cert
141-
curl_easy_setopt(*curl, CURLOPT_SSL_VERIFYHOST, 2L); // Verify that the cert matches the hostname
138+
CHECK_CURL_OK(curl_easy_setopt(*curl, CURLOPT_SSL_VERIFYPEER, 1L)); // Verify the cert
139+
CHECK_CURL_OK(
140+
curl_easy_setopt(*curl, CURLOPT_SSL_VERIFYHOST, 2L)); // Verify that the cert matches the hostname
142141
} else {
143-
curl_easy_setopt(*curl, CURLOPT_SSL_VERIFYPEER, 0L); // Override default, don't verify the cert
144-
curl_easy_setopt(*curl, CURLOPT_SSL_VERIFYHOST,
145-
0L); // Override default, don't verify that the cert matches the hostname
142+
CHECK_CURL_OK(
143+
curl_easy_setopt(*curl, CURLOPT_SSL_VERIFYPEER, 0L)); // Override default, don't verify the cert
144+
CHECK_CURL_OK(curl_easy_setopt(*curl, CURLOPT_SSL_VERIFYHOST,
145+
0L)); // Override default, don't verify that the cert matches the hostname
146146
}
147147

148148
// set read timeout
149-
curl_easy_setopt(*curl, CURLOPT_TIMEOUT, http_params.timeout);
149+
CHECK_CURL_OK(curl_easy_setopt(*curl, CURLOPT_TIMEOUT, http_params.timeout));
150150
// set connection timeout
151-
curl_easy_setopt(*curl, CURLOPT_CONNECTTIMEOUT, http_params.timeout);
151+
CHECK_CURL_OK(curl_easy_setopt(*curl, CURLOPT_CONNECTTIMEOUT, http_params.timeout));
152152
// accept content as-is (i.e no decompressing)
153-
curl_easy_setopt(*curl, CURLOPT_ACCEPT_ENCODING, "identity");
153+
CHECK_CURL_OK(curl_easy_setopt(*curl, CURLOPT_ACCEPT_ENCODING, "identity"));
154154
// follow redirects
155-
curl_easy_setopt(*curl, CURLOPT_FOLLOWLOCATION, 1L);
155+
CHECK_CURL_OK(curl_easy_setopt(*curl, CURLOPT_FOLLOWLOCATION, 1L));
156156

157157
// define the header callback
158-
curl_easy_setopt(*curl, CURLOPT_HEADERFUNCTION, RequestHeaderCallback);
159-
curl_easy_setopt(*curl, CURLOPT_HEADERDATA, &request_info->header_collection);
158+
CHECK_CURL_OK(curl_easy_setopt(*curl, CURLOPT_HEADERFUNCTION, RequestHeaderCallback));
159+
CHECK_CURL_OK(curl_easy_setopt(*curl, CURLOPT_HEADERDATA, &request_info->header_collection));
160160
// define the write data callback (for get requests)
161-
curl_easy_setopt(*curl, CURLOPT_WRITEFUNCTION, RequestWriteCallback);
162-
curl_easy_setopt(*curl, CURLOPT_WRITEDATA, &request_info->body);
161+
CHECK_CURL_OK(curl_easy_setopt(*curl, CURLOPT_WRITEFUNCTION, RequestWriteCallback));
162+
CHECK_CURL_OK(curl_easy_setopt(*curl, CURLOPT_WRITEDATA, &request_info->body));
163163

164164
if (!http_params.http_proxy.empty()) {
165-
curl_easy_setopt(*curl, CURLOPT_PROXY,
166-
StringUtil::Format("%s:%s", http_params.http_proxy, http_params.http_proxy_port).c_str());
165+
CHECK_CURL_OK(curl_easy_setopt(
166+
*curl, CURLOPT_PROXY,
167+
StringUtil::Format("%s:%s", http_params.http_proxy, http_params.http_proxy_port).c_str()));
167168

168169
if (!http_params.http_proxy_username.empty()) {
169-
curl_easy_setopt(*curl, CURLOPT_PROXYUSERNAME, http_params.http_proxy_username.c_str());
170-
curl_easy_setopt(*curl, CURLOPT_PROXYPASSWORD, http_params.http_proxy_password.c_str());
170+
CHECK_CURL_OK(curl_easy_setopt(*curl, CURLOPT_PROXYUSERNAME, http_params.http_proxy_username.c_str()));
171+
CHECK_CURL_OK(curl_easy_setopt(*curl, CURLOPT_PROXYPASSWORD, http_params.http_proxy_password.c_str()));
171172
}
172173
}
173174
}
@@ -191,9 +192,9 @@ class HTTPFSCurlClient : public HTTPClient {
191192
CURLcode res;
192193
{
193194
// If the same handle served a HEAD request, we must set NOBODY back to 0L to request content again
194-
curl_easy_setopt(*curl, CURLOPT_NOBODY, 0L);
195-
curl_easy_setopt(*curl, CURLOPT_URL, request_info->url.c_str());
196-
curl_easy_setopt(*curl, CURLOPT_HTTPHEADER, curl_headers ? curl_headers.headers : nullptr);
195+
CHECK_CURL_OK(curl_easy_setopt(*curl, CURLOPT_NOBODY, 0L));
196+
CHECK_CURL_OK(curl_easy_setopt(*curl, CURLOPT_URL, request_info->url.c_str()));
197+
CHECK_CURL_OK(curl_easy_setopt(*curl, CURLOPT_HTTPHEADER, curl_headers ? curl_headers.headers : nullptr));
197198
res = curl->Execute();
198199
}
199200

@@ -237,15 +238,15 @@ class HTTPFSCurlClient : public HTTPClient {
237238

238239
CURLcode res;
239240
{
240-
curl_easy_setopt(*curl, CURLOPT_URL, request_info->url.c_str());
241+
CHECK_CURL_OK(curl_easy_setopt(*curl, CURLOPT_URL, request_info->url.c_str()));
241242
// Perform PUT
242-
curl_easy_setopt(*curl, CURLOPT_CUSTOMREQUEST, "PUT");
243+
CHECK_CURL_OK(curl_easy_setopt(*curl, CURLOPT_CUSTOMREQUEST, "PUT"));
243244
// Include PUT body
244-
curl_easy_setopt(*curl, CURLOPT_POSTFIELDS, const_char_ptr_cast(info.buffer_in));
245-
curl_easy_setopt(*curl, CURLOPT_POSTFIELDSIZE, info.buffer_in_len);
245+
CHECK_CURL_OK(curl_easy_setopt(*curl, CURLOPT_POSTFIELDS, const_char_ptr_cast(info.buffer_in)));
246+
CHECK_CURL_OK(curl_easy_setopt(*curl, CURLOPT_POSTFIELDSIZE, info.buffer_in_len));
246247

247248
// Apply headers
248-
curl_easy_setopt(*curl, CURLOPT_HTTPHEADER, curl_headers ? curl_headers.headers : nullptr);
249+
CHECK_CURL_OK(curl_easy_setopt(*curl, CURLOPT_HTTPHEADER, curl_headers ? curl_headers.headers : nullptr));
249250

250251
res = curl->Execute();
251252
}
@@ -271,14 +272,14 @@ class HTTPFSCurlClient : public HTTPClient {
271272
CURLcode res;
272273
{
273274
// Set URL
274-
curl_easy_setopt(*curl, CURLOPT_URL, request_info->url.c_str());
275+
CHECK_CURL_OK(curl_easy_setopt(*curl, CURLOPT_URL, request_info->url.c_str()));
275276

276277
// Perform HEAD request instead of GET
277-
curl_easy_setopt(*curl, CURLOPT_NOBODY, 1L);
278-
curl_easy_setopt(*curl, CURLOPT_HTTPGET, 0L);
278+
CHECK_CURL_OK(curl_easy_setopt(*curl, CURLOPT_NOBODY, 1L));
279+
CHECK_CURL_OK(curl_easy_setopt(*curl, CURLOPT_HTTPGET, 0L));
279280

280281
// Add headers if any
281-
curl_easy_setopt(*curl, CURLOPT_HTTPHEADER, curl_headers ? curl_headers.headers : nullptr);
282+
CHECK_CURL_OK(curl_easy_setopt(*curl, CURLOPT_HTTPHEADER, curl_headers ? curl_headers.headers : nullptr));
282283

283284
// Execute HEAD request
284285
res = curl->Execute();
@@ -304,16 +305,16 @@ class HTTPFSCurlClient : public HTTPClient {
304305
CURLcode res;
305306
{
306307
// Set URL
307-
curl_easy_setopt(*curl, CURLOPT_URL, request_info->url.c_str());
308+
CHECK_CURL_OK(curl_easy_setopt(*curl, CURLOPT_URL, request_info->url.c_str()));
308309

309310
// Set DELETE request method
310-
curl_easy_setopt(*curl, CURLOPT_CUSTOMREQUEST, "DELETE");
311+
CHECK_CURL_OK(curl_easy_setopt(*curl, CURLOPT_CUSTOMREQUEST, "DELETE"));
311312

312313
// Follow redirects
313-
curl_easy_setopt(*curl, CURLOPT_FOLLOWLOCATION, 1L);
314+
CHECK_CURL_OK(curl_easy_setopt(*curl, CURLOPT_FOLLOWLOCATION, 1L));
314315

315316
// Add headers if any
316-
curl_easy_setopt(*curl, CURLOPT_HTTPHEADER, curl_headers ? curl_headers.headers : nullptr);
317+
CHECK_CURL_OK(curl_easy_setopt(*curl, CURLOPT_HTTPHEADER, curl_headers ? curl_headers.headers : nullptr));
317318

318319
// Execute DELETE request
319320
res = curl->Execute();
@@ -342,15 +343,15 @@ class HTTPFSCurlClient : public HTTPClient {
342343

343344
CURLcode res;
344345
{
345-
curl_easy_setopt(*curl, CURLOPT_URL, request_info->url.c_str());
346-
curl_easy_setopt(*curl, CURLOPT_POST, 1L);
346+
CHECK_CURL_OK(curl_easy_setopt(*curl, CURLOPT_URL, request_info->url.c_str()));
347+
CHECK_CURL_OK(curl_easy_setopt(*curl, CURLOPT_POST, 1L));
347348

348349
// Set POST body
349-
curl_easy_setopt(*curl, CURLOPT_POSTFIELDS, const_char_ptr_cast(info.buffer_in));
350-
curl_easy_setopt(*curl, CURLOPT_POSTFIELDSIZE, info.buffer_in_len);
350+
CHECK_CURL_OK(curl_easy_setopt(*curl, CURLOPT_POSTFIELDS, const_char_ptr_cast(info.buffer_in)));
351+
CHECK_CURL_OK(curl_easy_setopt(*curl, CURLOPT_POSTFIELDSIZE, info.buffer_in_len));
351352

352353
// Add headers if any
353-
curl_easy_setopt(*curl, CURLOPT_HTTPHEADER, curl_headers ? curl_headers.headers : nullptr);
354+
CHECK_CURL_OK(curl_easy_setopt(*curl, CURLOPT_HTTPHEADER, curl_headers ? curl_headers.headers : nullptr));
354355

355356
// Execute POST request
356357
res = curl->Execute();
@@ -430,13 +431,7 @@ class HTTPFSCurlClient : public HTTPClient {
430431
optional_ptr<HTTPState> state;
431432
unique_ptr<RequestInfo> request_info;
432433

433-
static std::mutex &GetRefLock() {
434-
static std::mutex mtx;
435-
return mtx;
436-
}
437-
438434
static void InitCurlGlobal() {
439-
GetRefLock();
440435
if (httpfs_client_count == 0) {
441436
curl_global_init(CURL_GLOBAL_DEFAULT);
442437
}
@@ -446,7 +441,6 @@ class HTTPFSCurlClient : public HTTPClient {
446441
static void DestroyCurlGlobal() {
447442
// TODO: when to call curl_global_cleanup()
448443
// calling it on client destruction causes SSL errors when verification is on (due to many requests).
449-
// GetRefLock();
450444
// if (httpfs_client_count == 0) {
451445
// throw InternalException("Destroying Httpfs client that did not initialize CURL");
452446
// }

‎src/s3fs.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -89,7 +89,7 @@ static HTTPHeaders create_s3_header(string url, string query, string host, strin
8989
if (use_sse_kms) {
9090
signed_headers += ";x-amz-server-side-encryption;x-amz-server-side-encryption-aws-kms-key-id";
9191
}
92-
auto canonical_request = method + "\n" + S3FileSystem::UrlEncode(url) + "\n" + query;
92+
auto canonical_request = method + "\n" + S3FileSystem::UrlEncode(url) + "\n" + query;
9393
if (content_type.length() > 0) {
9494
canonical_request += "\ncontent-type:" + content_type;
9595
}

0 commit comments

Comments
 (0)