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.
apache · Nov 24, 2023 · d209482 · d209482
1 parent 0bbc155
commit d209482
Show file tree

Hide file tree

Showing 2 changed files with 68 additions and 86 deletions.
diff --git 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
@@ -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<CurlWrapper::TlsContext> 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<CurlWrapper::TlsContext> 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) {