Avoid stack overflow when many lookup redirection happens#25
Open
BewareMyPower wants to merge 2 commits intomainfrom
Open
Avoid stack overflow when many lookup redirection happens#25BewareMyPower wants to merge 2 commits intomainfrom
BewareMyPower wants to merge 2 commits intomainfrom
Conversation
### 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 file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TODO: dig into the reason