You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@pulsar.apache.org by "BewareMyPower (via GitHub)" <gi...@apache.org> on 2023/11/23 09:16:53 UTC
[PR] Fix HTTP lookup segfault when the redirect host cannot be resolved [pulsar-client-cpp]
BewareMyPower opened a new pull request, #356:
URL: https://github.com/apache/pulsar-client-cpp/pull/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.
### 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix HTTP lookup segfault when the redirect host cannot be resolved [pulsar-client-cpp]
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower merged PR #356:
URL: https://github.com/apache/pulsar-client-cpp/pull/356
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Fix HTTP lookup segfault when the redirect host cannot be resolved [pulsar-client-cpp]
Posted by "BewareMyPower (via GitHub)" <gi...@apache.org>.
BewareMyPower commented on code in PR #356:
URL: https://github.com/apache/pulsar-client-cpp/pull/356#discussion_r1403114023
##########
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_) {
Review Comment:
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
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@pulsar.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org