Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix HTTP lookup segfault when the redirect host cannot be resolved #356

Merged
merged 1 commit into from
Nov 24, 2023
Merged

Fix HTTP lookup segfault when the redirect host cannot be resolved #356

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion lib/CurlWrapper.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
149 changes: 64 additions & 85 deletions lib/HTTPLookupService.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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_) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here are more explanations for removing this loop.

This trivial loop is added just to print the redirect URL each time. For example, if broker-1 redirects to broker-2 and broker-2 redirects to broker-3, there would be three logs before:

Curl [1] Lookup Request sent for http://broker-1...
Curl [2] Lookup Request sent for http://broker-2...
Curl [3] Lookup Request sent for http://broker-3...

After this patch, users won't know which brokers have been redirected to, the logs could be:

Curl Lookup Request sent for http://broker-1

// 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) {
Expand Down