From d2094828f9fe756b315ba194e1f8a69ca24ac6b4 Mon Sep 17 00:00:00 2001 From: Yunze Xu Date: Fri, 24 Nov 2023 12:35:01 +0800 Subject: [PATCH] Fix HTTP lookup segfault when the redirect host cannot be resolved (#356) ### Motivation When the host of the redirect URL cannot be resolved, segmentation fault will happen: https://github.com/apache/pulsar-client-cpp/blob/0bbc15502905d19c630d237b5e102bfb996bb098/lib/CurlWrapper.h#L173-L175 In this case, `curl` will be `nullptr`. Assigning a nullptr to a `std::string` is an undefined behavior that might cause segfault. ### Modifications Check if `url` is nullptr in `CurlWrapper::get` and before assigning it to the `redirectUrl` field. Improve the `HTTPLookupService::sendHTTPRequest` method by configuring the `maxLookupRedirects` instead of a loop and print more detailed error messages. --- lib/CurlWrapper.h | 5 +- lib/HTTPLookupService.cc | 149 +++++++++++++++++---------------------- 2 files changed, 68 insertions(+), 86 deletions(-) diff --git a/lib/CurlWrapper.h b/lib/CurlWrapper.h index 89b7919d..749a79c3 100644 --- a/lib/CurlWrapper.h +++ b/lib/CurlWrapper.h @@ -172,7 +172,10 @@ inline CurlWrapper::Result CurlWrapper::get(const std::string& url, const std::s if (responseCode == 307 || responseCode == 302 || responseCode == 301) { char* url; curl_easy_getinfo(handle_, CURLINFO_REDIRECT_URL, &url); - result.redirectUrl = url; + // `url` is null when the host of the redirect URL cannot be resolved + if (url) { + result.redirectUrl = url; + } } return result; } diff --git a/lib/HTTPLookupService.cc b/lib/HTTPLookupService.cc index 4ec72c1e..0959af2a 100644 --- a/lib/HTTPLookupService.cc +++ b/lib/HTTPLookupService.cc @@ -191,98 +191,77 @@ Result HTTPLookupService::sendHTTPRequest(std::string completeUrl, std::string & Result HTTPLookupService::sendHTTPRequest(std::string completeUrl, std::string &responseData, long &responseCode) { - uint16_t reqCount = 0; - Result retResult = ResultOk; - while (++reqCount <= maxLookupRedirects_) { - // Authorization data - AuthenticationDataPtr authDataContent; - Result authResult = authenticationPtr_->getAuthData(authDataContent); - if (authResult != ResultOk) { - LOG_ERROR("Failed to getAuthData: " << authResult); - return authResult; - } + // Authorization data + AuthenticationDataPtr authDataContent; + Result authResult = authenticationPtr_->getAuthData(authDataContent); + if (authResult != ResultOk) { + LOG_ERROR("Failed to getAuthData: " << authResult); + return authResult; + } - CurlWrapper curl; - if (!curl.init()) { - LOG_ERROR("Unable to curl_easy_init for url " << completeUrl); - return ResultLookupError; - } + CurlWrapper curl; + if (!curl.init()) { + LOG_ERROR("Unable to curl_easy_init for url " << completeUrl); + return ResultLookupError; + } - std::unique_ptr tlsContext; - if (isUseTls_) { - tlsContext.reset(new CurlWrapper::TlsContext); - tlsContext->trustCertsFilePath = tlsTrustCertsFilePath_; - tlsContext->validateHostname = tlsValidateHostname_; - tlsContext->allowInsecure = tlsAllowInsecure_; - if (authDataContent->hasDataForTls()) { - tlsContext->certPath = authDataContent->getTlsCertificates(); - tlsContext->keyPath = authDataContent->getTlsPrivateKey(); - } else { - tlsContext->certPath = tlsCertificateFilePath_; - tlsContext->keyPath = tlsPrivateFilePath_; - } + std::unique_ptr tlsContext; + if (isUseTls_) { + tlsContext.reset(new CurlWrapper::TlsContext); + tlsContext->trustCertsFilePath = tlsTrustCertsFilePath_; + tlsContext->validateHostname = tlsValidateHostname_; + tlsContext->allowInsecure = tlsAllowInsecure_; + if (authDataContent->hasDataForTls()) { + tlsContext->certPath = authDataContent->getTlsCertificates(); + tlsContext->keyPath = authDataContent->getTlsPrivateKey(); + } else { + tlsContext->certPath = tlsCertificateFilePath_; + tlsContext->keyPath = tlsPrivateFilePath_; } + } - LOG_INFO("Curl [" << reqCount << "] Lookup Request sent for " << completeUrl); - CurlWrapper::Options options; - options.timeoutInSeconds = lookupTimeoutInSeconds_; - options.userAgent = std::string("Pulsar-CPP-v") + PULSAR_VERSION_STR; - options.maxLookupRedirects = 1; // redirection is implemented by the outer loop - auto result = curl.get(completeUrl, authDataContent->getHttpHeaders(), options, tlsContext.get()); - const auto &error = result.error; - if (!error.empty()) { - LOG_ERROR(completeUrl << " failed: " << error); - return ResultConnectError; - } + LOG_INFO("Curl Lookup Request sent for " << completeUrl); + CurlWrapper::Options options; + options.timeoutInSeconds = lookupTimeoutInSeconds_; + options.userAgent = std::string("Pulsar-CPP-v") + PULSAR_VERSION_STR; + options.maxLookupRedirects = maxLookupRedirects_; + auto result = curl.get(completeUrl, authDataContent->getHttpHeaders(), options, tlsContext.get()); + const auto &error = result.error; + if (!error.empty()) { + LOG_ERROR(completeUrl << " failed: " << error); + return ResultConnectError; + } - responseData = result.responseData; - responseCode = result.responseCode; - auto res = result.code; - LOG_INFO("Response received for url " << completeUrl << " responseCode " << responseCode - << " curl res " << res); - - const auto &redirectUrl = result.redirectUrl; - switch (res) { - case CURLE_OK: - if (responseCode == 200) { - retResult = ResultOk; - } else if (!redirectUrl.empty()) { - LOG_INFO("Response from url " << completeUrl << " to new url " << redirectUrl); - completeUrl = redirectUrl; - retResult = ResultLookupError; - } else { - retResult = ResultLookupError; - } - break; - case CURLE_COULDNT_CONNECT: - LOG_ERROR("Response failed for url " << completeUrl << ". Error Code " << res); - retResult = ResultRetryable; - break; - case CURLE_COULDNT_RESOLVE_PROXY: - case CURLE_COULDNT_RESOLVE_HOST: - case CURLE_HTTP_RETURNED_ERROR: - LOG_ERROR("Response failed for url " << completeUrl << ". Error Code " << res); - retResult = ResultConnectError; - break; - case CURLE_READ_ERROR: - LOG_ERROR("Response failed for url " << completeUrl << ". Error Code " << res); - retResult = ResultReadError; - break; - case CURLE_OPERATION_TIMEDOUT: - LOG_ERROR("Response failed for url " << completeUrl << ". Error Code " << res); - retResult = ResultTimeout; - break; - default: - LOG_ERROR("Response failed for url " << completeUrl << ". Error Code " << res); - retResult = ResultLookupError; - break; - } - if (redirectUrl.empty()) { - break; - } + responseData = result.responseData; + responseCode = result.responseCode; + auto res = result.code; + if (res == CURLE_OK) { + LOG_INFO("Response received for url " << completeUrl << " responseCode " << responseCode); + } else if (res == CURLE_TOO_MANY_REDIRECTS) { + LOG_ERROR("Response received for url " << completeUrl << ": " << curl_easy_strerror(res) + << ", curl error: " << result.serverError + << ", redirect URL: " << result.redirectUrl); + } else { + LOG_ERROR("Response failed for url " << completeUrl << ": " << curl_easy_strerror(res) + << ", curl error: " << result.serverError); } - return retResult; + switch (res) { + case CURLE_OK: + return ResultOk; + case CURLE_COULDNT_CONNECT: + return ResultRetryable; + case CURLE_COULDNT_RESOLVE_PROXY: + case CURLE_COULDNT_RESOLVE_HOST: + case CURLE_HTTP_RETURNED_ERROR: + return ResultConnectError; + case CURLE_READ_ERROR: + return ResultReadError; + case CURLE_OPERATION_TIMEDOUT: + return ResultTimeout; + default: + return ResultLookupError; + } } LookupDataResultPtr HTTPLookupService::parsePartitionData(const std::string &json) {