GitLab

The 'checknames' field wasn't initialized in dns_view_create(), but it
should otherwise AddressSanitizer identifies the following runtime error
in query_test.c.

    runtime error: load of value 190, which is not a valid value for type '_Bool'

(cherry picked from commit 0c6fa164)

The lmdb.h header doesn't have to be included from the dns/lmdb.h
header as it can be separately included where used.  This stops
exposing the inclusion of lmdb.h from the libdns headers.

Diego Fronza authored Nov 28, 2020 and Matthijs Mekking committed Jan 29, 2021

Since it takes a couple lines of code to check whether stale answers
are enabled for a given view, code was extracted out to a proper
function.

(cherry picked from commit 74840ec5)

    WARNING: ThreadSanitizer: lock-order-inversion (potential deadlock)
    Cycle in lock order graph: M1 (0x000000000000) => M2 (0x000000000000) => M1

    Mutex M2 acquired here while holding mutex M1 in thread T1:
    #0 pthread_mutex_lock <null>
    #1 dns_view_findzonecut lib/dns/view.c:1310:2
    #2 fctx_create lib/dns/resolver.c:5070:13
    #3 dns_resolver_createfetch lib/dns/resolver.c:10813:12
    #4 dns_resolver_prime lib/dns/resolver.c:10442:12
    #5 dns_view_find lib/dns/view.c:1176:4
    #6 dbfind_name lib/dns/adb.c:3833:11
    #7 dns_adb_createfind lib/dns/adb.c:3155:12
    #8 findname lib/dns/resolver.c:3497:11
    #9 fctx_getaddresses lib/dns/resolver.c:3808:3
    #10 fctx_try lib/dns/resolver.c:4197:12
    #11 fctx_start lib/dns/resolver.c:4824:4
    #12 dispatch lib/isc/task.c:1152:7
    #13 run lib/isc/task.c:1344:2

    Mutex M1 previously acquired by the same thread here:
    #0 pthread_mutex_lock <null>
    #1 dns_resolver_createfetch lib/dns/resolver.c:10767:2
    #2 dns_resolver_prime lib/dns/resolver.c:10442:12
    #3 dns_view_find lib/dns/view.c:1176:4
    #4 dbfind_name lib/dns/adb.c:3833:11
    #5 dns_adb_createfind lib/dns/adb.c:3155:12
    #6 findname lib/dns/resolver.c:3497:11
    #7 fctx_getaddresses lib/dns/resolver.c:3808:3
    #8 fctx_try lib/dns/resolver.c:4197:12
    #9 fctx_start lib/dns/resolver.c:4824:4
    #10 dispatch lib/isc/task.c:1152:7
    #11 run lib/isc/task.c:1344:2

    Mutex M1 acquired here while holding mutex M2 in thread T1:
    #0 pthread_mutex_lock <null>
    #1 dns_resolver_shutdown lib/dns/resolver.c:10530:4
    #2 view_flushanddetach lib/dns/view.c:632:4
    #3 dns_view_detach lib/dns/view.c:689:2
    #4 qctx_destroy lib/ns/query.c:5152:2
    #5 fetch_callback lib/ns/query.c:5749:3
    #6 dispatch lib/isc/task.c:1152:7
    #7 run lib/isc/task.c:1344:2

    Mutex M2 previously acquired by the same thread here:
    #0 pthread_mutex_lock <null>
    #1 view_flushanddetach lib/dns/view.c:630:3
    #2 dns_view_detach lib/dns/view.c:689:2
    #3 qctx_destroy lib/ns/query.c:5152:2
    #4 fetch_callback lib/ns/query.c:5749:3
    #5 dispatch lib/isc/task.c:1152:7
    #6 run lib/isc/task.c:1344:2

    Thread T1 (running) created by main thread at:
    #0 pthread_create <null>
    #1 isc_thread_create lib/isc/pthreads/thread.c:73:8
    #2 isc_taskmgr_create lib/isc/task.c:1434:3
    #3 create_managers bin/named/main.c:915:11
    #4 setup bin/named/main.c:1223:11
    #5 main bin/named/main.c:1523:2

    SUMMARY: ThreadSanitizer: lock-order-inversion (potential deadlock) in pthread_mutex_lock

