Changelog category (leave one):

• Bug Fix (user-visible misbehavior in official stable or prestable release)

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

A couple of seg faults have been reported around c-ares. All of the recent stack traces observed fail on inserting into std::unodered_set<>. I believe I have found the root cause of this, it seems to be unprocessed queries. Prior to this PR, CH calls poll to wait on the file descriptors in the c-ares channel. According to the poll docs, a negative return value means an error has ocurred. Because of this, we would abort the execution and return failure. The problem is that poll will also return a negative value if a system interrupt occurs. A system interrupt does not mean the processing has failed or ended, but we would abort it anyways because we were checking for negative values. Once the execution is aborted, the whole stack is destroyed, which includes the std::unordered_set<std::string> passed to the void * parameter of the c-ares callback. Once c-ares completed the request, the callback would be invoked and would access an invalid memory address causing a segfault.

Solution was to check for EINTR == errno (errno is both thread-safe and thread-local, so I think it's ok to check it.) and retry if that was the case. If it was an actual error, call ares_cancel to cancel pending queries and then abort execution (similar to what libcurl does). Calling ares_cancel will make sure pending requests are canceled and the callback will not be executed with dangling references.

libcurl seems to have at least two wait_resolve/ failure recovery methods depending on build options, both of these make use of ares_cancel. See ref1 and ref2.

reverseDNSQuery function was added as a tool to debug this problem. The test implemented with it is able to reproduce the issue, so I think it's a good idea to keep it. Plus, it can also be used for reverse DNS querying lol.

The funny thing is that it was pretty easy to reproduce this issue with this function, but neither my multithreaded unit tests or scripts to make an "authentication storm" was able to crash CH. I believe unit tests were not able to repro the crash because it was testing only those classes and there was no system interrupts/ bigger failures to cause the "premature" abortion. Authentication storm, on the other hand, had system interrupts and CH was running normally, but it would test against a local DNS instance running in my machine, so the latency was very low. Maybe it was too low for an interrupt to happen?

To be on the super safe side, there are two more actions that we could take:

1. Unconditionally cancel c-ares requests after processing.
2. Do not use stack variables as arguments to the callback. Instead, implement some sort of pending request list that we can add and remove based on an ID or something.

I want to believe we have found the issue and there is no need for the above, but let me know what you guys think.

Documentation entry for user-facing changes

Information about CI checks: https://clickhouse.com/docs/en/development/continuous-integration/