Ondřej Surý [Tue, 4 Oct 2022 15:07:19 +0000 (17:07 +0200)]
Record the 'edns-udp-size' in the view, not in the resolver
Getting the recorded value of 'edns-udp-size' from the resolver requires
strong attach to the dns_view because we are accessing `view->resolver`.
This is not the case in places (f.e. dns_zone unit) where `.udpsize` is
accessed. By moving the .udpsize field from `struct dns_resolver` to
`struct dns_view`, we can access the value directly even with weakly
attached dns_view without the need to lock the view because `.udpsize`
can be accessed after the dns_view object has been shut down.
Ondřej Surý [Mon, 3 Oct 2022 13:54:12 +0000 (15:54 +0200)]
Resolve violation of weak referencing dns_view
The dns_view implements weak and strong reference counting. When strong
reference counting reaches zero, the adb, ntatable and resolver objects
are shut down and detached.
In dns_zone and dns_nta the dns_view was weakly attached, but the
view->resolver reference was accessed directly leading to dereferencing
the NULL pointer.
Add dns_view_getresolver() method which attaches to view->resolver
object under the lock (if it still exists) ensuring the dns_resolver
will be kept referenced until not needed.
Tony Finch [Wed, 5 Oct 2022 11:11:41 +0000 (12:11 +0100)]
Avoid dead code warning when using a constant boolean
The value of `sign_bit` is platform-dependent but constant at compile
time. Use a cast to convert the boolean `sign_bit` to 0 or 1 instead of
ternary `?:` because one branch of the conditional is dead code. (We
could leave out the cast to `size_t` but our style prefers to handle
booleans more explicitly, hence the `?:` that caused the issue.)
*** CID 358310: Possible Control flow issues (DEADCODE)
/lib/isc/resource.c: 118 in isc_resource_setlimit()
112 * rlim_t, and whether rlim_t has a sign bit.
113 */
114 isc_resourcevalue_t rlim_max = UINT64_MAX;
115 size_t wider = sizeof(rlim_max) - sizeof(rlim_t);
116 bool sign_bit = (double)(rlim_t)-1 < 0;
117
>>> CID 358310: Possible Control flow issues (DEADCODE)
>>> Execution cannot reach the expression "1" inside this statement: "rlim_max >>= 8UL * wider + ...".
118 rlim_max >>= CHAR_BIT * wider + (sign_bit ? 1 : 0);
119 rlim_value = ISC_MIN(value, rlim_max);
120 }
121
122 rl.rlim_cur = rl.rlim_max = rlim_value;
123 unixresult = setrlimit(unixresource, &rl);
Ondřej Surý [Fri, 26 Aug 2022 09:58:51 +0000 (11:58 +0200)]
Use isc_mem_regetx() when appropriate
While refactoring the isc_mem_getx(...) usage, couple places were
identified where the memory was resized manually. Use the
isc_mem_reget(...) that was introduced in [GL !5440] to resize the
arrays via function rather than a custom code.
Ondřej Surý [Fri, 26 Aug 2022 09:58:51 +0000 (11:58 +0200)]
Use designated initializers instead of memset()/MEM_ZERO for structs
In several places, the structures were cleaned with memset(...)) and
thus the semantic patch converted the isc_mem_get(...) to
isc_mem_getx(..., ISC_MEM_ZERO). Use the designated initializer to
initialized the structures instead of zeroing the memory with
ISC_MEM_ZERO flag as this better matches the intended purpose.
Ondřej Surý [Fri, 3 Jun 2022 10:23:49 +0000 (12:23 +0200)]
Replace isc_mem_*_aligned(..., alignment) with isc_mem_*x(..., flags)
Previously, the isc_mem_get_aligned() and friends took alignment size as
one of the arguments. Replace the specific function with more generic
extended variant that now accepts ISC_MEM_ALIGN(alignment) for aligned
allocations and ISC_MEM_ZERO for allocations that zeroes
the (re-)allocated memory before returning the pointer to the caller.
Petr Špaček [Tue, 4 Oct 2022 09:00:54 +0000 (11:00 +0200)]
Remove manually defined anchors pointing to statement definitions
This is hopefully end of duplication. This batch did not cause clashes
in Sphinx but it was pointless nonetheless as we have auto-generated
anchors for all statements.
Petr Špaček [Fri, 30 Sep 2022 11:57:17 +0000 (13:57 +0200)]
Deduplicate link anchors in the ARM
Some statement names like "allow-query" had manually defined link anchor
_allow-query and also implicit anchor created by
.. namedconf:statement:: syntax. This causes warnings if a ambiguous
reference is made using :any:`allow-query` syntax.
Remove (hopefully all) manually defined anchors which pointed to
identical place as the implicit anchor. This allows :any: to work.
In rare cases where manual anchor points to descriptive text separated
from statement definition the reference was disamguated by replacing
:any:`notify` with :ref:`notify` (for manual anchor)
vs. :namedconf:ref:`notify` (for statement definition).
Please note that `options` statement is a trap: It is ambiguous even
without manual anchor because rndc.conf has its own `options`. Use
:namedconf:ref:`options` vs. :rndcconf:ref:`options` to select
appropriate target.
We forgot to update TSAN paths when moving all the unit tests to
/tests/. Let's remove paths from find to make it less dependent on
exact location, and store all untracked files as we do in the normal
unit test template.
If refresh stale RRset times out, start stale-refresh-time
The previous commit failed some tests because we expect that if a
fetch fails and we have stale candidates in cache, the
stale-refresh-time window is started. This means that if we hit a stale
entry in cache and answering stale data is allowed, we don't bother
resolving it again for as long we are within the stale-refresh-time
window.
This is useful for two reasons:
- If we failed to fetch the RRset that we are looking for, we are not
hammering the authoritative servers.
- Successor clients don't need to wait for stale-answer-client-timeout
to get their DNS response, only the first one to query will take
the latency penalty.
The latter is not useful when stale-answer-client-timeout is 0 though.
So this exception code only to make sure we don't try to refresh the
RRset again if it failed to do so recently.
Refreshing a stale RRset is similar to prefetching an RRset, so
reuse the existing code. When refreshing an RRset we need to clear
all db options related to serve-stale so that stale RRsets in cache
are ignored during the refresh.
We no longer need to set the "nodetach" flag, because the refresh
fetch is now a "fetch and forget". So we can detach from the client
in the query_send().
This code will break some serve-stale test cases, this will be fixed
in the successor commit.
TODO: add explanation why the serve-stale test cases fail.
Ondřej Surý [Mon, 27 Jun 2022 08:13:19 +0000 (10:13 +0200)]
Add a case-insensitive option directly to siphash 2-4 implementation
Formerly, the isc_hash32() would have to change the key in a local copy
to make it case insensitive. Change the isc_siphash24() and
isc_halfsiphash24() functions to lowercase the input directly when
reading it from the memory and converting the uint8_t * array to
64-bit (respectively 32-bit numbers).
Mark Andrews [Thu, 15 Sep 2022 06:04:43 +0000 (16:04 +1000)]
Add support for 'dohpath' to SVCB (and HTTPS)
dohpath is specfied in draft-ietf-add-svcb-dns and has a value
of 7. It must be a relative path (start with a /), be encoded
as UTF8 and contain the variable dns ({?dns}).
Tony Finch [Tue, 20 Sep 2022 13:32:01 +0000 (14:32 +0100)]
Avoid signed integer overflow in isc_resource_setlimit()
On systems with signed rlim_t the old code calculated its maximum
value by shifting 1 into the sign bit, which is undefined behaviour.
Avoid the bug by using an unsigned shift.
Be more patient when stopping servers in the system tests
When the TCP test is run on the busy server, the server might take a
while to wind the server down because it might still be processing all
that 300k invalid XFR requests.
Increate the rncd wait time to 120 seconds, the SIGTERM time to 300
seconds, and reduce the time to wait for ans servers from 1200 second
to just 120 seconds.
The dns__nta_shutdown() could be run from different threads and it was
accessing nta->timer unlocked. Don't check and stop the timer from
dns__nta_shutdown() directly, but leave it for the async callback.
Because the dns_zonemgr_create() was run before the loopmgr was started,
the isc_ratelimiter API was more complicated that it had to be. Move
the dns_zonemgr_create() to run_server() task which is run on the main
loop, and simplify the isc_ratelimiter API implementation.
The isc_timer is now created in the isc_ratelimiter_create() and
starting the timer is now separate async task as is destroying the timer
in case it's not launched from the loop it was created on. The
ratelimiter tick now doesn't have to create and destroy timer logic and
just stops the timer when there's no more work to do.
This should also solve all the races that were causing the
isc_ratelimiter to be left dangling because the timer was stopped before
the last reference would be detached.
Mark Andrews [Thu, 29 Sep 2022 02:12:58 +0000 (12:12 +1000)]
Create a key directory in /tmp
Access to the source tree is not available with oss_fuzz. Have
fuzz/dns_message_checksig build and populate a key directory for
the fuzzer to use. This contains a key pair and a zone file which
has the public key from the key pair. Clean it up on shutdown.
Tony Finch [Mon, 13 Jun 2022 11:00:01 +0000 (12:00 +0100)]
Improve DBC in isc_mem_free
Unlike standard free(), isc_mem_free() is not a no-op when passed a
NULL pointer. For size accounting purposes it calls sallocx(), which
crashes when passed a NULL pointer. To get more helpful diagnostics,
REQUIRE() that the pointer is not NULL so that when the programmer
makes a mistake they get a backtrace that shows what went wrong.
Add developer documentation on the netmgr callbacks
Extra care must be taken when executing the callbacks to prevent the
deadlocks on the caller's side. Add a paragraph that addresses when we
can and when we cannot call the callbacks directly.
Call the isc__nm_udp_send() callbacks asynchronously on shutdown
The isc__nm_udp_send() callback would be called synchronously when
shutting down or when the socket has been closed. This could lead to
double locking in the calling code and thus those callbacks needs to be
called asynchronously.
Test dynamic update forwarding when using a TLS-enabled primary
Add several test cases in the 'upforwd' system test to make sure
that different scenarios of Dynamic DNS update forwarding are
tested, in particular when both the original and forwarded requests
are over Do53, or DoT, or they use different transports.
Fix dynamic update forwarding when using a TLS-enabled primary
Now that the 'dns_request' supports using TLS transport, implement
dynamic update forwarding using DoT when the primary server is
configured to use a TLS transport.
Previously, when using such configuration, the dynamic update forwarding
feature was broken.
Add autoconf option to enable memory leak detection in libraries
There's a known memory leak in the engine_pkcs11 at the time of writing
this and it interferes with the named ability to check for memory leaks
in the OpenSSL memory context by default.
Add an autoconf option to explicitly enable the memory leak detection,
and use it in the CI except for pkcs11 enabled builds. When this gets
fixed in the engine_pkc11, the option can be enabled by default.
Check the libuv, OpenSSL and libxml2 memory context on exit
As we can't check the deallocations done in the library memory contexts
by default because it would always fail on non-clean exit (that happens
on error or by calling exit() early), we just want to enable the checks
to be done on normal exit.
The libxml2 library provides a way to replace the default allocator with
user supplied allocator (malloc, realloc, strdup and free).
Create a memory context specifically for libxml2 to allow tracking the
memory usage that has originated from within libxml2. This will provide
a separate memory context for libxml2 to track the allocations and when
shutting down the application it will check that all libxml2 allocations
were returned to the allocator.
Additionally, move the xmlInitParser() and xmlCleanupParser() calls from
bin/named/main.c to library constructor/destructor in libisc library.
The OpenSSL library provides a way to replace the default allocator with
user supplied allocator (malloc, realloc, and free).
Create a memory context specifically for OpenSSL to allow tracking the
memory usage that has originated from within OpenSSL. This will provide
a separate memory context for OpenSSL to track the allocations and when
shutting down the application it will check that all OpenSSL allocations
were returned to the allocator.
The libuv library provides a way to replace the default allocator with
user supplied allocator (malloc, realloc, calloc and free).
Create a memory context specifically for libuv to allow tracking the
memory usage that has originated from within libuv. This requires
libuv >= 1.38.0 which provides uv_library_shutdown() function that
assures no more allocations will be made.
Check for working __builtin_mul_overflow() implementation
Instead of using generic HAVE_BUILTIN_OVERFLOW, we need to check whether
the overflow functions actually work as there was a bug in GCC that it
would not detect mul overflow when compiled with `-m32` option without
optimizations and the bug was fixed only for GCC 6.5+ and 7.3+/8+.
For further details see: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82274
Make the debugging flags local to the memory context
Previously, the isc_mem_debugging would be single global variable that
would affect the behavior of the memory context whenever it would be
changed which could be after some allocation were already done.
Change the memory debugging options to be local to the memory context
and immutable, so all allocations within the same memory context are
treated the same.
By bumping the minimum libuv version to 1.34.0, it allows us to remove
all libuv shims we ever had and makes the code much cleaner. The
up-to-date libuv is available in all distributions supported by BIND
9.19+ either natively or as a backport.
Mark Andrews [Thu, 15 Sep 2022 09:18:53 +0000 (19:18 +1000)]
Convert DST_ALG defines to enum and group HMAC algorithms
The HMACs and GSSAPI are just using unallocated values.
Moving them around shouldn't cause issues.
Only the dnssec system test knew the internal number in use for hmacmd5.