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.

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

Conversation

BewareMyPower
Copy link
Contributor

Motivation

When the host of the redirect URL cannot be resolved, segmentation fault will happen:

char* url;
curl_easy_getinfo(handle_, CURLINFO_REDIRECT_URL, &url);
result.redirectUrl = url;

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.

Verifications

It's hard to mock a redirect case so I have to modify the TopicLookupBase#internalLookupTopicAsync in broker side and return a 307 response.

When I return a http://unknown:8080 as the redirect URL:

2023-11-23 17:06:12.821 ERROR [0x16d31b000] HTTPLookupService:246 | Response failed for url http://localhost:8080/lookup/v2/topic/persistent/public/default/my-topic: Couldn't resolve host name, curl error: Could not resolve host: unknown

Before this patch, it will crash.

When I just return a http://localhost:8080 as the redirect URL:

2023-11-23 17:15:06.267 ERROR [0x16d657000] HTTPLookupService:243 | Response received for url http://localhost:8080/lookup/v2/topic/persistent/public/default/my-topic: Number of redirects hit maximum amount, curl error: Maximum (20) redirects followed, redirect URL: http://localhost:8080/lookup/v2/topic/persistent/public/default/my-topic?authoritative=false

Before this patch, it will print similar logs 20 times.

### 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.

### Verifications

It's hard to mock a redirect case so I have to modify the
`TopicLookupBase#internalLookupTopicAsync` in broker side and return a
307 response.

When I return a `http://unknown:8080` as the redirect URL:

```
2023-11-23 17:06:12.821 ERROR [0x16d31b000] HTTPLookupService:246 | Response failed for url http://localhost:8080/lookup/v2/topic/persistent/public/default/my-topic: Couldn't resolve host name, curl error: Could not resolve host: unknown
```

Before this patch, it will crash.

When I just return a `http://localhost:8080` as the redirect URL:

```
2023-11-23 17:15:06.267 ERROR [0x16d657000] HTTPLookupService:243 | Response received for url http://localhost:8080/lookup/v2/topic/persistent/public/default/my-topic: Number of redirects hit maximum amount, curl error: Maximum (20) redirects followed, redirect URL: http://localhost:8080/lookup/v2/topic/persistent/public/default/my-topic?authoritative=false
```

Before this patch, it will print similar logs 20 times.
@BewareMyPower BewareMyPower added the bug Something isn't working label Nov 23, 2023
@BewareMyPower BewareMyPower added this to the 3.5.0 milestone Nov 23, 2023
@BewareMyPower BewareMyPower self-assigned this Nov 23, 2023
@@ -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

@BewareMyPower BewareMyPower merged commit d209482 into apache:main Nov 24, 2023
@BewareMyPower BewareMyPower deleted the bewaremypower/url-crash branch November 24, 2023 04:35
BewareMyPower added a commit that referenced this pull request Dec 6, 2023
)

### 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.

(cherry picked from commit d209482)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working release/3.4.2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants