In order to make upcoming configuration tree changes easier, the
cfg_map_firstclause() and _nextclause() functions have been changed
to return the clause itself rather than only the clause name.
Add a unit test for `cfg_obj_clone` to verify that the cloned tree
indeed has independent child nodes. The test also verifies that the
clone is semantically correct by comparing a text dump of the original
tree and the cloned one.
Instead of (1) allocating a parser, (2) parsing a file/buffer then (3)
freeing the parser, the parser is now internally created/destroyed from
within the `cfg_parse_*` functions. This simplifies a lot the use cases,
especially around the error cases where the parser needs to be freed in
a cleanup goto.
The only trick was the parser callback mechanism, which would previously
have been set up between steps 1 and 2. Since it's never been used for
any purpose other than the "directory" option, the chdir call has now
been moved inside the parser and the generic callback mechanism has been
removed, replacing CFG_CLAUSEFLAG_CALLBACK with CFG_CLAUSEFLAG_CHDIR.
cfg_obj_t doesn't store a pointer to its a parser context anymore,
and does not depend on the parser's lifecycle. Instead, it stores a
reference to its own memory context (and in principle, each node
could have different memory context). This also slightly simplifies
the _destroy API as there is no need to pass a context through it
anymore.
When generating a new key, dnssec-keygen checks for possible
key ID collisions with existing keys. The dnssec.c:findmatchingkeys()
function, which is supposed to get the list of the existing keys,
fails to do that for the existing KEY rrtype keys (i.e. generated
using 'dnssec-keygen -T KEY') because it doesn't pass down to the
dst_key_fromnamedfile() -> dst_key_read_public() functions the type
of the keys it's interested in. Fix the issue by introducing a new
function parameter which tells in which type of keys the caller is
currently interested in.
It was discovered in an upcoming academic paper that a xoshiro128**
internal state can be recovered by an external 3rd party allowing to
predict UDP ports and DNS IDs in the outgoing queries. This could lead
to an attacker spoofing the DNS answers with great efficiency and
poisoning the DNS cache.
Change the internal random generator to system CSPRNG with buffering to
avoid excessive syscalls.
Thanks Omer Ben Simhon and Amit Klein of Hebrew University of Jerusalem
for responsibly reporting this to us. Very cool research!
Add an API to parse and extract either an IPv4 or IPv6 address from
a name using the reverse format. It takes care of family detection,
and returns a generic error in case of syntax error.
The field `ns_hookasync_t` was initially named `hook_actx` and wrongly
renamed `hook_aclctx` during a mass-renaming of various names for the
config acl context into a consistent `aclctx` name (see !11003). Of
course this is wrong as `ns_hookasync_t` has nothing to do with ACL but
about _async_ context. This commit fixes the mistake by renaming this
field `hookasyncctx`
ACL configuration context variables are inconsistently named as `actx`,
`ac`, or `aclconfctx`, which caused confusion during code reviews. This
commit renames all `cfg_aclconfctx_t` variables to `aclctx`, which is
short, consistent, and unambiguous.
Simplify the implementation around the database ownercase. Remove the
dns_rdataset_setownercase() implementation for the slabheaders and only
allow setting ownercase on rdatalists and rdatasets. The ownercase in
the database can now be set only with dns_db_addrdataset() by passing
rdataset with correctly set ownercase.
query_reset() is called during query initialization, but the only
time the NS_QUERY_SETUP hook runs is when it's called from
query_cleanup(). it makes more sense to move the hook point to
there and rename it to NS_QUERY_CLEANUP.
this change caused a crash in the unit tests due to the view being
unnecessarily detached before ns__client_reset_cb() was called.
this has also been fixed.
Update hooks-related unit tests since the removal of
NS_QUERY_QCTX_DESTROY and the introduction of NS_QUERY_RESET hook. This
also simplifies (a bit) the plugin usage as NS_QUERY_RESET is _always_
called when the client plugin is about to be freed from memory.
A new query unit test covers the logic where zone hooks must be called
first, then view ones, and finally the default hooks. It also ensures
that if any hook returns NS_HOOK_RETURN the chain immediately stops.
Mimic the Unbound behaviour where the cyclic offset is taken from query
ID, and remove recording of the current state. As the incoming query ID
should have random distribution, the cyclic ordering should also have
uniform distribution of the starting record.
Extend the `$name`, `$view` and `$type` tokens (expanding into the zone
name, zone's view name and type); the new following tokens are now also
accepted:
- $name or %s is replaced with the zone name in lower case;
- $type or %t is replaced with the zone type -- i.e., primary,
secondary, etc);
- $view or %v is replaced with the view name;
- $char1 or %1 is replaced with the first character of the zone name;
- $char2 or %2 is replaced with the second character of the zone name
(or a dot if there is no second character);
- $char3 or %3 is replaced with the third character of the zone name (or
a dot if there is no third character);
- $label1 or %z is replaced with the toplevel domain of the zone (or a
dot if it is the root zone);
- $label2 or %y is replaced with the next label under the toplevel
domain (or a dot if there is no next label);
- $label3 or %x is replaced with the next-next label under the toplevel
domain (or a dot if there is no next-next label).
> 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.
