The new unit isc_mem_overmem unit test sets hi and lo water marks and
then does allocations to go over:
0. x < lo_water
1. lo_water < x < hi_water
2. x > hi_water
3. lo_water < x < hi_water
4. < lo_water
Previously, there were two methods of working with the overmem
condition:
1. hi/lo water callback - when the overmem condition was reached
for the first time, the water callback was called with HIWATER
mark and .is_overmem boolean was set internally. Similarly,
when the used memory went below the lo water mark, the water
callback would be called with LOWATER mark and .is_overmem
was reset. This check would be called **every** time memory
was allocated or freed.
2. isc_mem_isovermem() - a simple getter for the internal
.is_overmem flag
This commit refactors removes the first method and move the hi/lo water
checks to the isc_mem_isovermem() function, thus we now have only a
single method of checking overmem condition and the check for hi/lo
water is removed from the hot path for memory contexts that doesn't use
overmem checks.
The AES algorithm for DNS cookies was being kept for legacy reasons, and
it can be safely removed in the next major release. Remove both the AES
usage for DNS cookies and the AES implementation itself.
The client connection timeout was set to just one second, which might
not be enough on busy systems (and the CI machines are oh-boy-busy).
Bump the server timeouts to 10 seconds and client timeouts to 5 seconds,
this will make the unit test run a little bit longer, but it should be
more reliable.
All changes in this commit were automated using the command:
shfmt -w -i 2 -ci -bn . $(find . -name "*.sh.in")
By default, only *.sh and files without extension are checked, so
*.sh.in files have to be added additionally. (See mvdan/sh#944)
Refactor the dispatch unit test to use more local variables (previously
dispatchmgr, dispatch and dispentry were all global), and add two new
tests:
* dispatch_getcp - test whether the TCP connection will get reused
* dispatch_newtcp - test that the TCP connection will not get reused
when DNS_DISPATCHOPT_UNSHARED is in effect
The current dispatch code could reuse the TCP connection when
dns_dispatch_gettcp() would be used first. This is problematic as the
dns_resolver doesn't use TCP connection sharing, but dns_request could
get the TCP stream that was created outside of the dns_request.
Add new DNS_DISPATCHOPT_UNSHARED option to dns_dispatch_createtcp() that
would prevent the TCP stream to be reused. Use that option in the
dns_resolver call to dns_dispatch_createtcp() to prevent dns_request
from reusing the TCP connections created by dns_resolver.
Additionally, the dns_xfrin unit added TCP connection sharing for
incoming transfers. While interleaving *xfr streams on a TCP connection
should work this should be a deliberate change and be property of the
server that can be controlled. Additionally some level of parallel TCP
streams is desirable. Revert to the old behaviour by removing the
dns_dispatch_gettcp() calls from dns_xfrin and use the new option to
prevent from sharing the transfer streams with dns_request.
In order to check whether there are enough inserted values the
code uses the 'tests' variable (loop counter), which is unreliable,
because the loop sometimes removes an item instead of inserting
one (when the randomly generated item already exists).
Instead of the loop counter, use the existing variable 'inserted',
which should indicate the correct number of the inserted items.
depending on how the QP trie is traversed during a lookup, it is
possible for a search to terminate on a leaf which is a partial
match, without that leaf being added to the chain. to ensure the
chain is correct in this case, when a partial match condition is
detected via qpkey_compare(), we will call add_link() again, just
in case. (add_link() will check for a duplicated node, so it will
be harmless if it was already done.)
This was generated from dnsperf queryfile with following script:
#!/usr/bin/env python3
names = {}
import sys
i = 0
for line in iter(sys.stdin.readline, ''):
name = line.rstrip('\n')
if not name in names:
names[name] = line
print(f"{i},{name}")
i += 1
if i >= 1024*1024:
break
The name_match() was errorneously converting struct item into dns_name
pointer. Correctly retype void *node to struct item * first and then
use item.fixed.name to pass the name to dns_name_equal() function.
The load_names benchmark expected the input CSV with domains would fill
the whole item array and it would crash when the number of lines would
be less than that.
Fix the expectations by using the real number or lines read to calculate
the array start and end position for each benchmark thread.
dns_qp_findname_ancestor() now takes an optional 'predecessor'
parameter, which if non-NULL is updated to contain the DNSSEC
predecessor of the name searched for. this is done by constructing
an iterator stack while carrying out the search, so it can be used
to step backward if needed.
since dns_qp_findname_ancestor() can now return a chain object, it is no
longer necessary to provide a _NOEXACT search option. if we want to look
up the closest ancestor of a name, we can just do a normal search, and
if successful, retrieve the second-to-last node from the QP chain.
this makes ancestor lookups slightly more complicated for the caller,
but allows us to simplify the code in dns_qp_findname_ancestor(), making
it easier to ensure correctness. this was a fairly rare use case:
outside of unit tests, DNS_QPFIND_NOEXACT was only used in the zone
table, which has now been updated to use the QP chain. the equivalent
RBT feature is only used by the resolver for cache lookups of 'atparent'
types (i.e, DS records).
- make iterators reversible: refactor dns_qpiter_next() and add a new
dns_qpiter_prev() function to support iterating both forwards and
backwards through a QP trie.
- added a 'name' parameter to dns_qpiter_next() (as well as _prev())
to make it easier to retrieve the nodename while iterating, without
having to construct it from pointer value data.
- the helper functions for accessing twigs beneath a branch
(branch_twig_pos(), branch_twig_ptr(), etc) were somewhat confusing
to read, since several of them were implemented by calling other
helper functions. they now all show what they're really doing.
- branch_twigs_vector() has been renamed to simply branch_twigs().
- revised some unrelated comments in qp_p.h for clarity.
dns_qp_findname_ancestor() now takes an optional 'chain' parameter;
if set, the dns_qpchain object it points to will be updated with an
array of pointers to the populated nodes between the tree root and the
requested name. the number of nodes in the chain can then be accessed
using dns_qpchain_length() and the individual nodes using
dns_qpchain_node().
add a 'foundname' parameter to dns_qp_findname_ancestor(),
and use it to set the found name in dns_nametree.
this required adding a dns_qpkey_toname() function; that was
done by moving qp_test_keytoname() from the test library to qp.c.
added some more test cases and fixed bugs with the handling of
relative and empty names.
this loads a file containing DNS names and measures the time it takes to:
1) iterate it,
2) look up each name with dns_qp_getname()
3) look up each name with dns_qp_findname_ancestor()
4) look up a modified name based on the name, to check performance
when the name is not found.
the refactoring of isc_job_run() and isc_async_run() in 9.19.12
intefered with the way the qpmulti benchmark uses uv_idle.
it has now been modified to use isc_job/isc_async instead.
Instead of creating new memory pools for each new dns_message, change
dns_message_create() method to optionally accept externally created
dns_fixedname_t and dns_rdataset_t memory pools. This allows us to
preallocate the memory pools in ns_client and dns_resolver units for the
lifetime of dns_resolver_t and ns_clientmgr_t.
When isc_hashmap_iter_delcurrent_next calls hashmap_delete_node
nodes from the front of the table could be added to the end of
the table resulting in them being returned twice. Detect when
this is happening and prevent those nodes being returned twice
buy reducing the effective size of the table by one each time
it happens.
Reusing TCP connections with dns_dispatch_gettcp() used linear linked
list to lookup existing outgoing TCP connections that could be reused.
Replace the linked list with per-loop cds_lfht hashtable to speedup the
lookups. We use cds_lfht because it allows non-unique node insertion
that we need to check for dispatches in different connection states.
Instead of high number of dispatches (4 * named_g_udpdisp)[1], make the
dispatches bound to threads and make dns_dispatchset_t create a dispatch
for each thread (event loop).
This required couple of other changes:
1. The dns_dispatch_createudp() must be called on loop, so the isc_tid()
is already initialized - changes to nsupdate and mdig were required.
2. The dns_requestmgr had only a single dispatch per v4 and v6. Instead
of using single dispatch, use dns_dispatchset_t for each protocol -
this is same as dns_resolver.
Refactor isc_hashmap to allow custom matching functions. This allows us
to have better tailored keys that don't require fixed uint8_t arrays,
but can be composed of more fields from the stored data structure.
When inserting items into hashtables (hashmaps), we might have a
fragmented key (as an example we might want to hash DNS name + class +
type). We either need to construct continuous key in the memory and
then hash it en bloc, or incremental hashing is required.
This incremental version of SipHash 2-4 algorithm is the first building
block.
As SipHash 2-4 is often used in the hot paths, I've turned the
implementation into header-only version in the process.
instead of allowing a NULL nametree in dns_nametree_covered(),
require nametree to exist, and ensure that the nametrees defined
for view and resolver objects are always created.
name trees can now also hold trees of counters. each time a name
dns_nametree_add() is called with a given name, the counter for that
name is incremented; the name is not deleted until dns_nametree_delete()
is called the same number of times.
this is meant to be used for synth-from-dnssec, which is incremented for
each key defined at a name, and decremented when a key is removed, the
name must continue to exist until the number of keys has reached zero.
name trees can now hold either boolean values or bit fields. the
type is selected when the name tree is created.
the behavior of dns_nametree_add() differs slightly beteween the types:
in a boolean tree adding an existing name will return ISC_R_EXISTS,
but in a bitfield tree it simply sets the specified bit in the bitfield
and returns ISC_R_SUCCESS.
this is a QP trie of boolean values to indicate whether a name is
included in or excluded from some policy. this can be used for
synth-from-dnssec, deny-answer-aliases, etc.
Use the new isc_mem_c*() calloc-like API for allocations that are
zeroed.
In turn, this also fixes couple of incorrect usage of the ISC_MEM_ZERO
for structures that need to be zeroed explicitly.
There are few places where isc_mem_cput() is used on structures with a
flexible member (or similar).
The resolution of the uv_hrtime() function is bigger than the
intervals used in the timers, which can result in an unexpected
difference between the start_time and stop_time variables.
Use isc_loop_now(), which is based on uv_now() and has the same
milliseconds resolution as the functions in the uv_timer_t API.
Also fix a couple wrong numbers in the comments.
Add tracing probes to isc_job unit:
* libisc:job_cb_before - before the job callback is called
* libisc:job_cb_after - after the job callback is called
This adds support for User Statically Defined Tracing (USDT). On
Linux, this uses the header from SystemTap and dtrace utility, but the
support is universal as long as dtrace is available.
Also add the required infrastructure to add probes to libisc, libdns and
libns libraries, where most of the probes will be.
The dns_dispatchmgr object was only set in the dns_view object making it
prone to use-after-free in the dns_xfrin unit when shutting down named.
Remove dns_view_setdispatchmgr() and optionally pass the dispatchmgr
directly to dns_view_create() when it is attached and not just assigned,
so the dns_dispatchmgr doesn't cease to exist too early.
The dns_view_getdnsdispatchmgr() is now protected by the RCU lock, the
dispatchmgr reference is incremented, so the caller needs to detach from
it, and the function can return NULL in case the dns_view has been
already shut down.
replace the red-black tree used by the negative trust anchor table
with a QP trie.
because of this change, dns_ntatable_init() can no longer fail, and
neither can dns_view_initntatable(). these functions have both been
changed to type void.
this function finds the closest matching ancestor, but the function
name could be read to imply that it returns the direct parent node;
this commit suggests a slightly less misleading name.
Make the `pval_r` and `ival_r` out arguments optional.
Add `pval_r` and `ival_r` out arguments to `dns_qp_deletekey()`
and `dns_qp_deletename()`, to return the deleted leaf.
Add simple rwlock unit test and rwlock benchmark. The benchmark
compares the pthread rwlock with isc rwlock implementation, so it's
mainly useful when developing a new isc rwlock implementation.
The load-names benchmark was originally only measuring single thread
performance of the data structures. As this is not how those are used
in the real life, it was refactored to be multi-threaded with proper
protections in place (rwlock for ht, hashmap and rbt; transactions for
qp).
The qp test has been extended to see effect of the dns_qp_compact() and
rcu_barrier() on the overall speed and memory consumption.
The dns_badcache unit had (yet another) own locked hashtable
implementation. Replace the hashtable used by dns_badcache with
lock-free cds_lfht implementation from liburcu.
The isc_stats_create() can no longer return anything else than
ISC_R_SUCCESS. Refactor isc_stats_create() and its variants in libdns,
libns and named to just return void.
Add a unit test to check if the overmem purging in the RBTDB is
effective when mixed size RR data is inserted into the database.
Co-authored-by: Ondřej Surý <ondrej@isc.org>
Co-authored-by: Jinmei Tatuya <jtatuya@infoblox.com>
These two configuration options worked in conjunction with 'auto-dnssec'
to determine KSK usage, and thus are now obsoleted.
However, in the code we keep KSK processing so that when a zone is
reconfigured from using 'dnssec-policy' immediately to 'none' (without
going through 'insecure'), the zone is not immediately made bogus.
Add one more test case for going straight to none, now with a dynamic
zone (no inline-signing).
store a pointer to the running loop when creating a dispatch entry
with dns_dispatch_add(), and use isc_loop_now() to get the timestamp for
the current event loop tick when we initialize the dispentry start time
and check for timeouts.
ultimately we want the slab implementation of dns_rdataset to
be usable by more database implementaions than just rbtdb. this
commit moves rdataset_methods to rdataslab.c, renamed
dns_rdataslab_rdatasetmethods.
new database methods have been added: locknode, unlocknode,
addglue, expiredata, and deletedata, allowing external functions to
perform functions that previously required internal access to the
database implementation.
database and heap pointers are now stored in the dns_slabheader object
so that header is the only thing that needs to be passed to some
functions; this will simplify moving functions that process slabheaders
out of rbtdb.c so they can be used by other database implementations.
to reduce the amount of common code that will need to be shared
between the separated cache and zone database implementations,
clean up unused portions of dns_db.
the methods dns_db_dump(), dns_db_isdnssec(), dns_db_printnode(),
dns_db_resigned(), dns_db_expirenode() and dns_db_overmem() were
either never called or were only implemented as nonoperational stub
functions: they have now been removed.
dns_db_nodefullname() was only used in one place, which turned out
to be unnecessary, so it has also been removed.
dns_db_ispersistent() and dns_db_transfernode() are used, but only
the default implementation in db.c was ever actually called. since
they were never overridden by database methods, there's no need to
retain methods for them.
in rbtdb.c, beginload() and endload() methods are no longer defined for
the cache database, because that was never used (except in a few unit
tests which can easily be modified to use the zone implementation
instead). issecure() is also no longer defined for the cache database,
as the cache is always insecure and the default implementation of
dns_db_issecure() returns false.
for similar reasons, hashsize() is no longer defined for zone databases.
implementation functions that are shared between zone and cache are now
prepended with 'dns__rbtdb_' so they can become nonstatic.
serve_stale_ttl is now a common member of dns_db.
in preparation for splitting up rbtdb.c, rename some types so they
can be defined in dns/types.h instead of only locally. these include:
- struct noqname, which is used to hold no-qname and closest-encloser
proofs, and is now named dns_proof_t;
- rbtdb_rdatatype_t, which is used to hold a pair of rdatatypes and
is now called dns_typepair_t and defined in rdatatype.h;
- rbtdb_serial_t, which is now just a uint32_t;
- rdatasetheader_t and rdatasetheaderlist_t, now called
dns_slabheader_t and dns_slabheaderlist_t;
- rbtdb_version_t, now called dns_rbtdb_version_t.
the helper functions header_from_raw() and raw_from_header() are
renamed dns_slabheader_fromrdataset() and dns_slabheader_raw().
also made further style changes:
- fixing uninitialized pointer variables throughout rbtdb.c;
- switching some initializations to struct literals;
- renaming some functions and struct members more descriptively;
- replacing dns_db_secure_t with a simple bool since it no longer needs
to be tri-valued.
BIND's rdataset structure is a view of some DNS records. It is
polymorphic, so the details of how the records are stored can vary.
For instance, the records can be held in an rdatalist, or in an
rdataslab in the rbtdb.
The dns_rdataset structure previously had a number of fields called
`private1` up to `private7`, which were used by the various rdataset
implementations. It was not at all clear what these fields were for,
without reading the code and working it out from context.
This change makes the rdataset inheritance hierarchy more clear. The
polymorphic part of a `struct dns_rdataset` is now a union of structs,
each of which is named for the class of implementation using it. The
fields of these structs replace the old `privateN` fields. (Note: the
term "inheritance hierarchy" refers to the fact that the builtin and
SDLZ implementations are based on and inherit from the rdatalist
implementation, which in turn inherits from the generic rdataset.
Most of this change is mechanical, but there are a few extras.
In keynode.c there were a number of REQUIRE()ments that were not
necessary: they had already been checked by the rdataset method
dispatch code. On the other hand, In ncache.c there was a public
function which needed to REQUIRE() that an rdataset was valid.
I have removed lots of "reset iterator state" comments, because it
should now be clear from `target->iter = NULL` where before
`target->private5 = NULL` could have been doing anything.
Initialization is a bit neater in a few places, using C structure
literals where appropriate.
The pointer arithmetic for translating between an rdataslab header and
its raw contents is now fractionally safer.
since it is not necessary to find partial matches when looking
up names in a TSIG keyring, we can use a hash table instead of
an RBT to store them.
the tsigkey object now stores the key name as a dns_fixedname
rather than allocating memory for it.
the `name` parameter to dns_tsigkeyring_add() has been removed;
it was unneeded since the tsigkey object already contains a copy
of the name.
the opportunistic cleanup_ring() function has been removed;
it was only slowing down lookups.
this function was no longer needed, because the algorithm name is no
longer copied into the tsigkey object by dns_tsigkey_createfromkey();
it's always just a pointer to a statically defined name.
the prior practice of passing a dns_name containing the
expanded name of an algorithm to dns_tsigkey_create() and
dns_tsigkey_createfromkey() is unnecessarily cumbersome;
we can now pass the algorithm number instead.
use the ISC_REFCOUNT attach/detach implementation in dns/tsig.c
so that detailed tracing can be used during refactoring.
dns_tsig_keyring_t has been renamed dns_tsigkeyring_t so the type
and the attach/detach function names will match.
- style cleanups.
- simplify the function parameters to dns_tsigkey_create():
+ remove 'restored' and 'generated', they're only ever set to false.
+ remove 'creator' because it's only ever set to NULL.
+ remove 'inception' and 'expiry' because they're only ever set to
(0, 0) or (now, now), and either way, this means "never expire".
+ remove 'ring' because we can just use dns_tsigkeyring_add() instead.
- rename dns_keyring_restore() to dns_tsigkeyring_restore() to match the
rest of the functions operating on dns_tsigkeyring objects.
dns_view_find* may be called after the final call to dns_view_detach
is made which detaches view->zonetable to permit the server to
shutdown. We need to detect if view->zonetable is NULL during this
stage and appropriately recover.
Remove the code implementing nonstardard behaviors that were formerly
needed to allow GSS-TSIG to work with Windows 2000, which passed
End-of-Life in 2010.
Deprecate the "oldgsstsig" command and "-o" command line option
to nsupdate; these are now treated as synonyms for "gsstsig" and "-g"
respectively.
These insertions are added to produce a radix tree that will trigger
the INSIST reported in [GL #4090]. Due to fixes added since BIND 9.9
an extra insert in needed to ensure node->parent is non NULL.
tests/isc/ht_test.c triggers the following compiler warnings when built
against development versions of cmocka:
In file included from ht_test.c:24:
ht_test.c: In function ‘test_ht_full’:
ht_test.c:68:45: warning: passing argument 2 of ‘_assert_ptr_equal’ makes pointer from integer without a cast [-Wint-conversion]
68 | assert_ptr_equal((void *)i, (uintptr_t)f);
/usr/include/cmocka.h:1513:56: note: in definition of macro ‘assert_ptr_equal’
1513 | #define assert_ptr_equal(a, b) _assert_ptr_equal((a), (b), __FILE__, __LINE__)
| ^
/usr/include/cmocka.h:2907:36: note: expected ‘const void *’ but argument is of type ‘long unsigned int’
2907 | const void *b,
| ~~~~~~~~~~~~^
ht_test.c:163:45: warning: passing argument 2 of ‘_assert_ptr_equal’ makes pointer from integer without a cast [-Wint-conversion]
163 | assert_ptr_equal((void *)i, (uintptr_t)f);
/usr/include/cmocka.h:1513:56: note: in definition of macro ‘assert_ptr_equal’
1513 | #define assert_ptr_equal(a, b) _assert_ptr_equal((a), (b), __FILE__, __LINE__)
| ^
/usr/include/cmocka.h:2907:36: note: expected ‘const void *’ but argument is of type ‘long unsigned int’
2907 | const void *b,
| ~~~~~~~~~~~~^
These are caused by a change to the definitions of pointer assert
functions in cmocka's development branch [1]. Fix by casting the
affected variables to (void *) instead of (uintptr_t).
[1] https://git.cryptomilk.org/projects/cmocka.git/commit/?id=09621179af67535788a67957a910d9f17c975b45
Development versions of cmocka require the intmax_t and uintmax_t types
to be defined by the time the test code includes the <cmocka.h> header.
These types are defined in the <stdint.h> header, which is included by
the <inttypes.h> header, which in turn is already explicitly included by
some of the programs in the tests/ directory. Ensure all programs in
that directory that include the <cmocka.h> header also include the
<inttypes.h> header to future-proof the code while keeping the change
set minimal and the resulting code consistent. Also prevent explicitly
including the <stdint.h> header in those programs as it is included by
the <inttypes.h> header.
Move registration and deregistration of the main thread from
`isc_loopmgr_run()` into `isc__initialize()` / `isc__shutdown()`:
liburcu-qsbr fails an assertion if we try to use it from an
unregistered thread, and we need to be able to use it when the
event loops are not running.
Use `rcu_assign_pointer()` and `rcu_dereference()` in qp-trie
transactions so that they properly mark threads as online. The
RCU-protected pointer is no longer declared atomic because
liburcu does not (yet) use standard C atomics.
Fix the definition of `isc_qsbr_rcu_dereference()` to return
the referenced value, and to call the right function inside
liburcu.
Change the thread sanitizer suppressions to match any variant of
`rcu_*_barrier()`
The teardown jobs are not executed immediately, so we need to delay the
check for ISC_R_SHUTTINGDOWN even more (as the UDP connect is
synchronous, it makes it harder to test it).
Instead of having a global hashtable with a global rwlock for the GLUE
cache, move the glue_list directly into rdatasetheader and use
Userspace-RCU to update the pointer when the glue_list is empty.
Additionally, the cached glue_lists needs to be stored in the RBTDB
version for early cleaning, otherwise the circular dependencies between
nodes and glue_lists will prevent nodes to be ever cleaned up.
A few of the source files in `tests/ns` included `<isc/util.h>`
before `<cmocka.h>`. This could cause compile failures because the
`CMOCKA_NORETURN` macro is defined as `__attribute__((noreturn))`
and `<stdnoreturn.h>` defines `noreturn` as `_Noreturn` which does
not work as a gcc-style attribute.
when the branch implementing mutex_test was rebased and merged,
a rebasing error was missed: the isc_threadresult and isc_threadarg
types no longer exist.
Add simple mutex unit test and mutex benchmark. The benchmark compares
the pthread mutext with isc mutex implementation, so it's mainly useful
when developing a new isc mutex implementation.
Remove the `isc_threadarg_t` and `isc_threadresult_t`
typedefs which were unhelpful disguises for `void *`,
and free the dummy jemalloc allocation sooner.
When liburcu is not installed from a system package, its headers are
not treated as system headers by the compiler, so BIND's -Werror and
other warning options take effect. The liburcu headers have a lot
of inline functions, some of which do not use all their arguments,
which BIND's build treats as an error.
The spinlock is small (atomic_uint_fast32_t at most), lightweight
synchronization primitive and should only be used for short-lived and
most of the time a isc_mutex should be used.
Add a isc_spinlock unit which is either (most of the time) a think
wrapper around pthread_spin API or an efficient shim implementation of
the simple spinlock.
The workers variable might be needed even to tests not using
loopmgr. Split the workers initialization into setup_workers() function
and always call it from the default main loop.
When shutting down TCP sockets, the read callback calling logic was
flawed, it would call either one less callback or one extra. Fix the
logic in the way:
1. When isc_nm_read() has been called but isc_nm_read_stop() hasn't on
the handle, the read callback will be called with ISC_R_CANCELED to
cancel active reading from the socket/handle.
2. When isc_nm_read() has been called and isc_nm_read_stop() has been
called on the on the handle, the read callback will be called with
ISC_R_SHUTTINGDOWN to signal that the dormant (not-reading) socket
is being shut down.
3. The .reading and .recv_read flags are little bit tricky. The
.reading flag indicates if the outer layer is reading the data (that
would be uv_tcp_t for TCP and isc_nmsocket_t (TCP) for TLSStream),
the .recv_read flag indicates whether somebody is interested in the
data read from the socket.
Usually, you would expect that the .reading should be false when
.recv_read is false, but it gets even more tricky with TLSStream as
the TLS protocol might need to read from the socket even when sending
data.
Fix the usage of the .recv_read and .reading flags in the TLSStream
to their true meaning - which mostly consist of using .recv_read
everywhere and then wrapping isc_nm_read() and isc_nm_read_stop()
with the .reading flag.
4. The TLS failed read helper has been modified to resemble the TCP code
as much as possible, clearing and re-setting the .recv_read flag in
the TCP timeout code has been fixed and .recv_read is now cleared
when isc_nm_read_stop() has been called on the streaming socket.
5. The use of Network Manager in the named_controlconf, isccc_ccmsg, and
isc_httpd units have been greatly simplified due to the improved design.
6. More unit tests for TCP and TLS testing the shutdown conditions have
been added.
Co-authored-by: Ondřej Surý <ondrej@isc.org>
Co-authored-by: Artem Boldariev <artem@isc.org>
OpenSSL 3.1.0 uses __attribute__(malloc), conflicting with a redefined
malloc in cmocka.h.
As a workaround, include an OpenSSL header file before including
cmocka.h in the unit tests where OpenSSL is used.
This reverts commit 3d5c7cd46c which
added wrapper around all the unit tests that would run the unit test in
the forked process.
This makes any debugging of the unit tests too hard. Futures attempts
to fix#3980 should add a custom automake test harness (log driver) that
would kill the unit test after configured timeout.
In e185412872, the TCP accept quota code
became broken in a subtle way - the quota would get initialized on the
first accept for the server socket and then deleted from the server
socket, so it would never get applied again.
Properly fixing this required a bigger refactoring of the isc_quota API
code to make it much simpler. The new code decouples the ownership of
the quota and acquiring/releasing the quota limit.
After (during) the refactoring it became more clear that we need to use
the callback from the child side of the accepted connection, and not the
server side.
It should be floor(DNS_NAME_MAXWIRE / 2) + 1 == 128
The mistake was introduced in c6bf51492d because:
* I was refactoring an existing `DNS_MAX_LABELS` defined as 127
* There was a longstanding bug in `dns_name_isvalid()` which
checked the number of labels against 127U instead of 128
* I mistakenly thought `dns_name_isvalid()` was correct and
`dns_name_countlabels()` was incorrect, but the reverse was true.
After this commit, occurrances of `DNS_NAME_MAXLABELS` with value
128 are consistent with the use of 127 or 128 before commit
c6bf51492d except for the mistake in `dns_name_isvalid()`.
This commit adds a test case that checks the MAXLABELS case
in `dns_name_fromtext()` and `dns_name_isvalid()`.
This change makes the zone table lock-free for reads. Previously, the
zone table used a red-black tree, which is not thread safe, so the hot
read path acquired both the per-view mutex and the per-zonetable
rwlock. (The double locking was to fix to cleanup races on shutdown.)
One visible difference is that zones are not necessarily shut down
promptly: it depends on when the qp-trie garbage collector cleans up
the zone table. The `catz` system test checks several times that zones
have been deleted; the test now checks for zones to be removed from
the server configuration, instead of being fully shut down. The catz
test does not churn through enough zones to trigger a gc, so the zones
are not fully detached until the server exits.
After this change, it is still possible to improve the way we handle
changes to the zone table, for instance, batching changes, or better
compaction heuristics.
Revert refcount debug tracing (commit a8b29f0365), there are better
ways to do it.
Use the dns_qpmethods_t typedef where appropriate.
Some stylistic improvements.
Commit 0858514ae8 enriched dns_qp_compact() to give callers more
control over how thoroughly the trie should be compacted.
In the DNS_QPGC_ALL case, if the trie is small it might be compacted
to a new position in the same memory chunk. In this situation it will
still be holding references to old leaf objects which have been
removed from the trie but will not be completely detached until the
chunk containing the references is freed.
This change resets the qp-trie allocator to a fresh chunk before a
DNS_QPGC_ALL compaction, so all the old memory chunks will be
evacuated and old leaf objects can be detached sooner.
This is the first of the "fancy" searches that know how the DNS
namespace maps on to the structure of a qp-trie. For example, it will
find the closest enclosing zone in the zone tree.
The `isc_histosummary_t` functions were written in the early days of
`hg64` and carried over when I brought `hg64` into BIND. They were
intended to be useful for graphing cumulative frequency distributions
and the like, but in practice whatever draws charts is better off with
a raw histogram export. Especially because of the poor performance of
the old functions.
The replacement `isc_histo_quantiles()` function is intended for
providing a few quantile values in BIND's stats channel, when the user
does not want the full histogram. Unlike the old functions, the caller
provides all the query fractions up-front, so that the values can be
found in a single scan instead of a scan per value. The scan is from
larger values to smaller, since larger quantiles are usually more
interesting, so the scan can bail out early.
This is an adaptation of my `hg64` experiments for use in BIND.
As well as renaming everything according to ISC style, I have
written some more extensive tests that ensure the edge cases are
correct and the fenceposts are in the right places.
I have added utility functions for working with precision in terms of
decimal significant figures as well as this code's native binary.
With FIPS mode enabled 'isc_hmac_init_test' and 'isc_hmac_md5_test'
tests of hmac_test and 'isc_md_init_test' and 'isc_md_md5_test' test
of md_test fail.
This is due to leveraging MD5, which is disabled in FIPS mode.
The CI doesn't provide useful forensics when a system test locks
up. Fork the process and kill it with ABRT if it is still running
after 20 minutes. Pass the exit status to the caller.
The isc_time_now() and isc_time_now_hires() were used inconsistently
through the code - either with status check, or without status check,
or via TIME_NOW() macro with RUNTIME_CHECK() on failure.
Refactor the isc_time_now() and isc_time_now_hires() to always fail when
getting current time has failed, and return the isc_time_t value as
return value instead of passing the pointer to result in the argument.
The only place where dns_name_hash() was being used is the old hash
table in the dns_badcache unit. Squash the dns_name_fullhash() and
dns_name_hash() into single dns_name_hash() function that's always
case-insensitive as it doesn't make to do case-sensitive hashing of the
domain names and we were not using this anywhere.
This is a simple replacement using the semantic patch from the previous
commit and as added bonus, one removal of previously undetected unused
variable in named/server.c.
Instead of marking the unused entities with UNUSED(x) macro in the
function body, use a `ISC_ATTR_UNUSED` attribute macro that expans to
C23 [[maybe_unused]] or __attribute__((__unused__)) as fallback.
Previously, isc_job_run() could have been used to run the job on the
current loop and the isc_job_run() would take care of allocating and
deallocating the job. After the change in this MR, the isc_job_run()
is more complicated to use, so we introduce the isc_async_current()
macro to suplement isc_async_run() when we need to run the job on the
current loop.
Change the isc_job_run() to not-make any allocations. The caller must
make sure that it allocates isc_job_t - usually as part of the argument
passed to the callback.
For simple jobs, using isc_async_run() is advised as it allocates its
own separate isc_job_t.
The isc_nm_httpconnect() would succeed even if the netmgr would be
already shuttingdown. This has been fixed and the unit test has been
updated to cope with fact that the handle would be NULL when
isc_nm_httpconnect() returns with an error.
add a public function ns_interface_create() allowing the caller
to set up a listening interface directly without having to set
up listen-on and scan network interfaces.
stop and restart the server in the 'tsiggss' test, in order
to confirm that GSS negotiated TSIG keys are saved and restored
when named loads.
added logging to dns_tsigkey_createfromkey() to indicate whether
a key has been statically configured, generated via GSS negotiation,
or restored from a file.
Completely remove the TKEY Mode 2 (Diffie-Hellman Exchanged Keying) from
BIND 9 (from named, named.conf and all the tools). The TKEY usage is
fringe at best and in all known cases, GSSAPI is being used as it should.
The draft-eastlake-dnsop-rfc2930bis-tkey specifies that:
4.2 Diffie-Hellman Exchanged Keying (Deprecated)
The use of this mode (#2) is NOT RECOMMENDED for the following two
reasons but the specification is still included in Appendix A in case
an implementation is needed for compatibility with old TKEY
implementations. See Section 4.6 on ECDH Exchanged Keying.
The mixing function used does not meet current cryptographic
standards because it uses MD5 [RFC6151].
RSA keys must be excessively long to achieve levels of security
required by current standards.
We might optionally implement Elliptic Curve Diffie-Hellman (ECDH) key
exchange mode 6 if the draft ever reaches the RFC status. Meanwhile the
insecure DH mode needs to be removed.
This implements node reference tracing that passes all the internal
layers from dns_db API (and friends) to increment_reference() and
decrement_reference().
It can be enabled by #defining DNS_DB_NODETRACE in <dns/trace.h> header.
The output then looks like this:
incr:node:check_address_records:rootns.c:409:0x7f67f5a55a40->references = 1
decr:node:check_address_records:rootns.c:449:0x7f67f5a55a40->references = 0
incr:nodelock:check_address_records:rootns.c:409:0x7f67f5a55a40:0x7f68304d7040->references = 1
decr:nodelock:check_address_records:rootns.c:449:0x7f67f5a55a40:0x7f68304d7040->references = 0
There's associated python script to find the missing detach located at:
https://gitlab.isc.org/isc-projects/bind9/-/snippets/1038
Add some qp-trie tracing macros which can be enabled by a
developer. These print a message when a leaf is attached or
detached, indicating which part of the qp-trie implementation
did so. The refcount methods must now return the refcount value
so it can be printed by the trace macros.
The first working multi-threaded qp-trie was stuck with an unpleasant
trade-off:
* Use `isc_rwlock`, which has acceptable write performance, but
terrible read scalability because the qp-trie made all accesses
through a single lock.
* Use `liburcu`, which has great read scalability, but terrible
write performance, because I was relying on `rcu_synchronize()`
which is rather slow. And `liburcu` is LGPL.
To get the best of both worlds, we need our own scalable read side,
which we now have with `isc_qsbr`. And we need to modify the write
side so that it is not blocked by readers.
Better write performance requires an async cleanup function like
`call_rcu()`, instead of the blocking `rcu_synchronize()`. (There
is no blocking cleanup in `isc_qsbr`, because I have concluded
that it would be an attractive nuisance.)
Until now, all my multithreading qp-trie designs have been based
around two versions, read-only and mutable. This is too few to
work with asynchronous cleanup. The bare minimum (as in epoch
based reclamation) is three, but it makes more sense to support an
arbitrary number. Doing multi-version support "properly" makes
fewer assumptions about how safe memory reclamation works, and it
makes snapshots and rollbacks simpler.
To avoid making the memory management even more complicated, I
have introduced a new kind of "packed reader node" to anchor the
root of a version of the trie. This is simpler because it re-uses
the existing chunk lifetime logic - see the discussion under
"packed reader nodes" in `qp_p.h`.
I have also made the chunk lifetime logic simpler. The idea of a
"generation" is gone; instead, chunks are either mutable or
immutable. And the QSBR phase number is used to indicate when a
chunk can be reclaimed.
Instead of the `shared_base` flag (which was basically a one-bit
reference count, with a two version limit) the base array now has a
refcount, which replaces the confusing ad-hoc lifetime logic with
something more familiar and systematic.
Adjust the dns_qp_memusage() and dns_qp_compact() functions
to be more informative and flexible about handling fragmentation.
Avoid wasting space in runt chunks.
Switch from twigs_mutable() to cells_immutable() because that is the
sense we usually want.
Drop the redundant evacuate() function and rename evacuate_twigs() to
evacuate(). Move some chunk test functions closer to their point of
use.
Clarify compact_recursive(). Some small cleanups to comments.
Use isc_time_monotonic() for qp-trie timing stats.
Use #define constants to control debug logging.
Set up DNS name label offsets in dns_qpkey_fromname() so it is easier
to use in cases where the name is not fully hydrated.
The main benchmark is `qpmulti`, which exercizes the qp-trie
transactional API with differing numbers of threads and differing data
sizes, to get some idea of how its performance scales.
The `load-names` benchmark compares the times to populate and query
and the memory used by various BIND data structures: qp-trie, hash
table (chained), hash map (closed), and red-black tree.
The `qp-dump` program is a test utility rather than a benchmark. It
populates a qp-trie and prints it out, either in an ad-hoc text
format, or as input to the graphviz `dot` program.
Randomized testing with intensive consistency and correctness checks
make it much easier to get good coverage and to shake out bugs than
hand-written unit tests for specific cases.
These tests only run in a single thread, but each test transaction
uses both a write/update and a query/snapshot, to ensure that
modifications are not visible to concurrent readers.
This change adds a number of support routines for the unit tests, and
for benchmarks and fuzz tests to be added later. It isn't necessary to
include the support routines in libdns, since they are not needed by
BIND's installed programs. So `libtest` seems like the best place for
them.
The tests themselves verify that dns_qpkey_fromname() behaves as
expected.
`libirs` used to be a reference implementation of `getaddrinfo` and
related modern resolver APIs. It was stripped down in BIND 9.18
leaving only the `irs_resconf` module, which parses
`/etc/resolv.conf`. I have kept its include path and namespace prefix,
so it remains a little fragment of libirs now embedded in libdns.
the 'dispatchmgr' member of the resolver object is used by both
the dns_resolver and dns_request modules, and may in the future
be used by others such as dns_xfrin. it doesn't make sense for it
to live in the resolver object; this commit moves it into dns_view.
as there is no further use of isc_task in BIND, this commit removes
it, along with isc_taskmgr, isc_event, and all other related types.
functions that accepted taskmgr as a parameter have been cleaned up.
as a result of this change, some functions can no longer fail, so
they've been changed to type void, and their callers have been
updated accordingly.
the tasks table has been removed from the statistics channel and
the stats version has been updated. dns_dyndbctx has been changed
to reference the loopmgr instead of taskmgr, and DNS_DYNDB_VERSION
has been udpated as well.
callback events from dns_resolver_createfetch() are now posted
using isc_async_run.
other modules which called the resolver and maintained task/taskmgr
objects for this purpose have been cleaned up.
the rate limter now uses loop callbacks rather than task events.
the API for isc_ratelimiter_enqueue() has been changed; we now pass
in a loop, a callback function and a callback argument, and
receive back a rate limiter event object (isc_rlevent_t). it
is no longer necessary for the caller to allocate the event.
the callback argument needs to include a pointer to the rlevent
object so that it can be freed using isc_rlevent_free(), or by
dequeueing.
The total memory counter had again little or no meaning when we removed
the internal memory allocator. It was just a monotonic counter that
would count add the allocation sizes but never subtracted anything, so
it would be just a "big number".
