Skip to content

Refactor the dns_resolver fetch context hash tables and locking

Ondřej Surý requested to merge ondrej-fix-dns_resolver-hash-tables into main

This is second in the series of fixing the usage of hashtables in the dns_adb and the dns_resolver units.

Currently, the fetch buckets (used to hold the fetch context) and zone buckets (used to hold per-domain counters) would never get cleaned from the memory. Combined with the fact that the hashtable now grows as needed (instead of using hashtable as buckets), the memory usage in the resolver can just grow and it never drops down.

In this commit, the usage of hashtables (hashmaps) has been completely rewritten, so there are no "buckets" and all the matching conditions are directly mapped into the hashtable key:

  1. For per-domain counter hashtable, this is simple as the lowercase domain name is used directly as a counter.

  2. For fetch context hashtable, this requires copying some extra flags back and forth in the key.

As we don't hold the "buckets" forever, the cleaning mechanism has been rewritten as well:

  1. For per-domain counter hashtable, this is again much simpler, as we only need to check whether the usage counter is still zero under the lock and bail-out on cleaning if the counter is in use.

  2. For fetch context hashtable, this is more complicated as the fetch context cannot be reused after it has been finished. The algorithm is different, the fetch context is always removed from the hashtable, but if we find the fetch context that has been marked as finished in the lookup function, we help with the cleaning from the hashtable and try again.

Couple of additional changes have been implemented in this refactoring as those were needed for correct functionality and could be split into individual commits (or would not make sense as seperate commits):

  1. The dns_resolver_createfetch() has an option to create "unshared" fetch. The "unshared" fetch will never get matched, so there's little point in storing the "unshared" fetch in the hashtable. Therefore the "unshared" fetches are now detached from the hashtable and live just on their own.

  2. Replace the custom reference counting with ISC_REFCOUNT_DECL/IMPL macros for better tracing.

  3. fctx_done_detach() is idempotent, it makes the "final" detach (the one matching the create function) only once. But that also means that it has to be called before the detach that kept the fetch context alive in the callback. A new macro fctx_done_unref() has been added to allow this code flow:

    fctx_done_unref(fctx, result); fctx_detach(&fctx);

    Doing this the other way around could cause fctx to get destroyed in the fctx_unref() first and fctx_done_detach() would cause UAF.

  4. The resume_qmin() and resume_dslookup() callbacks have been refactored for more readability and simpler code paths. The validated() callback has also received some of the simplifications, but it should be refactored in the future as it is bit of spaghetti now.

Edited by Ondřej Surý

Merge request reports