(cherry picked from commit a669c919)

Obtain references to view->redirect and view->managed_keys then
release view->lock so dns_zone_setviewcommit and dns_zone_setviewrevert
can obtain the view->lock while holding zone->lock.

WARNING: ThreadSanitizer: lock-order-inversion (potential deadlock) (pid=9132)
  Cycle in lock order graph: M987831431424375936 (0x000000000000) => M1012319771577875480 (0x000000000000) => M987831431424375936

  Mutex M1012319771577875480 acquired here while holding mutex M987831431424375936 in thread T2:
    #0 pthread_mutex_lock <null> (named+0x4642a6)
    #1 dns_zone_setviewcommit /builds/isc-projects/bind9/lib/dns/zone.c:1571:2 (libdns.so.1110+0x1d74eb)
    #2 dns_view_setviewcommit /builds/isc-projects/bind9/lib/dns/view.c:2388:3 (libdns.so.1110+0x1cfe29)
    #3 load_configuration /builds/isc-projects/bind9/bin/named/./server.c:8188:3 (named+0x51eadd)
    #4 loadconfig /builds/isc-projects/bind9/bin/named/./server.c:9438:11 (named+0x510c66)
    #5 ns_server_reconfigcommand /builds/isc-projects/bind9/bin/named/./server.c:9773:2 (named+0x510b41)
    #6 ns_control_docommand /builds/isc-projects/bind9/bin/named/control.c:243:12 (named+0x4e451a)
    #7 control_recvmessage /builds/isc-projects/bind9/bin/named/controlconf.c:465:13 (named+0x4e9056)
    #8 dispatch /builds/isc-projects/bind9/lib/isc/task.c:1157:7 (libisc.so.1107+0x507d5)
    #9 run /builds/isc-projects/bind9/lib/isc/task.c:1331:2 (libisc.so.1107+0x4d729)

  Mutex M987831431424375936 previously acquired by the same thread here:
    #0 pthread_mutex_lock <null> (named+0x4642a6)
    #1 dns_view_setviewcommit /builds/isc-projects/bind9/lib/dns/view.c:2382:2 (libdns.so.1110+0x1cfde7)
    #2 load_configuration /builds/isc-projects/bind9/bin/named/./server.c:8188:3 (named+0x51eadd)
    #3 loadconfig /builds/isc-projects/bind9/bin/named/./server.c:9438:11 (named+0x510c66)
    #4 ns_server_reconfigcommand /builds/isc-projects/bind9/bin/named/./server.c:9773:2 (named+0x510b41)
    #5 ns_control_docommand /builds/isc-projects/bind9/bin/named/control.c:243:12 (named+0x4e451a)
    #6 control_recvmessage /builds/isc-projects/bind9/bin/named/controlconf.c:465:13 (named+0x4e9056)
    #7 dispatch /builds/isc-projects/bind9/lib/isc/task.c:1157:7 (libisc.so.1107+0x507d5)
    #8 run /builds/isc-projects/bind9/lib/isc/task.c:1331:2 (libisc.so.1107+0x4d729)

  Mutex M987831431424375936 acquired here while holding mutex M1012319771577875480 in thread T7:
    #0 pthread_mutex_lock <null> (named+0x4642a6)
    #1 dns_view_findzonecut2 /builds/isc-projects/bind9/lib/dns/view.c:1300:2 (libdns.so.1110+0x1cc93a)
    #2 dns_view_findzonecut /builds/isc-projects/bind9/lib/dns/view.c:1261:9 (libdns.so.1110+0x1cc864)
    #3 fctx_create /builds/isc-projects/bind9/lib/dns/resolver.c:4459:13 (libdns.so.1110+0x1779d3)
    #4 dns_resolver_createfetch3 /builds/isc-projects/bind9/lib/dns/resolver.c:9628:12 (libdns.so.1110+0x176cb6)
    #5 dns_resolver_createfetch /builds/isc-projects/bind9/lib/dns/resolver.c:9504:10 (libdns.so.1110+0x174e17)
    #6 zone_refreshkeys /builds/isc-projects/bind9/lib/dns/zone.c:10061:12 (libdns.so.1110+0x2055a5)
    #7 zone_maintenance /builds/isc-projects/bind9/lib/dns/zone.c:10274:5 (libdns.so.1110+0x203a78)
    #8 zone_timer /builds/isc-projects/bind9/lib/dns/zone.c:13106:2 (libdns.so.1110+0x1e815a)
    #9 dispatch /builds/isc-projects/bind9/lib/isc/task.c:1157:7 (libisc.so.1107+0x507d5)
    #10 run /builds/isc-projects/bind9/lib/isc/task.c:1331:2 (libisc.so.1107+0x4d729)

  Mutex M1012319771577875480 previously acquired by the same thread here:
    #0 pthread_mutex_lock <null> (named+0x4642a6)
    #1 zone_refreshkeys /builds/isc-projects/bind9/lib/dns/zone.c:9951:2 (libdns.so.1110+0x204dc3)
    #2 zone_maintenance /builds/isc-projects/bind9/lib/dns/zone.c:10274:5 (libdns.so.1110+0x203a78)
    #3 zone_timer /builds/isc-projects/bind9/lib/dns/zone.c:13106:2 (libdns.so.1110+0x1e815a)
    #4 dispatch /builds/isc-projects/bind9/lib/isc/task.c:1157:7 (libisc.so.1107+0x507d5)
    #5 run /builds/isc-projects/bind9/lib/isc/task.c:1331:2 (libisc.so.1107+0x4d729)

  Thread T2 'isc-worker0001' (tid=9163, running) created by main thread at:
    #0 pthread_create <null> (named+0x446edb)
    #1 isc_thread_create /builds/isc-projects/bind9/lib/isc/pthreads/thread.c:60:8 (libisc.so.1107+0x726d8)
    #2 isc__taskmgr_create /builds/isc-projects/bind9/lib/isc/task.c:1468:7 (libisc.so.1107+0x4d635)
    #3 isc_taskmgr_create /builds/isc-projects/bind9/lib/isc/task.c:2109:11 (libisc.so.1107+0x4f587)
    #4 create_managers /builds/isc-projects/bind9/bin/named/./main.c:886:11 (named+0x4f1a97)
    #5 setup /builds/isc-projects/bind9/bin/named/./main.c:1305:11 (named+0x4f05ee)
    #6 main /builds/isc-projects/bind9/bin/named/./main.c:1556:2 (named+0x4ef12d)

  Thread T7 'isc-worker0006' (tid=9168, running) created by main thread at:
    #0 pthread_create <null> (named+0x446edb)
    #1 isc_thread_create /builds/isc-projects/bind9/lib/isc/pthreads/thread.c:60:8 (libisc.so.1107+0x726d8)
    #2 isc__taskmgr_create /builds/isc-projects/bind9/lib/isc/task.c:1468:7 (libisc.so.1107+0x4d635)
    #3 isc_taskmgr_create /builds/isc-projects/bind9/lib/isc/task.c:2109:11 (libisc.so.1107+0x4f587)
    #4 create_managers /builds/isc-projects/bind9/bin/named/./main.c:886:11 (named+0x4f1a97)
    #5 setup /builds/isc-projects/bind9/bin/named/./main.c:1305:11 (named+0x4f05ee)
    #6 main /builds/isc-projects/bind9/bin/named/./main.c:1556:2 (named+0x4ef12d)

SUMMARY: ThreadSanitizer: lock-order-inversion (potential deadlock) (/builds/isc-projects/bind9/bin/named/.libs/named+0x4642a6) in pthread_mutex_lock
(cherry picked from commit cdcfde9e)

Hold a weak reference to the view so that it can't go away while
nta is performing its lookups.  Cancel nta timers once all external
references to the view have gone to prevent them triggering new work.

(cherry picked from commit 0b2555e8)

Created isc_refcount_decrement_expect macro to test conditionally
the return value to ensure it is in expected range.  Converted
unchecked isc_refcount_decrement to use isc_refcount_decrement_expect.
Converted INSIST(isc_refcount_decrement()...) to isc_refcount_decrement_expect.

(cherry picked from commit bde5c763)

There were several problems with rbt hashtable implementation:

1. Our internal hashing function returns uint64_t value, but it was
   silently truncated to unsigned int in dns_name_hash() and
   dns_name_fullhash() functions.  As the SipHash 2-4 higher bits are
   more random, we need to use the upper half of the return value.

2. The hashtable implementation in rbt.c was using modulo to pick the
   slot number for the hash table.  This has several problems because
   modulo is: a) slow, b) oblivious to patterns in the input data.  This
   could lead to very uneven distribution of the hashed data in the
   hashtable.  Combined with the single-linked lists we use, it could
   really hog-down the lookup and removal of the nodes from the rbt
   tree[a].  The Fibonacci Hashing is much better fit for the hashtable
   function here.  For longer description, read "Fibonacci Hashing: The
   Optimization that the World Forgot"[b] or just look at the Linux
   kernel.  Also this will make Diego very happy :).

3. The hashtable would rehash every time the number of nodes in the rbt
   tree would exceed 3 * (hashtable size).  The overcommit will make the
   uneven distribution in the hashtable even worse, but the main problem
   lies in the rehashing - every time the database grows beyond the
   limit, each subsequent rehashing will be much slower.  The mitigation
   here is letting the rbt know how big the cache can grown and
   pre-allocate the hashtable to be big enough to actually never need to
   rehash.  This will consume more memory at the start, but since the
   size of the hashtable is capped to `1 << 32` (e.g. 4 mio entries), it
   will only consume maximum of 32GB of memory for hashtable in the
   worst case (and max-cache-size would need to be set to more than
   4TB).  Calling the dns_db_adjusthashsize() will also cap the maximum
   size of the hashtable to the pre-computed number of bits, so it won't
   try to consume more gigabytes of memory than available for the
   database.

   FIXME: What is the average size of the rbt node that gets hashed?  I
   chose the pagesize (4k) as initial value to precompute the size of
   the hashtable, but the value is based on feeling and not any real
   data.

For future work, there are more places where we use result of the hash
value modulo some small number and that would benefit from Fibonacci
Hashing to get better distribution.

Notes:
a. A doubly linked list should be used here to speedup the removal of
   the entries from the hashtable.
b. https://probablydance.com/2018/06/16/fibonacci-hashing-the-optimization-that-the-world-forgot-or-a-better-alternative-to-integer-modulo/

(cherry picked from commit e24bc324)

(cherry picked from commit 57e54c46)

Ondřej Surý authored Apr 28, 2020 and Ondřej Surý committed May 11, 2020

Originally, every library and binaries got linked to everything, which
creates unnecessary overlinking.  This wasn't as straightforward as it
should be as we still support configuration without libtool for 9.16.

Couple of smaller issues related to include headers and an issue where
sanitizer overload dlopen and dlclose symbols, so we were getting false
negatives in the autoconf test.

(cherry picked from commit ba0313e6)

Start enforcing the clang-format rules on changed files

Closes #46

See merge request !3063

(cherry picked from commit a04cdde4)

d2b5853b Start enforcing the clang-format rules on changed files
618947c6 Switch AlwaysBreakAfterReturnType from TopLevelDefinitions to All
654927c8 Add separate .clang-format files for headers
5777c44a Reformat using the new rules
60d29f69 Don't enforce copyrights on .clang-format

adjust clang-format options to get closer to ISC style

See merge request !3061

(cherry picked from commit d3b49b66)

0255a974 revise .clang-format and add a C formatting script in util
e851ed0b apply the modified style

Add curly braces using uncrustify and then reformat with clang-format back

Closes #46

See merge request !3057

(cherry picked from commit 67b68e06)

36c6105e Use coccinelle to add braces to nested single line statement
d14bb713 Add copy of run-clang-tidy that can fixup the filepaths
056e133c Use clang-tidy to add curly braces around one-line statements

Reformat source code with clang-format

Closes #46

See merge request !2156

(cherry picked from commit 7099e79a)

4c3b063e Import Linux kernel .clang-format with small modifications
f50b1e06 Use clang-format to reformat the source files
11341c76 Update the definition files for Windows
df6c1f76 Remove tkey_test (which is no-op anyway)

Also disable the semantic patch as the code needs tweaks here and there because
some destroy functions might not destroy the object and return early if the
object is still in use.

Function dns_view_findzonecut in view.c wasn't correctly handling
classes other than IN (chaos, hesiod, etc) whenever the name being
looked up wasn't in cache or in any of the configured zone views' database.

