> Put a space before opening parentheses only after control statement
> keywords (for/if/while...) except this option doesn’t apply to ForEach
> and If macros. This is useful in projects where ForEach/If macros are
> treated as function calls instead of control statements.
The slabheader.c, qpzone.c and qpcache.c had couple of shared macros
that were copied and paste between the units. Move these common
attributes access macros into private header, so these can be shared
among the three compilation units.
The RR type 0 is a reserved type for SIG[1] resource record. It should
not be ever inserted into the database nor queried. Add a special
handling to bail out quickly with DNS_R_DISALLOWED when inserting and
ISC_R_NOTFOUND when looking up TYPE0. This is also prerequisite for
stricter checks in the follow-up commit.
1. https://www.rfc-editor.org/rfc/rfc2535#section-4.1.8.1
All databases in the codebase follow the same structure: a database is
an associative container from DNS names to nodes, and each node is an
associative container from RR types to RR data.
Each database implementation (qpzone, qpcache, sdlz, builtin, dyndb) has
its own corresponding node type (qpznode, qpcnode, etc). However, some
code needs to work with nodes generically regardless of their specific
type - for example, to acquire locks, manage references, or
register/unregister slabs from the heap.
Currently, these generic node operations are implemented as methods in
the database vtable, which creates problematic coupling between database
and node lifetimes. If a node outlives its parent database, the node
destructor will destroy all RR data, and each RR data destructor will
try to unregister from heaps by calling a virtual function from the
database vtable. Since the database was already freed, this causes a
crash.
This commit breaks the coupling by standardizing the layout of all
database nodes, adding a dedicated vtable for node operations, and
moving node-specific methods from the database vtable to the node
vtable.
dns_keytable_issecuredomain() and dns_view_issecuredomain()
previously returned a result code to inform the caller of
unexpected database failures when looking up names in the
keytable and/or NTA table. such failures are not actually
possible. both functions now return a simple bool.
also, dns_view_issecuredomain() now returns false if
view->enablevalidation is false, so the caller no longer
has to check for that.
When running the isc_quota unit test with less than usual amount of
RAM (e.g. in a CI for architectures with 32 bits of address space),
the pthread_create() function fails with the "Resource temporarily
unavailable (11):" error code.
Add functions to get and set the thread stack size (if requested),
and use these to set the thread stack size to smaller value in the
isc_quota unit test.
This required couple of internal changes to the isc_mem_debugging.
The isc_mem_debugging is now internal to isc_mem unit and there are
three new functions:
1. isc_mem_setdebugging() can change the debugging setting for an
individual memory context. This is need for the memory contexts used
for OpenSSL, libxml and libuv accounting as recording and tracing
memory is broken there.
2. isc_mem_debugon() / isc_mem_debugoff() can be used to change default
memory debugging flags as well as debugging flags for isc_g_mctx.
Additionally, the memory debugging is inconsistent across the code-base.
For now, we are keeping the existing flags, but three new environment
variables have been added 'ISC_MEM_DEBUGRECORD', 'ISC_MEM_DEBUGTRACE'
and 'ISC_MEM_DEBUGUSAGE' to set the global debugging flags at any
program using the memory contexts.
Instead of having individual memory contexts scattered across different
files and called different names, add a single memory context called
isc_g_mctx that replaces named_g_mctx and various other global memory
contexts in various utilities and tests.
Parts of ns_plugin_expandpath() test expected the plugin extension to be
appened automatically (the plugin name/path is provided without the
extension), this enable to test the logic which adds the correct
extension based on the platfrom.
But the expected expanded paths from the test were hard coded with the
`.so` extension, so the test can't pass on macOS platform. This fixes
the test by using the macro providing the current-platform extension.
MR !10753 breaks macOS build for plugin unit test as its linker doesn't
supports `--wrap` option, which is used in in order to mock the function
`isc_file_exits()`.
To work around the problem, a mocked `isc_file_exits()` is implemented
inside the plugin test as a static function before inlining the file
using it, which effectively links to this version rather than the isclib
one.
Update existing ns_plugin_expandpath() unit test to cover the logic
appending the plugin extension if missing.
Because ns_plugin_expandpath() now relies on isc_file_exists() API, a
mocked version has been added in tests/ns/plugin_test.c and relies on the
linker --wrap mechanism.
Locally, clang reported following odr-violation:
=================================================================
==1132009==ERROR: AddressSanitizer: odr-violation (0x555555589280):
[1] size=8 'isc__loopmgr' ../lib/isc/loop.c:52:16 in /home/ondrej/Projects/bind9/build/tests/isc/loop
[2] size=8 'isc__loopmgr' ../lib/isc/loop.c:52:16 in /home/ondrej/Projects/bind9/build/tests/isc/../../libisc.so
These globals were registered at these points:
[1]:
#0 0x7ffff785306f in __asan_register_globals ../../../../src/libsanitizer/asan/asan_globals.cpp:350
#1 0x55555556abce in _sub_I_00099_1 (/home/ondrej/Projects/bind9/build/tests/isc/loop+0x16bce) (BuildId: e7c586e966e6986532a3da40df41223ae16e55c9)
#2 0x7ffff702a303 in call_init ../csu/libc-start.c:145
#3 0x7ffff702a303 in __libc_start_main_impl ../csu/libc-start.c:347
#4 0x5555555622e4 in _start (/home/ondrej/Projects/bind9/build/tests/isc/loop+0xe2e4) (BuildId: e7c586e966e6986532a3da40df41223ae16e55c9)
[2]:
#0 0x7ffff785306f in __asan_register_globals ../../../../src/libsanitizer/asan/asan_globals.cpp:350
#1 0x7ffff75335b9 in _sub_I_00099_1 (/home/ondrej/Projects/bind9/build/tests/isc/../../libisc.so+0x1335b9) (BuildId: 33ab72bc676e9ef9111b3db1fc4347595069cd29)
#2 0x7ffff7fca71e in call_init elf/dl-init.c:74
#3 0x7ffff7fca823 in call_init elf/dl-init.c:120
#4 0x7ffff7fca823 in _dl_init elf/dl-init.c:121
#5 0x7ffff7fe459f (/lib64/ld-linux-x86-64.so.2+0x1f59f) (BuildId: 281ac1521b4102509b1c7ac7004db7c1efb81796)
==1132009==HINT: if you don't care about these errors you may set ASAN_OPTIONS=detect_odr_violation=0
SUMMARY: AddressSanitizer: odr-violation: global 'isc__loopmgr' at ../lib/isc/loop.c:52:16 in /home/ondrej/Projects/bind9/build/tests/isc/loop
==1132009==ABORTING
Aborted (core dumped)
Rename isc__loopmgr when including the loop.c into loop_test.c to
prevent odr-violation over isc__loopmgr.
Locally, clang reported odr-violation:
=================================================================
==588371==ERROR: AddressSanitizer: odr-violation (0x55555556a060):
[1] size=256 'client_addrs' ../tests/ns/netmgr_wrap.c:36:18 in /home/ondrej/Projects/bind9/build/tests/ns/query
[2] size=256 'client_addrs' ../tests/ns/netmgr_wrap.c:36:18 in /home/ondrej/Projects/bind9/build/tests/ns/../libbindtest.so
These globals were registered at these points:
[1]:
#0 0x7ffff785306f in __asan_register_globals ../../../../src/libsanitizer/asan/asan_globals.cpp:350
#1 0x7ffff6a2a303 in call_init ../csu/libc-start.c:145
#2 0x7ffff6a2a303 in __libc_start_main_impl ../csu/libc-start.c:347
#3 0x55555555a084 in _start (/home/ondrej/Projects/bind9/build/tests/ns/query+0x6084) (BuildId: fbe4a3fcf1a249c7d7da69ee8b255a1dbb610c7a)
[2]:
#0 0x7ffff785306f in __asan_register_globals ../../../../src/libsanitizer/asan/asan_globals.cpp:350
#1 0x7ffff7fca71e in call_init elf/dl-init.c:74
#2 0x7ffff7fca823 in call_init elf/dl-init.c:120
#3 0x7ffff7fca823 in _dl_init elf/dl-init.c:121
#4 0x7ffff7fe459f (/lib64/ld-linux-x86-64.so.2+0x1f59f) (BuildId: 281ac1521b4102509b1c7ac7004db7c1efb81796)
==588371==HINT: if you don't care about these errors you may set ASAN_OPTIONS=detect_odr_violation=0
SUMMARY: AddressSanitizer: odr-violation: global 'client_addrs' at ../tests/ns/netmgr_wrap.c:36:18 in /home/ondrej/Projects/bind9/build/tests/ns/query
==588371==ABORTING
Move the client_addrs and client_refs to libtest to prevent this.
There is only a single network manager running on top of the loop
manager (except for tests). Refactor the network manager to be a
singleton (a single instance) and change the unit tests, so that the
shorter read timeouts apply only to a specific handle, not the whole
extra 'connect_nm' network manager instance.
All the applications built on top of the loop manager were required to
create just a single instance of the loop manager. Refactor the loop
manager to not expose this instance to the callers and keep the loop
manager object internal to the isc_loop compilation unit.
This significantly simplifies a number of data structures and calls to
the isc_loop API.
The beauty and horrors of the C - the compiler properly detects variable
shadowing, but you can freely shadow a standard function 'free()' with
variable called 'free'. And if you reference 'free()' just as 'free'
you get the function pointer which means you can do also pointer
arithmetics, so 'free > 0' is always valid even when you delete the
local variable.
Replace the local variables 'free' with a name that doesn't shadow the
'free()' function to prevent future hard to detect bugs.
Now that we have to code working, rename 'dns_qp_lookup2' back to
'dns_qp_lookup' and adjust all remaining 'dns_qp_lookup' occurrences
to take a space=0 parameter.
For now we only allow DNS_DB_NSEC_* values so it makes sense to change
the type to an enum.
Rename 'denial' to the more intuitive 'space', indicating the namespace
of the keyvalue pair.
If zone and denial data are going to be stored in the same qp storage,
the unit tests need to be updated to reflect this change. The code
changes mainly affect name to qpkey conversion, lookups, and
predecessors.
A note on predecessors: since the denial and zone data are now in the
same qp storage, the predecessor of the first name in the zone data will
consequently be the last name in the denial data.
In preparation to merge the three qp tries (tree, nsec, nsec3) into
one, add the piece of information into the qpkey. This is the most
significant bit of information, so prepend the denial type to the qpkey.
This means we need to pass on the denial type when constructing the
qpkey from a name, or doing a lookup.
Reuse the the DNS_DB_NSEC_* values. Most qp tries in the code we just
pass on 0 (nta, rpz, zt, etc.), because there is no need for denial of
existence, but for qpzone and qpcache we must pass the right value.
Change the code, so that node->nsec no longer can have the value
DNS_DB_NSEC_HAS_NSEC, instead track this in a new attribute 'havensec'.
Since we use node->nsec to convert names to keys, the value MUST be set
before inserting the node into the qp-trie.
Update the fuzzing and unit tests accordingly. This only adds a few
extra test cases, more are needed.
In the qp_test.c we can remove test code for empty keys as this is
no longer possible.
RRset ordering is now an enum inside struct rdataset attributes. This
was done to keep size to of the structure to its original value before
this MR.
I expect zero performance impact but it should be easier to deal with
attributes in debuggers and language servers.
The ns_client_t struct is reset and zero-ed out on every query,
but some fields (query, message, manager) are preserved.
We observe two things:
- The sendbuf field is going to be overwritten anyway, there's
no need to zero it out.
- The fields are copied out when the struct is zero-ed out, and
then copied back in. For the query field (which is 896 bytes)
this is very inefficient.
This commit makes the reset more efficient avoiding to unnecessary
zero-ing and copy.
Instead of having hand crafted attach/detach/destroy functions, replace
them with the standard ISC_REFCOUNT macro. This also have advantage
that delayed netmgr detach (from dns_dispatch) now doesn't cause
assertion failure. This can happen with delayed (call_rcu) shutdown of
dns_adb.
Qpzone employs a locking strategy where rwlocks are grouped into
buckets, and each zone gets 17 buckets.
This strategy is suboptimal in two ways:
- If named is serving a single zone or a zone is the majority of the
traffic, this strategy pretty much guarantees contention when using
more than a dozen threads.
- If named is serving many small zones, it causes substantial memory
usage.
This commit switches the locking to a global table initialized at start
time. This should have three effects:
- Performance should improve in the single zone case, since now we are
selecting from a bigger pool of locks.
- Memory consumption should go down significantly in the many zone
cases.
- Performance should not degrade substantially in the many zone cases.
The reason for this is that, while we could have substantially more
zones than locks, we can query/edit only O(num threads) at the same
time. So by making the global table much bigger than the expected
number of threads, we can limit contention.
In the current implementation, the resigning heap is part of the zone
database. This leads to a cycle, as the database has a reference to its
nodes, but each node needs a reference to the database.
This MR splits the resigning heap into its own separate struct, in order
to help breaking the cycle.
Recovering the node lock from a pointer to the header and a pointer to
the db is a common operation. This commit abstracts it away into a
function, so that the node lock selection logic may be modified more
easily.
Change the internal type used for isc_tid unit to isc_tid_t to hide the
specific integer type being used for the 'tid'. Internally, the signed
integer type is being used. This allows us to have negatively indexed
arrays that works both for threads with assigned tid and the threads
with unassigned tid. This should be used only in specific situations.
We need disable clang-format here to preserve the brackets around
the string concatenation to prevent -Wstring-concatenation -Werror
breaking the build.
Add support for proposed DS digest types that encode the private
algorithm identifier at the start of the DS digest as is done for
DNSKEY and RRSIG. This allows a DS record to identify the specific
DNSSEC algorithm, rather than a set of algorithms, when the algorithm
field is set to PRIVATEDNS or PRIVATEOID.
Meson is a modern build system that has seen a rise in adoption and some
version of it is available in almost every platform supported.
Compared to automake, meson has the following advantages:
* Meson provides a significant boost to the build and configuration time
by better exploiting parallelism.
* Meson is subjectively considered to be better in readability.
These merits alone justify experimenting with meson as a way of
improving development time and ergonomics. However, there are some
compromises to ensure the transition goes relatively smooth:
* The system tests currently rely on various files within the source
directory. Changing this requirement is a non-trivial task that can't
be currently justified. Currently the last compiled build directory
writes into the source tree which is in turn used by pytest.
* The minimum version supported has been fixed at 0.61. Increasing this
value will require choosing a baseline of distributions that can
package with meson. On the contrary, there will likely be an attempt
to decrease this value to ensure almost universal support for building
BIND 9 with meson.
The cache for unreachable primaries was added to BIND 9 in 2006 via
1372e172d0e0b08996376b782a9041d1e3542489. It features a 10-slot LRU
array with 600 seconds (10 minutes) fixed delay. During this time, any
primary with a hiccup would be blocked for the whole block duration
(unless overwritten by a different entry).
As this design is not very flexible (i.e. the fixed delay and the fixed
amount of the slots), redesign it based on the badcache.c module, which
was implemented earlier for a similar mechanism.
The differences between the new code and the badcache module were large
enough to create a new module instead of trying to make the badcache
module universal, which could complicate the implementation.
The new design implements an exponential backoff for entries which are
added again soon after expiring, i.e. the next expiration happens in
double the amount of time of the previous expiration, but in no more
time than the defined maximum value.
The initial and the maximum expiration values are hard-coded, but, if
required, it should be trivial to implement configurable knobs.
Special tokens can now be specified in a zone "file" option
in order to generate the filename parametrically. The first
instead of "$name" in the "file" option is replaced with the
zone origin, the first instance of "$type" is replaced with the
zone type (i.e., primary, secondary, etc), and the first instance
of "$view" is replaced with the view name..
This simplifies the creation of zones using initial-file templates.
For example:
$ rndc addzone <zonename> \
{ type primary; file "$name.db"; initial-file "template.db"
When loading a primary zone for the first time, if the zonefile
does not exist but an "initial-file" option has been set, then a
new file will be copied into place from the path specified by
"initial-file".
This can be used to simplify the process of adding new zones. For
instance, a template zonefile could be used by running:
$ rndc addzone example.com \
'{ type primary; file "example.db"; initial-file "template.db"; };'
Coverity flagged a potential divide by zero error in collect in
qpmulti.c when the elapsed time is zero but that is only called
once the elapsed time is greater than or equal to RUNTIME (1/4
second) so INSIST this is the case.
Instead of giving the memory pools names with an explicit call to
isc_mempool_setname(), add the name to isc_mempool_create() call to have
all the memory pools an unconditional name.
Instead of giving the memory context names with an explicit call to
isc_mem_setname(), add the name to isc_mem_create() call to have all the
memory contexts an unconditional name.
previously, ISC_LIST_FOREACH and ISC_LIST_FOREACH_SAFE were
two separate macros, with the _SAFE version allowing entries
to be unlinked during the loop. ISC_LIST_FOREACH is now also
safe, and the separate _SAFE macro has been removed.
similarly, the ISC_LIST_FOREACH_REV macro is now safe, and
ISC_LIST_FOREACH_REV_SAFE has also been removed.
The `max-rsa-exponent-size` could limit the exponents of the RSA
public keys during the DNSSEC verification. Instead of providing
a cryptic (not cryptographic) knob, hardcode the max exponent to
be 4096 (the theoretical maximum for DNSSEC).
The DST API has been cleaned up, duplicate functions has been squashed
into single call (verify and verify2 functions), and couple of unused
functions have been completely removed (createctx2, computesecret,
paramcompare, and cleanup).
