A catalog zone is updated in an offloaded thread, which is not
stopped during a reconfiguration in an exclusive mode, and so
can cause a race condition with it.
Waiting for the offloaded threads to complete their work before
entering into the exclusive mode can potentially cause unwanted
delays, because offloaded threads are generally "allowed" to take
a longer amount of time before they complete.
Add a dns_catz_zone_prereconfig()/dns_catz_zone_postreconfig() pair
of functions which currently just lock the catalog zone when
reconfiguring it. The change should eliminate the race.
As a side note, there was already a similar pair of functions,
dns_catz_prereconfig() and dns_catz_postreconfig() which are called
before and after reconfiguring a 'dns_catz_zones_t' object.
Below are the stack traces of the reconfiguration thread which has
asserted, and a catalog zone update thread which was caught in the
middle of its work despite the fact that the exclusive mode is
turned on.
Stack trace of thread 23859:
#0 0x00007f80e7b8e52f raise (libc.so.6)
#1 0x00007f80e7b61e65 abort (libc.so.6)
#2 0x0000000000422558 assertion_failed (named)
#3 0x00007f80eaa6799e isc_assertion_failed (libisc-9.18.41.so)
#4 0x00007f80ea5bc788 dns_catz_entry_getname (libdns-9.18.41.so)
#5 0x000000000042ce0e catz_reconfigure (named)
#6 0x000000000042d3c5 configure_catz_zone (named)
#7 0x000000000042d7a4 configure_catz (named)
#8 0x0000000000430645 configure_view (named)
#9 0x000000000043d998 load_configuration (named)
#10 0x000000000044184f loadconfig (named)
#11 0x0000000000442525 named_server_reconfigcommand (named)
#12 0x000000000041b277 named_control_docommand (named)
#13 0x000000000041c74a control_command (named)
#14 0x00007f80eaa912ae task_run (libisc-9.18.41.so)
#15 0x00007f80eaa914cd isc_task_run (libisc-9.18.41.so)
#16 0x00007f80eaa46435 isc__nm_async_task (libisc-9.18.41.so)
#17 0x00007f80eaa467aa process_netievent (libisc-9.18.41.so)
#18 0x00007f80eaa475a6 process_queue (libisc-9.18.41.so)
#19 0x00007f80eaa46227 process_all_queues (libisc-9.18.41.so)
#20 0x00007f80eaa462a1 async_cb (libisc-9.18.41.so)
#21 0x00007f80e8d01893 uv__async_io.part.3 (libuv.so.1)
#22 0x00007f80e8d13ac4 uv__io_poll (libuv.so.1)
#23 0x00007f80e8d023fb uv_run (libuv.so.1)
#24 0x00007f80eaa45ced nm_thread (libisc-9.18.41.so)
#25 0x00007f80eaa9bda3 isc__trampoline_run (libisc-9.18.41.so)
#26 0x00007f80e7f1e1ca start_thread (libpthread.so.0)
#27 0x00007f80e7b798d3 __clone (libc.so.6)
...
...
Stack trace of thread 23912:
#0 0x00007f80ea5bc2da dns_catz_options_setdefault (libdns-9.18.41.so)
#1 0x00007f80ea5bd411 dns__catz_zones_merge (libdns-9.18.41.so)
#2 0x00007f80ea5c3c2f dns__catz_update_cb (libdns-9.18.41.so)
#3 0x00007f80eaa4fee9 isc__nm_work_run (libisc-9.18.41.so)
#4 0x00007f80eaa9bda3 isc__trampoline_run (libisc-9.18.41.so)
#5 0x00007f80eaa4ff48 isc__nm_work_cb (libisc-9.18.41.so)
#6 0x00007f80e8cfc75e worker (libuv.so.1)
#7 0x00007f80e7f1e1ca start_thread (libpthread.so.0)
#8 0x00007f80e7b798d3 __clone (libc.so.6)
Manually go through the code using dns_rdataset_isassociated() and
use dns_rdataset_cleanup() where appropriate in places that a simple
semantic patch is not able to find automatically.
We had a common pattern in the code that looks like this:
if (dns_rdataset_isassociated(rdataset)) {
dns_rdataset_disassociate(rdataset);
}
add a helper macro that checks for rdataset != NULL and the above
called dns_rdataset_cleanup(rdataset).
Extended DNS Error 9 (Missing DNSKEY) is now sent when a validating resolver
attempts to validate a response but can't get the DNSKEY from the authoritative
server of the zone, while the DS record is present in the parent zone.
Note the EDE 9 is send as part of the proveunsecure flow, after the
validator successfully fetched the DS of the zone from the parent. So if
the DS is also missing, the EDE 9 won't be sent.
The makeslab function in rdataslab.c contains an optimization for cases
where the source is already an rdataslab. In these cases, it copies the
entire slab using memmove. However, this creates a race condition: while
the target slab is protected by a node lock, the source slab is not
protected. This becomes problematic because the TTL heap needs to
modify the heap index stored in the slab header, potentially while the
memmove operation is reading from it.
A closer look at makeslab shows that copying the header part of the slab
is unnecessary, the header can be default-initialized instead. This MR
modifies makeslab to copy only the raw part of the slab, while
default-initializing the header, eliminating the race condition. For
consistency, it also applies the same change to vecheader/makevec.
The bitset packing of the resign_lsb and heap_index in struct vecheader
was causing a race condition, since both bindrdataset and heap
operations tried to access the same byte (even though they are accessing
different fields).
While heap operations are protected by the node lock of the header being
inserted, they aren't protected by the node locks of the headers being
displaced, leading to the race condition.
This commit fixes the issue by reverting the struct packing
optimization.
This is a new seek function for dbiterator that is meant to find an
NSEC3 node in a zone database. The difference with dns_dbiterator_seek
is that if the node does not exist, this seek function will point the
iterator to the next NSEC3 name.
Add a FOREACH macro modelled around the DNS_RDATASET_FOREACH one, that
uses vecheader directly. Useful when you want to manipulate a vecheader
without binding it to avoid having to take the node lock inside qpzone.c.
Add an implementation of rdataset specialized for authoritative
workloads. For now, it is a copy of rdataslab, with redundant fields
from the header removed.
The removal of the foundname and name parameters from various qp.c
functions led to formatting issues. Restore the correct formatting via
clang-format.
Outside of unit tests, the name parameter in dns_qpiter_<...> and
dns_qpchain_<...> is only used in context where the name can be
extracted directly from the underlying node.
This commits modifies the signatures of dns_qpiter_<...> and
dns_qpchain_<...> not to have a name parameter. Where the name parameter
was needed, we now query the node and copy the name directly from it.
This allows us to remove maybe_set_name from qp.c. Besides simplifying
the API, this leads to a performance speedup for NXDOMAIN handling,
as we avoid calling maybe_set_name inside step, and maybe_set_name is
very inefficient.
A copy of the implementation maybe_set_name is retained for the unit
tests.
The `foundname` parameter in dns_qp_lookup is used only in the unit
tests. This commit simplifies the API by removing it, and modifying the
unit tests to extract the name from pval.
When dns_name_totext() is called with a dynamically allocated
target buffer which is too small for the name, it will resize
the buffer instead of returning ISC_R_NOSPACE.
It is possible to have a fetch that is active, but it has been cloned,
so it won't be used when found in the hash table. The fetch options
also prevent matching in the hash table, so add a hexadecimal dump of
the fctx->options to the output.
This commit implements a batch update function for qpzone. The main
reason for this is speed: using addrdataset would cause a qp transaction
per rrdataset added, leading to a substantial slowdown compared to
RBTDB. The new API results in a qp transaction per applied diff.
This commit adds a layer of indirection to the apply_diff logic used by
IXFR and resigning by having the database updates go through a vtable.
We do this in three steps:
- We extend dns_rdatacallbacks_t vtable to allow subtraction and
resigning.
- We add a new set of api (begin|commit|abort)update to the dbmethods
vtable, that model an incremental update that can be aborted.
- We extract the core logic of diff_apply into a function that
satisfies the new interface.
- We make diff_apply use this new function, and log the results.
The intent of this commit is to allow databases to expose a batch
incremental update implementation, just like they expose a custom
batch creation implementation through (begin|end)load.
Make the API tighter. The idea of this commit is to highlight the
distinction between a database transaction and a journal transaction,
and ensure we run dns_zone_verifydb on error.
Done to simplify a later refactor.
The dns_catz_generate_zonecfg() function generates a zone configuration
string to use with a new catalog zone member zone. The buffer for the
string is 512 bytes initially (ISC_BUFFER_INCR), but can be reallocated
when required, when using corresponding isc_buffer functions like
isc_buffer_reserve(), isc_buffer_putstr(), isc_buffer_copyregion(), etc.
However, the dns_name_totext() function, which expects the buffer as an
argument, doesn't automatically resize it if the name doesn't fit there,
but instead just returns ISC_R_NOSPACE.
The chance of this occurring increases when the configuration string is
large due to, for example, long zone name, long list of primary servers
which have keys configured and/or TLS configured.
Use dns_name_format() accompanied with isc_buffer_putstr() instead of
dns_name_totext().
Instead of just flagging the qpcache node to be dirty, add the headers
to be cleaned to the dirty list and when cleaning the node, only walk
through the dirty node, not all the slabtops.
dns_qpiter_{prev,next} requires the current iterator node to still be
valid which might not always the case after dereference_iter_node was
called. Currently, this is ensured via closeversion() mechanism, but it
is not guaranteed to be true in the future.
Move the call to dereference_iter_node to after the dns_qpiter_prev()
and dns_qpiter_next() to prevent a possible use-after-free of the
current iterator node.
If the keymgr state machine is in an invalid state, it tries to move
it self to a valid state. But when you do key rollovers during an
invalid state, and the next state is also an invalid state, the keymgr
will happily do the transition.
It would be good to not do key rollovers if there is not a KSK and ZSK
fully omnipresent. But also it would be good to safeguard against
unexpected transitions.
This commit does that by not moving things to unretentive (which is
the state where we would remove the corresponding record from the zone)
if the state machine is currently in an invalid state.
Document the way `__attribute__((__constructor__))` and
`__attribute__((__destructor__))` must be used in BIND9 libraries in
order to avoid unexpected behaviors with other third-party libraries.
CLEANUP is a macro similar to CHECK but unconditional, jumping
to cleanup even if the result is ISC_R_SUCCESS. It is now used
in place of DST_RET, CLEANUP_WITH, and CHECK(<non-success constant>).
previously, there were over 40 separate definitions of CHECK macros, of
which most used "goto cleanup", and the rest "goto failure" or "goto
out". there were another 10 definitions of RETERR, of which most were
identical to CHECK, but some simply returned a result code instead of
jumping to a cleanup label.
this has now been standardized throughout the code base: RETERR is for
returning an error code in the case of an error, and CHECK is for jumping
to a cleanup tag, which is now always called "cleanup". both macros are
defined in isc/util.h.
In commit aea251f3bc, `isc_buffer_reserve()` was changed to
take a simple `isc_buffer_t *` instead of `isc_buffer_t **`.
A number of functions calling it have now been similarly
modified.
Wrap 'dns_keymgr_status()' in 'dns_zone_dnssecstatus()' so we can easily
retrieve the zone string name and refresh key time value.
In addition to the current time, output when the next key event is
expected.
Don't log keys that are completely hidden unless verbose is set.
Don't log key state values unless verbose is set, or they are in a
weird state.
For expected key states, log a more useful message of the stage of
the rollover. If we are in the middle of a key rollover, don't log
when the next key rollover is scheduled.
Condense the output for better readability.
During refactoring, a condition that prevented caching RRSIGs for
records that we already have cached NODATA records was changed in an
invalid way. This was caught later when a cached NODATA(type) +
RRSIG(type) was found in the cache and caused an assertion failure.
Fix and simplify condition that prevents adding such RRSIGs.
Formerly, we've evicted the RRSIG(type) only when we were changing
existing header from positive to negative. Move the eviction routine
for the RRSIG to a common path, so the RRSIG also gets evicted when we
are adding new negative header for a specific type.
When named is being reconfigured, it detaches from the old
'isc_tlsctx_cache_t' TLS context cache object and creates a
new one. This can cause an assertion failure within the
resolver when the object is destroyed while still in use,
because the resolver is using the object without getting
attached to it.
Add an attach/detach so that the 'isc_tlsctx_cache_t' doesn't
get destroyed while still being in use.
Maintain the relationship between the parent and child fetch and when
creating a new child fetch, properly check the resolution loops that
would lead to a new fetch would join one of the parent's fetch contexts.
In !9155, the QNAME minimization was changed to not leak the query type
to the parent name server. This violates RFC 9156 Section 3, step (3)
and it is not necessary. It also breaks some (weird) authoritative DNS
setups, especially when CNAMEs are involved. Also there is really no
privacy leak with query type.
As the fetch context reference counting was converted to userspace RCU
reference counting, the ability to debug the reference counting was
lost. Restore the debugging by adding the optional compile-time enabled
debugging output again.
The .delegating flag was only set, but never used in the dns_qpcache.
Remove it completely together with the code that was locking the node
to set the flag if the added type was DNAME.
When synchronizing the secure database, we skip DNSSEC records that
BIND 9 maintains with inline-signing. We should also skip private
RDATA type records that are used to track the current state of a
zone-signing process.
now that the EDNS state is stored within dns_message_t, it's no longer
necessary to have a public API call to build an opt rdataset; we can
just have dns_message_setopt() build the opt record internally.