That resulted in a NULL fname being used in resolver.c:4900, which
in turn was triggering abort.

this function is used by dns_view_untrust() to handle revoked keys, so
it will still be needed after the keytable/validator refactoring is
complete, even though the keytable will be storing DS trust anchors
instead of keys. to simplify the way it's called, it now takes a DNSKEY
rdata struct instead of a DST key.

Ondřej Surý authored Sep 10, 2019 and Mark Andrews committed Oct 01, 2019

Use the semantic patch from the previous commit to replace all the calls to
dns_name_copy() with NULL as third argument with dns_name_copynf().

Ondřej Surý authored Sep 10, 2019 and Mark Andrews committed Oct 01, 2019

This second commit uses second semantic patch to replace the calls to
dns_name_copy() with NULL as third argument where the result was stored in a
isc_result_t variable.  As the dns_name_copy(..., NULL) cannot fail gracefully
when the third argument is NULL, it was just a bunch of dead code.

Couple of manual tweaks (removing dead labels and unused variables) were
manually applied on top of the semantic patch.

Ondřej Surý authored Sep 10, 2019 and Mark Andrews committed Oct 01, 2019

This commit add RUNTIME_CHECK() around all simple dns_name_copy() calls where
the third argument is NULL using the semantic patch from the previous commit.

Mark Andrews authored Aug 06, 2019 and Ondřej Surý committed Aug 07, 2019

so that cleanup can all be done in dns_view_weakattach().

(cherry picked from commit be8af3af)
(cherry picked from commit e3946327)

There's a deadlock in BIND 9 code where (dns_view_t){ .lock } and
(dns_resolver_t){ .buckets[i].lock } gets locked in different order.  When
view->weakrefs gets converted to a reference counting we can reduce the locking
in dns_view_weakdetach only to cases where it's the last instance of the
dns_view_t object.

(cherry picked from commit a7c9a52c)
(cherry picked from commit 232140ed)

Michał Kępień authored Apr 24, 2019 and Evan Hunt committed May 09, 2019

If named is configured to perform DNSSEC validation and also forwards
all queries ("forward only;") to validating resolvers, negative trust
anchors do not work properly because the CD bit is not set in queries
sent to the forwarders.  As a result, instead of retrieving bogus DNSSEC
material and making validation decisions based on its configuration,
named is only receiving SERVFAIL responses to queries for bogus data.
Fix by ensuring the CD bit is always set in queries sent to forwarders
if the query name is covered by an NTA.

- "hook" is now used only for hook points and hook actions
- the "hook" statement in named.conf is now "plugin"
- ns_module and ns_modlist are now ns_plugin and ns_plugins
- ns_module_load is renamed ns_plugin_register
- the mandatory functions in plugin modules (hook_register,
  hook_check, hook_version, hook_destroy) have been renamed

- use a per-view module list instead of global hook_modules
- create an 'instance' pointer when registering modules, store it in
  the module structure, and use it as action_data when calling
  hook functions - this enables multiple module instances to be set
  up in parallel
- also some nomenclature changes and cleanup

- make some cfg-parsing functions global so they can be run
  from filter-aaaa.so
- add filter-aaaa options to the hook module's parser
- mark filter-aaaa options in named.conf as obsolete, remove
  from named and checkconf, and update the filter-aaaa test not to
  use checkconf anymore
- remove filter-aaaa-related struct members from dns_view

- allow multiple "hook" statements at global or view level
- add "optional bracketed text" type for optional parameter list
- load hook module from specified path rather than hardcoded path
- add a hooktable pointer (and a callback for freeing it) to the
  view structure
- change the hooktable functions so they no longer update ns__hook_table
  by default, and modify PROCESS_HOOK so it uses the view hooktable, if
  set, rather than ns__hook_table. (ns__hook_table is retained for
  use by unit tests.)
- update the filter-aaaa system test to load filter-aaaa.so
- add a prereq script to check for dlopen support before running
  the filter-aaaa system test

not yet done:
- configuration parameters are not being passed to the filter-aaaa
  module; the filter-aaaa ACL and filter-aaaa-on-{v4,v6} settings are
  still stored in dns_view