As we now setup the logging very early, parsing the default config would
always print warnings about experimental (and possibly deprecated)
options in the default config. This would even mess with commands like
`named -V` and it is also wrong to warn users about using experimental
options in the default config, because they can't do anything about
this. Add CFG_PCTX_NODEPRECATED and CFG_PCTX_NOEXPERIMENTAL options
that we can pass to cfg parser and silence the early warnings caused by
using experimental options in the default config.
OpenSSL has added support for deterministic ECDSA (RFC 6979) with
version 3.2.
Use it by default as derandomization doesn't pose a risk for DNS
usecases and is allowed by FIPS 186-5.
The fcount_incr() was not increasing counter->count when force was set
to true, but fcount_decr() would try to decrease the counter leading to
underflow and assertion failure. Swap the order of the arguments in the
condition, so the !force is evaluated after incrementing the .count.
Since the enable_fips_mode() now resides inside the isc_tls unit, BIND 9
would fail to compile when FIPS mode was enabled as the DST subsystem
logging functions were missing.
Move the crypto library logging functions from the openssl_link unit to
isc_tls unit and enhance it, so it can now be used from both places
keeping the old dst__openssl_toresult* macros alive.
implement, document, and test the 'max-query-restarts' option
which specifies the query restart limit - the number of times
we can follow CNAMEs before terminating resolution.
MAX_RESTARTS is no longer hard-coded; ns_server_setmaxrestarts()
and dns_client_setmaxrestarts() can now be used to modify the
max-restarts value at runtime. in both cases, the default is 11.
the number of steps that can be followed in a CNAME chain
before terminating the lookup has been reduced from 16 to 11.
(this is a hard-coded value, but will be made configurable later.)
there were cases in resolver.c when queries for NS records were
started without passing a pointer to the parent fetch's query counter;
as a result, the max-recursion-queries quota for those queries started
counting from zero, instead of sharing the limit for the parent fetch,
making the quota ineffective in some cases.
Instead of calling dst_lib_init() and dst_lib_destroy() explicitly by
all the programs, create a separate memory context for the DST subsystem
and use the library constructor and destructor to initialize the DST
internals.
When the SSL object was destroyed, it would invalidate all SSL_SESSION
objects including the cached, but not yet used, TLS session objects.
Properly disassociate the SSL object from the SSL_SESSION before we
store it in the TLS session cache, so we can later destroy it without
invalidating the cached TLS sessions.
Co-authored-by: Ondřej Surý <ondrej@isc.org>
Co-authored-by: Artem Boldariev <artem@isc.org>
Co-authored-by: Aram Sargsyan <aram@isc.org>
When TLS connection (TLSstream) connection was accepted, the children
listening socket was not attached to sock->server and thus it could have
been freed before all the accepted connections were actually closed.
In turn, this would cause us to call isc_tls_free() too soon - causing
cascade errors in pending SSL_read_ex() in the accepted connections.
Properly attach and detach the children listening socket when accepting
and closing the server connections.
Since the support for OpenSSL Engines has been removed, we can now also
remove the checks for OPENSSL_API_LEVEL; The OpenSSL 3.x APIs will be
used when compiling with OpenSSL 3.x, and OpenSSL 1.1.xx APIs will be
used only when OpenSSL 1.1.x is used.
The OpenSSL 1.x Engines support has been deprecated in the OpenSSL 3.x
and is going to be removed. Remove the OpenSSL Engine support in favor
of OpenSSL Providers.
When adding glue to the header, we add header to the wait-free stack to
be cleaned up later which sets wfc_node->next to non-NULL value. When
the actual cleaning happens we would only cleanup the .glue_list, but
since the database isn't locked for the time being, the headers could be
reused while cleaning the existing glue entries, which creates a data
race between database versions.
Revert the code back to use per-database-version hashtable where keys
are the node pointers. This allows each database version to have
independent glue cache table that doesn't affect nodes or headers that
could already "belong" to the future database version.
when searching the cache for a node so that we can delete an
rdataset, it is not necessary to set the 'create' flag. if the
node doesn't exist yet, we then we won't be able to delete
anything from it anyway.
dns_difftuple_create() could only return success, so change
its type to void and clean up all the calls to it.
other functions that only returned a result value because of it
have been cleaned up in the same way.
when a priming query is complete, it's currently logged at
level ISC_LOG_DEBUG(1), regardless of success or failure. we
are now raising it to ISC_LOG_NOTICE in the case of failure.
There isn't a realistic reason to ever use e = 4294967297. Fortunately
its codepath wasn't reachable to users and can be safetly removed.
Keep in mind the `dns_key_generate` header comment was outdated. e = 3
hasn't been used since 2006 so there isn't a reason to panic. The
toggle was the public exponents between 65537 and 4294967297.
The previous work in this area was led by the belief that we might be
calling call_rcu() from within call_rcu() callbacks. After carefully
checking all the current callback, it became evident that this is not
the case and the problem isn't enough rcu_barrier() calls, but something
entirely else.
Call the rcu_barrier() just once as that's enough and the multiple
rcu_barrier() calls will not hide the real problem anymore, so we can
find it.
Since the minimal OpenSSL version is now OpenSSL 1.1.1, remove all kind
of OpenSSL shims and checks for functions that are now always present in
the OpenSSL libraries.
Co-authored-by: Ondřej Surý <ondrej@isc.org>
Co-authored-by: Aydın Mercan <aydin@isc.org>
When putting the 48-bit number into a fixed-size buffer that's exactly 6
bytes, the assertion failure would occur as the 48-bit number is
internally represented as 64-bit number and the code was checking if
there is enough space for `sizeof(val)`. This causes assertion failure
when otherwise valid TSIG signature has a bad timing information.
Specify the size of the argument explicitly, so the 48-bit number
doesn't require 8-byte long buffer.
The fcount_incr() was incorrectly skipping the accounting for the
fetches-per-zone if the force argument was set to true. We want to skip
the accounting only when the fetches-per-zone is completely disabled,
but for individual names we need to do the accounting even if we are
forcing the result to be success.
The PTHREAD_MUTEX_ADAPTIVE_NP and PTHREAD_MUTEX_ERRORCHECK_NP are
usually not defines, but enum values, so simple preprocessor check
doesn't work.
Check for PTHREAD_MUTEX_ADAPTIVE_NP from the autoconf AS_COMPILE_IFELSE
block and define HAVE_PTHREAD_MUTEX_ADAPTIVE_NP. This should enable
adaptive mutex on Linux and FreeBSD.
As PTHREAD_MUTEX_ERRORCHECK actually comes from POSIX and Linux glibc
does define it when compatibility macros are being set, we can just use
PTHREAD_MUTEX_ERRORCHECK instead of PTHREAD_MUTEX_ERRORCHECK_NP.
When automatic-interface-scan is disabled, the route socket was still
being opened. Add new API to connect / disconnect from the route socket
only as needed.
Additionally, move the block that disables periodic interface rescans to
a place where it actually have access to the configuration values.
Previously, the values were being checked before the configuration was
loaded.
Decrementing optlen immediately before calling continue is unneccesary
and inconsistent with the rest of dns_message_pseudosectiontoyaml
and dns_message_pseudosectiontotext. Coverity was also reporting
an impossible false positive overflow of optlen (CID 499061).
4176 } else if (optcode == DNS_OPT_CLIENT_TAG) {
4177 uint16_t id;
4178 ADD_STRING(target, "; CLIENT-TAG:");
4179 if (optlen == 2U) {
4180 id = isc_buffer_getuint16(&optbuf);
4181 snprintf(buf, sizeof(buf), " %u\n", id);
4182 ADD_STRING(target, buf);
CID 499061: (#1 of 1): Overflowed constant (INTEGER_OVERFLOW)
overflow_const: Expression optlen, which is equal to 65534, underflows
the type that receives it, an unsigned integer 16 bits wide.
4183 optlen -= 2;
4184 POST(optlen);
4185 continue;
4186 }
4187 } else if (optcode == DNS_OPT_SERVER_TAG) {
There are use cases for which shorter timeout values make sense.
For example if there is a load balancer which sets RD=1 and
forwards queries to a BIND resolver which is then configured to
talk to backend servers which are not visible in the public NS set.
WIth a shorter timeout value the frontend can give back SERVFAIL
early when backends are not available and the ultimate client will
not penalize the BIND-frontend for non-response.
The period between the most significant nibble of the IPv4 address
and the 2.0.0.2.IP6.ARPA suffix was missing resulting in the wrong
name being checked.
In yaml mode we emit a string for each question and record. Certain
names and data could result in invalid yaml being produced. Use single
quote string for all questions and records. This requires that single
quotes get converted to two quotes within the string.
ALPN are defined as 1*255OCTET in RFC 9460. commatxt_fromtext was not
rejecting invalid inputs produces by missing a level of escaping
which where later caught be dns_rdata_fromwire on reception.
These inputs should have been rejected
svcb in svcb 1 1.svcb alpn=\,abc
svcb1 in svcb 1 1.svcb alpn=a\,\,abc
and generated 00 03 61 62 63 and 01 61 00 02 61 62 63 respectively.
The correct inputs to include commas in the alpn requires double
escaping.
svcb in svcb 1 1.svcb alpn=\\,abc
svcb1 in svcb 1 1.svcb alpn=a\\,\\,abc
and generate 04 2C 61 62 63 and 06 61 2C 2C 61 62 63 respectively.
Due to the maximum query restart limitation a long CNAME chain
it is cut after 16 queries but named still returns NOERROR.
Return SERVFAIL instead and the partial answer.
On a 32 bit machine casting to size_t can still lead to an overflow.
Cast to uint64_t. Also detect all possible negative values for
pages and pagesize to silence warning about possible negative value.
39#if defined(_SC_PHYS_PAGES) && defined(_SC_PAGESIZE)
1. tainted_data_return: Called function sysconf(_SC_PHYS_PAGES),
and a possible return value may be less than zero.
2. assign: Assigning: pages = sysconf(_SC_PHYS_PAGES).
40 long pages = sysconf(_SC_PHYS_PAGES);
41 long pagesize = sysconf(_SC_PAGESIZE);
42
3. Condition pages == -1, taking false branch.
4. Condition pagesize == -1, taking false branch.
43 if (pages == -1 || pagesize == -1) {
44 return (0);
45 }
46
5. overflow: The expression (size_t)pages * pagesize might be negative,
but is used in a context that treats it as unsigned.
CID 498034: (#1 of 1): Overflowed return value (INTEGER_OVERFLOW)
6. return_overflow: (size_t)pages * pagesize, which might have underflowed,
is returned from the function.
47 return ((size_t)pages * pagesize);
48#endif /* if defined(_SC_PHYS_PAGES) && defined(_SC_PAGESIZE) */
The key lifetime should no longer be adjusted if the key is being
retired earlier, for example because a manual rollover was started.
This would falsely be seen as a dnssec-policy lifetime reconfiguration,
and would adjust the retire/removed time again.
This also means we should update the status output, and the next
rollover scheduled is now calculated using (retire-active) instead of
key lifetime.
If dnssec-policy is reconfigured and the key lifetime has changed,
update existing keys with the new lifetime and adjust the retire
and removed timing metadata accordingly.
If the key has no lifetime yet, just initialize the lifetime. It
may be that the retire/removed timing metadata has already been set.
Skip keys which goal is not set to omnipresent. These keys are already
in the progress of retiring, or still unused.
This commit ensures that we are not attempting to accept an expired
TCP connection as we are not interested in any data that could have
been accumulated in its internal buffers. Now we just drop them for
good.
When sending fails, the ns__client_request() would not reset the
connection and continue as nothing is happening. This comes from the
model that we don't care about failed UDP sends because datagrams are
unreliable anyway, but it greatly affects TCP connections with
keep-alive.
The worst case scenario is as follows:
1. the 3-way TCP handshake gets completed
2. the libuv calls the "uv_connection_cb" callback
3. the TCP connection gets queue because of the tcp-clients quota
4. the TCP client sends as many DNS messages as the buffers allow
5. the TCP connection gets dropped by the client due to the timeout
6. the TCP connection gets accepted by the server
7. the data already sent by the client gets read
8. all sending fails immediately because the TCP connection is dead
9. we consume all the data in the buffer in a very tight loop
As it doesn't make sense to trying to process more data on the TCP
connection when the sending is failing, drop the connection immediately
on the first sending error.
As ns_query_init() cannot fail now, remove the error paths, especially
in ns__client_setup() where we now don't have to care what to do with
the connection if setting up the client could fail. It couldn't fail
even before, but now it's formal.
Be more aggressive when throttling the reading - when we can't send the
outgoing TCP synchronously with uv_try_write(), we start throttling the
reading immediately instead of waiting for the send buffers to fill up.
This should not affect behaved clients that read the data from the TCP
on the other end.
Instead of outright refusing to add new RR types to the cache, be a bit
smarter:
1. If the new header type is in our priority list, we always add either
positive or negative entry at the beginning of the list.
2. If the new header type is negative entry, and we are over the limit,
we mark it as ancient immediately, so it gets evicted from the cache
as soon as possible.
3. Otherwise add the new header after the priority headers (or at the
head of the list).
4. If we are over the limit, evict the last entry on the normal header
list.
Add HTTPS, SVCB, SRV, PTR, NAPTR, DNSKEY and TXT records to the list of
the priority types that are put at the beginning of the slabheader list
for faster access and to avoid eviction when there are more types than
the max-types-per-name limit.
Due to omission it was possible to un-throttle a TCP connection
previously throttled due to the peer not reading back data we are
sending.
In particular, that affected DoH code, but it could also affect other
transports (the current or future ones) that pause/resume reading
according to its internal state.
Clear qctx->zversion when clearing qctx->zrdataset et al in
lib/ns/query.c:qctx_freedata. The uncleared pointer could lead to
an assertion failure if zone data needed to be re-saved which could
happen with stale data support enabled.
A different solution in the future might be adopted depending
on feedback and other new information, so it makes sense to mark
these options as EXPERIMENTAL until we have more data.
View matching on an incoming query checks the query's signature,
which can be a CPU-heavy task for a SIG(0)-signed message. Implement
an asynchronous mode of the view matching function which uses the
offloaded signature checking facilities, and use it for the incoming
queries.
Add support for using the offload threadpool to perform message
signature verifications. This should allow check SIG(0)-signed
messages without affecting the worker threads.
This is a tiny helper function which is used only once and can be
replaced with two function calls instead. Removing this makes
supporting asynchronous signature checking less complicated.
In order to protect from a malicious DNS client that sends many
queries with a SIG(0)-signed message, add a quota of simultaneously
running SIG(0) checks.
This protection can only help when named is using more than one worker
threads. For example, if named is running with the '-n 4' option, and
'sig0checks-quota 2;' is used, then named will make sure to not use
more than 2 workers for the SIG(0) signature checks in parallel, thus
leaving the other workers to serve the remaining clients which do not
use SIG(0)-signed messages.
That limitation is going to change when SIG(0) signature checks are
offloaded to "slow" threads in a future commit.
The 'sig0checks-quota-exempt' ACL option can be used to exempt certain
clients from the quota requirements using their IP or network addresses.
The 'sig0checks-quota-maxwait-ms' option is used to define a maximum
amount of time for named to wait for a quota to appear. If during that
time no new quota becomes available, named will answer to the client
with DNS_R_REFUSED.
By default we log a rekey failure on debug level. We should probably
change the log level to error. We make an exception for when the zone
is not loaded yet, it often happens at startup that a rekey is
run before the zone is fully loaded.
when signatures were not added because of too many types already
existing at a node, the diff was not being cleaned up; this led to
a memory leak being reported at shutdown.
Previously, the number of RR types for a single owner name was limited
only by the maximum number of the types (64k). As the data structure
that holds the RR types for the database node is just a linked list, and
there are places where we just walk through the whole list (again and
again), adding a large number of RR types for a single owner named with
would slow down processing of such name (database node).
Add a configurable limit to cap the number of the RR types for a single
owner. This is enforced at the database (rbtdb, qpzone, qpcache) level
and configured with new max-types-per-name configuration option that
can be configured globally, per-view and per-zone.
Previously, the number of RRs in the RRSets were internally unlimited.
As the data structure that holds the RRs is just a linked list, and
there are places where we just walk through all of the RRs, adding an
RRSet with huge number of RRs inside would slow down processing of said
RRSets.
Add a configurable limit to cap the number of the RRs in a single RRSet.
This is enforced at the database (rbtdb, qpzone, qpcache) level and
configured with new max-records-per-type configuration option that can
be configured globally, per-view and per-zone.
The changes in this MR prevent the memory used for sending the outgoing
TCP requests to spike so much. That strictly remove the extra need for
own memory context, and thus since we generally prefer simplicity,
remove the extra memory context with own jemalloc arenas just for the
outgoing send buffers.
The single TCP read can create as much as 64k divided by the minimum
size of the DNS message. This can clog the processing thread and trash
the memory allocator because we need to do as much as ~20k allocations in
a single UV loop tick.
Limit the number of the DNS messages processed in a single UV loop tick
to just single DNS message and limit the number of the outstanding DNS
messages back to 23. This effectively limits the number of pipelined
DNS messages to that number (this is the limit we already had before).
As a single thread can process only one TCP send at the time, we don't
really need a memory pool for the TCP buffers, but it's enough to have
a single per-loop (client manager) static buffer that's being used to
assemble the DNS message and then it gets copied into own sending
buffer.
In the future, this should get optimized by exposing the uv_try API
from the network manager, and first try to send the message directly
and allocate the sending buffer only if we need to send the data
asynchronously.
Constantly allocating, reallocating and deallocating 64K TCP send
buffers by 'ns_client' instances takes too much CPU time.
There is an existing mechanism to reuse the ns_clent_t structure
associated with the handle using 'isc_nmhandle_getdata/_setdata'
(see ns_client_request()), but it doesn't work with TCP, because
every time ns_client_request() is called it gets a new handle even
for the same TCP connection, see the comments in
streamdns_on_complete_dnsmessage().
To solve the problem, we introduce an array of available (unused)
TCP buffers stored in ns_clientmgr_t structure so that a 'client'
working via TCP can have a chance to reuse one (if there is one)
instead of allocating a new one every time.
When TCP client would not read the DNS message sent to them, the TCP
sends inside named would accumulate and cause degradation of the
service. Throttle the reading from the TCP socket when we accumulate
enough DNS data to be sent. Currently this is limited in a way that a
single largest possible DNS message can fit into the buffer.
This commit ensures that an HTTP endpoints set reference is stored in
a socket object associated with an HTTP/2 stream instead of
referencing the global set stored inside a listener.
This helps to prevent an issue like follows:
1. BIND is configured to serve DoH clients;
2. A client is connected and one or more HTTP/2 stream is
created. Internal pointers are now pointing to the data on the
associated HTTP endpoints set;
3. BIND is reconfigured - the new endpoints set object is created and
promoted to all listeners;
4. The old pointers to the HTTP endpoints set data are now invalid.
Instead referencing a global object that is updated on
re-configurations we now store a local reference which prevents the
endpoints set objects to go out of scope prematurely.
It was reported that HTTP/2 session might get closed or even deleted
before all async. processing has been completed.
This commit addresses that: now we are avoiding using the object when
we do not need it or specifically check if the pointers used are not
'NULL' and by ensuring that there is at least one reference to the
session object while we are doing incoming data processing.
This commit makes the code more resilient to such issues in the
future.
Replace the ISC_LIST based deadnodes implementation with isc_queue which
is wait-free and we don't have to acquire neither the tree nor node lock
to append nodes to the queue and the cleaning process can also
copy (splice) the list into a local copy without acquiring the list.
Currently, there's little benefit to this as we need to hold those
locks anyway, but in the future as we move to RCU based implementation,
this will be ready.
To align the cleaning with our event loop based model, remove the
hardcoded count for the node locks and use the number of the event loops
instead. This way, each event loop can have its own cleaning as part of
the process. Use uniform random numbers to spread the nodes evenly
between the buckets (instead of hashing the domain name).
Add an isc_queue implementation that hides the gory details of cds_wfcq
into more neat API. The same caveats as with cds_wfcq.
TODO: Add documentation to the API.
When the cache's memory context was in over memory state when the
cache was flushed it resulted in LRU cleaning removing newly entered
data in the new cache straight away until the old cache had been
destroyed enough to take it out of over memory state. When flushing
the cache create a new memory context for the new db to prevent this.
The `axfr_makedb()` didn't set the loop on the newly created database,
effectively killing delayed cleaning on such database. Move the
database creation into dns_zone API that knows all the gory details of
creating new database suitable for the zone.
The rdataslab.c was full of code like this:
length = raw[0] * 256 + raw[1];
and
count2 = *current2++ * 256;
count2 += *current2++;
Refactor code like this into peek_uint16() and get_uint16 macros
to prevent code repetition and possible mistakes when copy and
pasting the same code over and over.
As a side note for an entertainment of a careful reader of the commit
messages: The byte manipulation was changed from multiplication and
addition to shift with or.
The difference in the assembly looks like this:
MUL and ADD:
movzx eax, BYTE PTR [rdi]
movzx edi, BYTE PTR [rdi+1]
sal eax, 8
or edi, eax
SHIFT and OR:
movzx edi, WORD PTR [rdi]
rol di, 8
movzx edi, di
If the result and/or buffer is then being used after the macro call,
there's more differences in favor of the SHIFT+OR solution.
The detach function declaration in `ISC__REFCOUNT_TRACE_DECL` had an
returned an accidental implicit int. While not allowed since C99, it
became an error by default in GCC 14.
`ISC_REFCOUNT_TRACE_IMPL` and `ISC_REFCOUNT_STATIC_TRACE_IMPL` expanded
into the wrong macros, trying to declare it again with the wrong number
of parameters.
- duplicated question
- duplicated answer
- qtype as an answer
- two question types
- question names
- nsec3 bad owner name
- short record
- short question
- mismatching question class
- bad record owner name
- mismatched class in record
- mismatched KEY class
- OPT wrong owner name
- invalid RRSIG "covers" type
- UPDATE malformed delete type
- TSIG wrong class
- TSIG not the last record
there were TSAN error reports because of conflicting uses of
node->dirty and node->nsec, which were in the same qword.
this could be resolved by separating them, but we could also
make them into atomic values and remove some node locking.
The fix_iterator() function had a lot of bugs in it and while fixing
them, the number of corner cases and the complexity of the function
got out of hand. Rewrite the function with the following modifications:
The function now requires that the iterator is pointing to a leaf node.
This removes the cases we have to deal when the iterator was left on a
dead branch.
From the leaf node, pop up the iterator stack until we encounter the
branch where the offset point is before the point where the search key
differs. This will bring us to the right branch, or at the first
unmatched node, in which case we pop up to the parent branch. From
there it is easier to retrieve the predecessor.
Once we are at the right branch, all we have to do is find the right
twig (which is either the twig for the character at the position where
the search key differs, or the previous twig) and walk down from there
to the greatest leaf or, in case there is no good twig, get the
previous twig from the successor and get the greatest leaf from there.
If there is no previous twig to select in this branch, because every
leaf from this branch node is greater than the one we wanted, we need
to pop up the stack again and resume at the parent branch. This is
achieved by calling prevleaf().
Move the fix_iterator out of the loop and only call it when we found
a leaf node. This leaf node may be the wrong leaf node, but fix_iterator
should correct that.
Also, when we don't need to set the iterator, just get any leaf. We
only need to have a leaf for the qpkey_compare and the end result does
not matter if compare was against an ancestor leaf or any leaf below
that point.
An assertion failure would be triggered when sending the TCP data ends
after the TCP reading gets closed. Implement proper reference counting
for the isc_httpd object.
When searching for a requested name in dns_qp_lookup(), we may add
a leaf node to the QP chain, then subsequently determine that the
branch we were on was a dead end. When that happens, the chain can be
left holding a pointer to a node that is *not* an ancestor of the
requested name.
We correct for this by unwinding any chain links with an offset
value greater or equal to that of the node we found.
The code below the if/else construction could only be run if the 'if'
code path was taken. Move the code into the 'if' code block so that
it is more easier to read.
The high-water allows administrators to better tune the recursive
clients limit without having to to poll the statistics channel in high
rates to get this number.
Returning the value allows for better high-water tracking without
running into edge cases like the following:
0. The counter is at value X
1. Increment the value (X+1)
2. The value is decreased multiple times in another threads (X+1-Y)
3. Get the value (X+1-Y)
4. Update-if-greater misses the X+1 value which should have been the
high-water
When parsing a zonefile named-checkzone (and others) could loop
infinitely if a directory was $INCLUDED. Record the error and treat
as EOF when looking for multiple errors.
This was found by Eric Sesterhenn from X41.
In nibble mode if the value to be converted was negative the parser
would loop forever. Process the value as an unsigned int instead
of as an int to prevent sign extension when shifting.
This was found by Eric Sesterhenn from X41.
An RPZ response's SOA record TTL is set to 1 instead of the SOA TTL,
a boolean value is passed on to query_addsoa, which is supposed to be
a TTL value. I don't see what value is appropriate to be used for
overriding, so we will pass UINT32_MAX.
The expireheader() call in the expire_ttl_headers() function
is erroneous as it passes the 'nlocktypep' and 'tlocktypep'
arguments in a wrong order, which then causes an assertion
failure.
Fix the order of the arguments so it corresponds to the function's
prototype.
in QP keys, characters that are not common in DNS names are
encoded as two-octet sequences. this caused a glitch in iterator
positioning when some lookups failed.
consider the case where we're searching for "\009" (represented
in a QP key as {0x03, 0x0c}) and a branch exists for "\000"
(represented as {0x03, 0x03}). we match on the 0x03, and continue
to search down. at the point where we find we have no match,
we need to pop back up to the branch before the 0x03 - which may
be multiple levels up the stack - before we position the iterator.
there were some structure names used in qpcache.c and qpzone.c that
were too similar to each other and could be confusing when debugging.
they have been changed as follows:
in qcache.c:
- changed_t was unused, and has been removed
- search_t -> qpc_search_t
- qpdb_rdatasetiter_t -> qpc_rditer_t
- qpdb_dbiterator_t -> qpc_dbiter_t
in qpzone.c:
- qpdb_changed_t -> qpz_changed_t
- qpdb_changedlist_t -> qpz_changedlist_t
- qpdb_version_t -> qpz_version_t
- qpdb_versionlist_t -> qpz_versionlist_t
- qpdb_search_t -> qpz_search_t
- qpdb_load_t -> qpz_search_t
when calling dns_qp_lookup() from qpcache, instead of passing
'foundname' so that a name would be constructed from the QP key,
we now just use the name field in the node data. this makes
dns_qp_lookup() run faster.
the same optimization has also been added to qpzone.
the documentation for dns_qp_lookup() has been updated to
discuss this performance consideration.
when the cache is over memory, we purge from the LRU list until
we've freed the approximate amount of memory to be added. this
approximation could fail because the memory allocated for nodenames
wasn't being counted.
add a dns_name_size() function so we can look up the size of nodenames,
then add that to the purgesize calculation.
in a cache database, unlike zones, NSEC3 records are stored in
the main tree. it is not necessary to maintain a separate 'nsec3'
tree, nor to have code in the dbiterator implementation to traverse
from one tree to another.
(if we ever implement synth-from-dnssec using NSEC3 records, we'll
need to revert this change. in the meantime, simpler code is better.)
the QP database doesn't support relative names as the RBTDB did, so
there's no need for a 'new_origin' flag or to handle `DNS_R_NEWORIGIN`
result codes.
- remove unneeded struct members and misleading comments.
- remove unused parameters for static functions.
- rename 'find_callback' to 'delegating' for consistency with qpzone;
the find callback mechanism is not used in QP databases.
- change dns_qpdata_t to qpcnode_t (QP cache node), and dns_qpdb_t to
qpcache_t, as these types are only accessed locally.
- also change qpdata_t in qpzone.c to qpznode_t (QP zone node), for
consistency.
- make the refcount declarations for qpcnode_t and qpznode_t static,
using the new ISC_REFCOUNT_STATIC macros.
In qpcache (and rbtdb), there are some functions that acquire
neither the tree lock nor the node lock when calling newref().
In theory, this could lead to a race in which a new reference
is added to a node that was about to be deleted.
We now detect this condition by passing the current tree and node
lock status to newref(). If the node was previously unreferenced
and we don't hold at least one read lock, we will assert.
Because some tests don't have a legtimate handle, provide a temporary
return early that should be fixed and removed before squashing. This
short circuiting is still correct until DoQ/DoH3 support is introduced.
GCC might fail to compile because it expects a return after UNREACHABLE.
It should ideally just work anyway since UNREACHABLE is either a
noreturn or UB (__builtin_unreachable / C23 unreachable).
Either way, it should be optimized almost always so the fallback is
free or basically free anyway when it isn't optimized out.
the previous commit introduced a possible race in getsigningtime()
where the rdataset header could change between being found on the
heap and being bound.
getsigningtime() now looks at the first element of the heap, gathers the
locknum, locks the respective lock, and retrieves the header from the
heap again. If the locknum has changed, it will rinse and repeat.
Theoretically, this could spin forever, but practically, it almost never
will as the heap changes on the zone are very rare.
we simplify matters further by changing the dns_db_getsigningtime()
API call. instead of passing back a bound rdataset, we pass back the
information the caller actually needed: the resigning time, owner name
and type of the rdataset that was first on the heap.
in RBTDB, the heap was used by zone databases for resigning, and
by the cache for TTL-based cache cleaning. the cache use case required
very frequent updates, so there was a separate heap for each of the
node lock buckets.
qpzone is for zones only, so it doesn't need to support the cache
use case; the heap will only be touched when the zone is updated or
incrementally signed. we can simplify the code by using only a single
heap.
fix_iterator() and related functions are quite difficult to read.
perhaps it would be a little clearer if we didn't assign values
to variables that won't subsequently be used, or unnecessarily
pop the stack and then push the same value back onto it.
also, in dns_qp_lookup() we previously called fix_iterator(),
removed the leaf from the top of the iterator stack, and then
added it back on. this would be clearer if we just push the leaf
onto the stack when we need to, but leave the stack alone when
it's already complete.
under some circumstances it was possible for the iterator to
be set to the first leaf in a set of twigs, when it should have
been set to the last.
a unit test has been added to test this scenario. if there is a
a tree containing the following values: {".", "abb.", "abc."}, and
we query for "acb.", previously the iterator would be positioned at
"abb." instead of "abc.".
the tree structure is:
branch (offset 1, ".")
branch (offset 3, ".ab")
leaf (".abb")
leaf (".abc")
we find the branch with offset 3 (indicating that its twigs differ
from each other in the third position of the label, "abB" vs "abC").
but the search key differs from the found keys at position 2
("aC" vs "aB"). we look up the bit value in position 3 of the
search key ("B"), and incorrectly follow it onto the wrong twig
("abB").
to correct for this, we need to check for the case where the search
key is greater than the found key in a position earlier than the
branch offset. if it is, then we need to pop from the current leaf
to its parent, and get the greatest leaf from there.
a further change is needed to ensure that we don't do this twice;
when we've moved to a new leaf and the point of difference between
it and the search key even earlier than before, then we're definitely
at a predecessor node and there's no need to continue the loop.
The previous value of 30 minutes used to cache the ADB names and entries
was quite long. Change the value to 60 seconds for faster recovery
after cached intermittent failure of the remote nameservers.
The algorithm from the previous commit[1] is now used to calculate all
the expiration values through the code (ncache results, cname/dname
targets).
1. ISC_MIN(cur, ISC_MAX(now + ADB_ENTRY_WINDOW, now + rdataset->ttl))
Correct the logic to set the expiration period of expire_{v4,v6} as
follows:
1. If the trust is ultimate (local entry), immediately set the entry as
expired, so the changes to the local zones have immediate effect.
3. If the expiration is already set and smaller than the new value, then
leave the expiration value as it is.
2. Otherwise pick larger of `now + ADB_ENTRY_WINDOW` and `now + TTL` as
the new expiration value.
When ADB entry was created it was set to never expire. If we never
called any of the functions that adjust the expiration, it could linger
in the ADB forever.
Set the expiration (.expires) to now + ADB_ENTRY_WINDOW when creating
the new ADB entry to ensure the ADB entry will always expire.
Previously, only a single controlconf message would be processed from a
single TCP read even if the TCP read buffer contained multiple messages.
Refactor the isccc_ccmsg unit to store the extra buffer in the internal
buffer and use the already read data first before reading from the
network again.
Co-authored-by: Ondřej Surý <ondrej@isc.org>
Co-authored-by: Dominik Thalhammer <dominik@thalhammer.it>
Prepare the statistics channel data in the offloaded worker thread, so
the networking thread is not blocked by the process gathering data from
various data structures. Only the netmgr send is then run on the
networkin thread when all the data is already there.
Protect the access to the trust byte in the ncache data with relaxed
atomic operation to mimick the current behaviour. This will teach
TSAN that the concurrent access is fine.
This prevents TSAN errors with the ncache code where the trust byte
access needs to be protected by a lock. The old code copied the
entire region before determining where the name ended. We now
determine where the name ends then copy just that data and in doing
so avoid reading the trust byte.
The source address field of 'newnotify' was not updated from the
default (0.0.0.0) when the destination address was an IPv6 address.
This resulted in the messages failing to be sent. Set the source
address to :: when the destination address is an IPv6 address.
Since the dns_validator_destroy() function doesn't guarantee that
it destroys the validator, rename it to dns_validator_shutdown()
and require explicit dns_validator_detach() to follow.
Enforce the documented function requirement that the validator must
be completed when the function is called.
Make sure to set val->name to NULL when the function is called,
so that the owner of the validator may destroy the name, even if
the validator is not destroyed immediately. This should be safe,
because the name can be used further only for logging by the
offloaded work callbacks when they detect that the validator is
already canceled/complete, and the logging function has a condition
to use the name only when it is non-NULL.
If val->result is not ISC_R_SUCCESS, a similar message is logged
further down in the function. Remove the redundant log message.
Also remove an unnecessary code comment line.
isc_loop() can now take its place.
This also requires changes to the test harness - instead of running the
setup and teardown outside of th main loop, we now schedule the setup
and teardown to run on the loop (via isc_loop_setup() and
isc_loop_teardown()) - this is needed because the new the isc_loop()
call has to be run on the active event loop, but previously the
isc_loop_current() (and the variants like isc_loop_main()) would work
even outside of the loop because it needed just isc_tid() to work, but
not the full loop (which was mainly true for the main thread).
if we had a method to get the running loop, similar to how
isc_tid() gets the current thread ID, we can simplify loop
and loopmgr initialization.
remove most uses of isc_loop_current() in favor of isc_loop().
in some places where that was the only reason to pass loopmgr,
remove loopmgr from the function parameters.
an assertion could be triggered in the QPDB cache if a DNAME
was found above a queried NS, because the 'foundname' value was
not correctly updated to point to the zone cut.
the same mistake existed in qpzone and has been fixed there as well.
If there are no more previous leaves, it means the queried name
precedes the entire range of names in the database, so we should just
move the iterator one step back and return, instead of continuing our
search for the predecessor.
This is similar to an earlier bug fixed in an earlier commit:
ea9a8cb392
every node of a QP database contains a copy of the nodename,
which is used as the key for the QP-trie. previously, the name
was stored as a dns_fixedname object, which has room for up to
255 characters. we can reduce the space consumed by dynamically
allocating a dns_name object that's just long enough for the name
to be stored.
The dns_qpiter_next() was called without checking the return value. If
we cannot move the iterator forward, there is no use in calling the
step() function.
/lib/dns/qpzone.c: 2804 in activeempty()
2798 * of the name we were searching for. Step the iterator
2799 * forward, then step() will continue forward until it
2800 * finds a node with active data. If that node is a
2801 * subdomain of the one we were looking for, then we're
2802 * at an active empty nonterminal node.
2803 */
>>> CID 487882: Error handling issues (CHECKED_RETURN)
>>> Calling "dns_qpiter_next" without checking return value (as is done elsewhere 26 out of 27 times).
2804 dns_qpiter_next(it, NULL, NULL, NULL);
2805 return (step(search, it, FORWARD, next) &&
2806 dns_name_issubdomain(next, current));
2807 }
Be stricter in durations that are accepted. Basically we accept ISO 8601
formats, but fail to detect garbage after the integers in such strings.
For example, 'P7.5D' will be treated as 7 days. Pass 'endptr' to
'strtoll' and check if the endptr is at the correct suffix.
dns_db_addrdataset() enforces a requirement that version can only
be NULL for a cache database. code that checks for zone semantics
and version == NULL can never be reached.
qpzone does not support cache semantics, so dns_db_addrdataset(),
_deleterdataset() and _subtractrdataset() can't be run with
version == NULL; there's no need to check for it.
we can also clean up free_qpdb() a bit since current_version
is always non-NULL.
The Depends relation refers to types of rollovers in which a certain
record type is going to be swapped. Specifically, the Depends relation
says there should be no dependency on the predecessor key (the set
Dep(x, T) must be empty).
But if the key is phased out (all its states are in HIDDEN), there is
no longer a dependency. Since the relationship is still maintained
(Predecessor and Successor metadata), the keymgr_dep function still
returned true. In other words, the set Dep(x, T) is not considered
empty.
This slows down key rollovers, only retiring keys when the successor
key has been fully propagated.
When there is a secure chain of trust with a KSK that is not actively
signing the DNSKEY RRset, the code for validating the DNSKEY RRset
against the DS RRset could potentially skip DS records, thinking the
chain of trust is broken while there is a valid DS with corresponding
DNSKEY record present.
This is because we pass the result ISC_R_NOMORE on when we are done
checking for signatures, but then treat it as "no more DS records".
Chaning the return value to something else (DNS_R_NOVALIDSIG seems the
most appropriate here) fixes the issue.
the code in qpdb.c was previously shared by qp-cachedb.c and
qp-zonedb.c. since qp-zonedb.c no longer exists, it's not necessary
to keep these separate any longer. the two files have been merged,
and functions that were previously globally accessible have been
changed to static and renamed.
now that "qpzone" databases are available for use in zones, we no
longer need to retain the zone semantics in the "qp" database.
all zone-specific code has been removed from QPDB, and "configure
--with-zonedb" once again takes two values, rbt and qp.
some database API methods that are never used with a cache have
been removed from qpdb.c and qp-cachedb.c; these include newversion,
closeversion, subtractrdataset, and nodefullname.
because dns_qpmulti_commit() can be time consuming, it's inefficient
to open and commit a qpmulti transaction for each rdataset being loaded
into a database. we can improve load time by opening a qpmulti
transaction before adding a group of rdatasets and then committing it
afterward.
this commit adds 'setup' and 'commit' functions to dns_rdatacallbacks_t,
which can be called before and after the loops in which 'add' is
called in dns_master_load() and axfr_apply().
when copying the non-dnssec records in receive_secure_db(),
use DNS_DB_NONSEC3 so we don't accidentally create nodes in
the main tree for NSEC3 records. this was a long-standing error
in the code, but was harmless in the RBTDB.
QP database node data is not reference counted the same way RBT nodes
were: in the RBT, node->references could be zero if the node was in the
tree but was not in use by any caller, whereas in the QP trie, the
database itself uses reference counting of nodes internally.
this caused some subtle errors. in RBTDB, when the newref() function is
called and the node reference count was zero, the node lock reference
counter would also be incremented. in the QP trie, this can never
happen - because as long as the node is in the database its reference
count cannot be zero - and so the node lock reference counter was never
incremented.
this has been addressed by maintaining a separate "erefs" counter for
external references to the node. this is the same approach used in the
"qpdb-lite" database in commit e91fbd8dea.
while troubleshooting this issue, some compile errors were discovered
when building with DNS_DB_NODETRACE; those have also been fixed.
use the dns_qpmulti-based "qpzone" by default throughout BIND,
instead of the existing dns_qp-based "qp", when creating zone
databases. (cache databases still use "qp".)
the "--with-zonedb" option has been updated in configure.ac to permit
the use of both "qp" and "qpzone" databases.
in zone.c there was a test that prevented any database type other than
"qp" from hosting an RPZ. this was outdated, and has been removed.
previously, an RCU critical section was held open for the duration
of a snapshot. this should not be necessary, as the snapshot makes
local copies of QP trie metadata, and it causes problems when a
DB iterator is held open between two loop events. we now call
rcu_read_unlock() after setting up the snapshot.
finish importing the database API methods from RBTDB to qpzone:
issecure, nodecount, getnsec3parameters, findnsec3node, setsigningtime,
getsigningtime, getsize, setgluecachestats, locknode, unlocknode, and
addglue.
add database API methods needed to apply updates to an existing zone
database (newversion, addrdataset, subtractrdataset and deleterdataset).
it is now possible to apply journals to zone databases after loading, so
named-checkzone -J works correctly.
add database API method implementations needed to iterate and dump
a qpzone database to a file (createiterator, allrdatasets and
attachversion, plus dbiterator and rdatasetiter methods).
named-checkzone -D can now dump the contents of most zones,
but zone cuts are not correctly detected.
add database API methods needed for loading rdatasets into memory
(currentversion, beginload, endload), plus the methods used by
zone_postload() for zone consistency checks (getoriginnode, find,
findnode, findrdataset, attachnode, detachnode, deletedata).
the QP trie doesn't support the find callback mechanism available
in dns_rbt_findnode() which allows examination of intermediate nodes
while searching, so the detection of wildcard and delegation nodes
is now done by scanning QP chains after calling dns_qp_lookup().
Note that the lookup in previous_closest_nsec() cannot return
ISC_R_NOTFOUND. In RBTDB, we checked for this return value and
ovewrote the result with ISC_R_NOMORE if it occurred. In the
qpzone implementation, we insist that this return value cannot happen.
dns_qp_lookup() would only return ISC_R_NOTFOUND if we asked for a
name outside the zone's authoritative domain, and we never do that
when looking up a predecessor NSEC record.
named-checkzone is now able to load a zone and check it for errors,
but cannot dump it.
The dns_cache_flush() drops the old database and creates a new one, but
it forgets to pass the loop that runs the node pruning and cleaning
the rbtdb when flushing it next time. This causes the cleaning to skip
cleaning the parent nodes (with .down == NULL) leading to increased
memory usage over time until the database is unable to keep up and just
stays overmem all the time.
Reconstruct the variant of the prune_tree() parent cleaning to consider
all elibible parents in a single loop as we were doing before all the
changes that led to this commit.
Update code comments so that they more precisely describe what the
relevant bits of code actually do.
by default, QPDB is the database used by named and all tools and
unit tests. the old default of RBTDB can now be restored by using
"configure --with-zonedb=rbt --with-cachedb=rbt".
some tests have been fixed so they will work correctly with either
database.
CHANGES and release notes have been updated to reflect this change.
When running resolver benchmark pipeline, a crash occurred:
https://gitlab.isc.org/isc-projects/bind9-shotgun-ci/-/pipelines/163946
In the code we are doing a lookup, it fails (meaning there is no node
with lookup name), we create the node and insert it and it fails.
But dns_qp_insert can only return ISC_R_SUCCESS or ISC_R_EXISTS.
So it must have been inserted in between. This is a race condition bug.
The first lookup only requires a write lock and if the lookup failed
the lock gets upgraded to a write lock and we insert the missing data.
To fix the race condition bug, we need to do a lookup again after we
have upgraded the lock to make sure it wasn't inserted in the mean
time.
the dyndb test requires a mechanism to retrieve the name associated
with a database node, and since the database no longer uses RBT for
its underlying storage, dns_rbt_fullnamefromnode() doesn't work.
addressed this by adding dns_db_nodefullname() to the database API.
If the iterator is paused, the tree is unlocked and may change.
In an RBT tree it's always possible to resume iteration as long
as a valid node pointer was still held, but now that the underlying
database structure is a QP trie, the iterator needs to be initialized
based on the existing structure of the trie or it will return
inconsistent results. We now call dns_qp_lookup() to reinitialize
the QP iterator whenever dbiterator_next() or dbiterator_prev() is
called on a paused iterator.
QP database node data is not reference counted the same way RBT nodes
were: in the RBT, node->references could be zero if the node was in the
tree but was not in use by any caller, whereas in the QP trie, the
database itself uses reference counting of nodes internally.
this caused some subtle errors. in RBTDB, when the newref() function is
called and the node reference count was zero, the node lock reference
counter would also be incremented. in the QP trie, this can never
happen - because as long as the node is in the database its reference
count cannot be zero - and so the node lock reference counter was never
incremented.
reference counting will probably need to be refactored in more detail
later; the node lock reference count may not be needed at all. but
for now, as a temporary measure, we add a third reference counter,
'erefs' (external references), to the dns_qpdata structure. this is
counted separately from the main reference counter, and should match
the node reference count as it would have been in RBTDB.
this change revealed a number of places where the node reference counter
was being incremented on behalf of a caller without newref() being
called; those were cleaned up as well.
This is an adaptation of commit 3dd686261d2c4bcd15a96ebfea10baffa277732b
Fix reference counting: unreference nodes that are succesfully inserted
in the tree, detach created nodes, and cleanup the interior data in
dns_qpdata_destroy().
The name will be stored inside the node now so we can just copy it.
These are leftovers, most of the namefromnode code has been replaced
already in previous commits.
The qp approach pulled apart the chain and iterator into two separate
things. Replace the rbtnodechain with qpchain and qpiter. Most of the
times we are interested in the iterator only, the rbtnodechain was
mainly used as an an iterator to get the previous and next name in the
DNS canonical order.
Since dns_qpiter_prev() and dns_qpiter_next() store the name, origin,
and node in the provided parameters, often there is no need to call
a current() function anymore.
Getting the first or last item from the iterator is done by
re-initializing the iterator and then call dns_qpiter_next() or
dns_qpiter_prev() respectively.
The dbiterator no longer needs to maintain a chain, only an iterator.
All dns_qp_lookup() calls assume it is okay to find empty data, so
we don't need to do anything special for the DNS_RBTFIND_EMPTYDATA.
You can pass a callback function to dns_rbt_findnode(), something that
qp does not support. Instead, call the function afterwards. This has
the drawback that we do more lookup work if there was a zonecut.
With dns_qp_lookup() we also don't pass any options. In this case,
when DNS_RBTFIND_NOEXACT was set, we adapt the result after the lookup.
Replace dns_rbt_deletenode calls with dns_qp_deletename. For removing
the name from the nsec tree, we no longer first have to find it: we can
just remove the key (retrieved by name).
Replace dns_rbt_addnode calls with dns_qp_insert. With QP, it sometimes
makes more sense to first lookup the name and see if there is an
existing node (rather than create new data, insert, find out a node
already exists, and destroy the data again). This is done with
dns_qp_getname(), which is more lightweight than dns_qp_lookup(),
and we are only interested in if there is already a leaf node for this
name or not.
replace the string "rbt" throughout BIND with "qp" so that
qpdb databases will be used by default instead of rbtdb.
rbtdb databases can still be used by specifying "database rbt;"
in a zone statement.
- Copy rbtdb.c, rbt-zonedb.c and rbt-cachedb.c to qp-*.
- Added qpmethods.
- Added a new structure dns_qpdata that will replace dns_rbtnode.
- Replaced normal, nsec, and nsec3 dns_rbt trees with dns_qp tries.
- Replaced dns_rbt_create() calls with dns_qp_create().
- Replaced the dns_rbt_destroy() call with dns_qp_destroy().
- Create a dns_qpdata struct and create/destroy methods.
This commit will not build.
[GL #3709] reordered the dns_rdataset_disassociate call to after
the dns_resolver_createfetch call resulting in qctx->nsrrset still
being associated when dns_resolver_createfetch is called in
resume_dslookup (7e4e125e). Revert that part of the change and add
comments as to why the multiple dns_rdataset_disassociate calls are
where they are.
The TCP dispatch connected callbacks could be called synchronously which
in turn could destroy xfrin before we return from dns_xfrin_create().
Delay the calling the callback called from tcp_dispatch_connect() by
calling it always asynchronously.
Instead of getting the loop from the zone every time, attach the xfrin
directly to the loop. This also allows to remove the extra safety tid
checks from the dns_xfrin unit.
It was discovered that the TTL-based cleaning could build up
a significant backlog of the rdataset headers during the periods where
the top of the TTL heap isn't expired yet. Make the TTL-based cleaning
more aggressive by cleaning more headers from the heap when we are
adding new header into the RBTDB.
It was discovered that an expired header could sit on top of the heap
a little longer than desireable. Remove expired headers (headers with
rdh_ttl set to 0) from the heap completely, so they don't block the next
TTL-based cleaning.
Instead of juggling with node locks in a cycle, cleanup the node we are
just pruning and send any the parent that's also subject to the pruning
to the prune tree via normal way (e.g. enqueue pruning on the parent).
This simplifies the code and also spreads the pruning load across more
event loop ticks which is better for lock contention as less things run
in a tight loop.
The log message for commit 24381cc36d
explained:
In some older BIND 9 branches, the extra queuing overhead eliminated by
this change could be remotely exploited to cause excessive memory use.
Due to architectural shift, this branch is not vulnerable to that issue,
but applying the fix to the latter is nevertheless deemed prudent for
consistency and to make the code future-proof.
However, it turned out that having a single queue for the nodes to be
pruned increased lock contention to a level where cleaning up nodes from
the RBTDB took too long, causing the amount of memory used by the cache
to grow indefinitely over time.
This commit reverts the change to the pruning mechanism introduced by
commit 24381cc36d as BIND branches newer
than 9.16 were not affected by the excessive event queueing overhead
issue mentioned in the log message for the above commit.
dns__cacherbt_expireheader can unlink / free header_prev underneath
it. Use ISC_LIST_TAIL after calling dns__cacherbt_expireheader
instead to get the next pointer to be processed.
This commit ensures that worker threads are not sleeping (by using
select()) when '-T transferslowly/transferstuck' test options are
used. This commit converts synchronous implementation of the code into
an asynchronous one based on timers.
The resolver's code was not ready to failures when trying to establish
a connection via TCP-based transports (e.g. when creating TLS contexts
before establishing a TLS connection).
This commit fixes that.
The warnings (see below) seem to be false-positives. Address them
by adding runtime checks.
resolver.c:1627:10: warning: Access to field 'tid' results in a dereference of a null pointer (loaded from variable 'fctx') [core.NullDereference]
1627 | REQUIRE(fctx->tid == isc_tid());
| ^~~~~~~~~
../../lib/isc/include/isc/util.h:332:34: note: expanded from macro 'REQUIRE'
332 | #define REQUIRE(e) ISC_REQUIRE(e)
| ^
../../lib/isc/include/isc/assertions.h:45:11: note: expanded from macro 'ISC_REQUIRE'
45 | ((void)((cond) || \
| ^~~~
resolver.c:10335:6: warning: Access to field 'depth' results in a dereference of a null pointer (loaded from variable 'fctx') [core.NullDereference]
10335 | if (fctx->depth > depth) {
| ^~~~~~~~~~~
2 warnings generated.
- the DNS_DB_NSEC3ONLY and DNS_DB_NONSEC3 flags are mutually
exclusive; it never made sense to set both at the same time.
to enforce this, it is now a fatal error to do so. the
dbiterator implementation has been cleaned up to remove
code that treated the two as independent: if nonsec3 is
true, we can be certain nsec3only is false, and vice versa.
- previously, iterating a database backwards omitted
NSEC3 records even if DNS_DB_NONSEC3 had not been set. this
has been corrected.
- when an iterator reaches the origin node of the NSEC3 tree, we
need to skip over it and go to the next node in the sequence.
the NSEC3 origin node is there for housekeeping purposes and
never contains data.
- the dbiterator_test unit test has been expanded, several
incorrect expectations have been fixed. (for example, the
expected number of iterations has been reduced by one; we were
previously counting the NSEC3 origin node and we should not
have been doing so.)
- create_node() in rbt.c cannot fail
- the dns_rbt_*name() functions, which are wrappers around
dns_rbt_[add|find|delete]node(), were never used except in tests.
this change isn't really necessary since RBT is likely to go away
eventually anyway. but keeping the API as simple as possible while it
persists is a good thing, and may reduce confusion while QPDB is being
developed from RBTDB code.
these values pertain to whether a node is in the main, nsec, or nsec3
tree of an RBTDB. they need to be moved to a more generic location so
they can also be used by QPDB.
(this is in db.h rather than db_p.h because rbt.c needs access to it.
technically, that's a layer violation, but it's a long-existing one;
refactoring to get rid of it would be a large hassle, and eventually
we expect to remove rbt.c anyway.)
when the QPDB is implemented, we will need to have both qpdb_p.h and
rbtdb_p.h. in order to prevent name collisions or code duplication,
this commit adds a generic private header file, db_p.h, containing
structures and macros that will be used by both databases.
some functions and structs have been renamed to more specifically refer
to the RBT database, in order to avoid namespace collision with similar
things that will be needed by the QPDB later.
refactor the wildcard matching code to make it a bit easier to
understand, in hopes that it will reduce the difficulty of converting
from RBTDB to QPDB later.
there are also some minor optimizations: previously, after stepping
backward to find the predecessor, we stepped back foward *from* the
predecessor to find the successor. we now reset the rbtnode chain to
its original starting point before stepping forward; this eliminates
some unnecessary processing. and, if neither predecessor nor successor
is found, we return early rather than carrying on with an unnecessary
effort to match labels.
Coverity detected that address->type.sa was too small when copying
a struct sockaddr_sin6, use the alterative union element
address->type.sin6 instead.
The case insensitive matching in isc_ht was basically completely broken
as only the hashvalue computation was case insensitive, but the key
comparison was always case sensitive.
The isc_log_t contains a isc_logconfig_t that is swapped, dereferenced
or accessed its fields through a mutex. Instead of protecting it with a
rwlock, use RCU.
When shutting down the whole server, the reading could stop and detach
from controlconnection before sending is done. If send callback then
detaches from the last controlconnection handle, the ccmsg would be
invalidated after the send callback and thus we must not access ccmsg
after calling the send_cb().
We need to stop reading when calling isc_ccmsg_disconnect() as the
reading handle doesn't have to be last because sending might be in
progress. After that, we can safely remove .reading member because the
reading would not be called after the disconnect has been called.
The ccmsg_senddone() should also not call the recv callback if the
sending failed, that's the job of the caller's send callback - in fact
it already does that, so the code in ccmsg_senddone() was superfluous.
To reduce memory pressure, we can add light per-loop (netmgr worker)
memory pools for isc_nmsocket_t structures. This will help in
situations where there's a lot of churn creating and destroying the
nmsockets.
Embedding isc_nmsocket_h2_t directly inside isc_nmsocket_t had increased
the size of isc_nmsocket_t to 1840 bytes. Making the isc_nmsocket_h2_t
to be a pointer to the structure and allocated on demand allows us to
reduce the size to 1208 bytes. While there are still some possible
reductions in the isc_nmsocket_t (embedded tlsstream, streamdns
structures), this was the far biggest drop in the memory usage.
The uv_req union member of struct isc__nm_uvreq contained libuv request
types that we don't use. Turns out that uv_getnameinfo_t is 1000 bytes
big and unnecessarily enlarged the whole structure. Remove all the
unused members from the uv_req union.
After removing sockaddr_unix from isc_sockaddr, we can also remove
sockaddr_storage and reduce the isc_sockaddr size from 152 bytes to just
48 bytes needed to hold IPv6 addresses.
As it was pointed out, the alignas() can't be used on objects larger
than `max_align_t` otherwise the compiler might miscompile the code to
use auto-vectorization on unaligned memory.
As we were only using alignas() as a way to prevent false memory
sharing, we can use manual padding in the affected structures.
Stop the cname_and_other_data processing if we already know that the
result is true. Also, we know that CNAME will be placed in the priority
headers, so we can stop looking for CNAME if we haven't found CNAME and
we are past the priority headers.
Mark the infrastructure RRTypes as "priority" types and place them at
the beginning of the rdataslab header data graph. The non-priority
types either go right after the priority types (if any).
The cachedb was missing piece of code (already found in zonedb) that
would make lookups in the slabheaders to miss the RRSIGs for CNAME if
the order of CNAME and RRSIG(CNAME) was reversed in the node->data.
With _exit() instead of exit() in place, we don't need
isc__tls_setfatalmode() mechanism as the atexit() calls will not be
executed including OpenSSL atexit hooks.
Instead of crude 5x rcu_barrier() call in the isc__mem_destroy(), change
the mechanism to call rcu_barrier() until the memory use and references
stops decreasing. This should deal with any number of nested call_rcu()
levels.
Additionally, don't destroy the contextslock if the list of the contexts
isn't empty. Destroying the lock could make the late threads crash.
Expose the newly added 'first refresh' flag in the information
provided by the 'rndc staus' command, by showing the number of
zones, which are not yet fully ready, and their first refresh
is pending or is in-progress.
Add a new zone flag to indicate that a secondary type zone is
not yet fully ready, and a first time refresh is pending or is
in progress.
Expose this new flag in the statistics channel's "Incoming Zone
Transfers" section.
Using the 'dnssec-validation yes' option now requires an explicitly
confgiured 'trust-anchors' statement (or 'managed-keys' or
'trusted-keys', both deprecated).
Use the (1 << N) form for defining the flags, in order to avoid
errors like the one fixed in the previous commit.
Also convert the definitions to an enum, as done in some of our
recent refactoring work.
The DNS_GETDB_STALEFIRST flag is defined as 0x0C, which is the
combination of the DNS_GETDB_PARTIAL (0x04) and the
DNS_GETDB_IGNOREACL (0x08) flags (0x04 | 0x08 == 0x0C) , which is
an obvious error.
All the flags should be power of two, so they don't interfere with
each other. Fix the DNS_GETDB_STALEFIRST flag by setting it to 0x10.
Instead of running all the cryptographic validation in a tight loop,
spread it out into multiple event loop "ticks", but moving every single
validation into own isc_async_run() asynchronous event. Move the
cryptographic operations - both verification and DNSKEY selection - to
the offloaded threads (isc_work_enqueue), this further limits the time
we spend doing expensive operations on the event loops that should be
fast.
Limit the impact of invalid or malicious RRSets that contain crafted
records causing the dns_validator to do many validations per single
fetch by adding a cap on the maximum number of validations and maximum
number of validation failures that can happen before the resolving
fails.
Checking the DS at the parent only happens if dns_zone_getdnsseckeys()
returns success. However, if this function somehow fails, it can also
prevent the keymgr from running.
Before adding the check DS functionality, the keymgr should only run
if 'dns_dnssec_findmatchingkeys()' did not return an error (either
ISC_R_SUCCESS or ISC_R_NOTFOUND). After this change the correct
result code is used again.
the 'low', 'high' and 'discount' parameters to 'fetch-quota-param'
are meant to be ratios with values between zero and one, but higher
values can be assigned. this could potentially lead to an assertion
in maybe_adjust_quota().
The fix for CVE-2023-4408 introduced a regression in the message
parser, which could cause a crash if an rdata type that can only
occur in the question was found in another section.
Use 'dns__message_putassociatedrdataset()' instead of
'dns__message_puttemprdataset()', because after calling the
'dns_rdatalist_tordataset()' function earlier the 'rdataset'
is associated.
the fix for CVE-2023-4408 introduced a regression in the message
parser, which could cause a crash if duplicate rdatasets were found
in the question section. this commit ensures that rdatasets are
correctly disassociated and freed when this occurs.
This is now the default way to implement attaching to/detaching from
a pointer.
Also update cfg_keystore_fromconfig() to allow NULL value for the
keystore pointer. In most cases we detach it immediately after the
function call.
Add a default key-directory parameter to the function that can
be returned if there is no keystore, or if the keystore directory
is NULL (the latter is also true for the built-in keystore).
The name "uri" was considered to be too generic and could potentially
clash with a future URI configuration option. Renamed to "pkcs11-uri".
Note that this option name was also preferred over "pkcs11uri", the
dash is considered to be the more clearer form.
When using the same PKCS#11 URI for a zone that uses different
DNSSEC policies, the PKCS#11 label could collide, i.e. the same
label could be used for different keys. Add the policy name to
the label to make it more unique.
Also, the zone name could contain characters that are interpreted
as special characters when parsing the PKCS#11 URI string. Mangle
the zone name through 'dns_name_tofilenametext()' to make it
PKCS#11 safe.
Move the creation to a separate function for clarity.
Furthermore, add a log message whenever a PKCS#11 object has been
successfully created.
The internal keymgr used 'isc_dir_open(&dir)' and 'isc_dir_close(&dir)',
but was not using the variable 'dir`, other than checking if the
directory can be opened. Errors like these will be be caught already
in the dst_api function calls.
The pkcs11-provider did not yet support getting X/Y coordinates
on newly generated EC PKEY keys, thus we attempted to get the
key from the label after it was generated in the keystore.
This has been fixed in:
https://github.com/latchset/pkcs11-provider/pull/293
Thus now we should be able to use the generated key structure
immediately.
If not set, the created keys allows signing plus decrypt which is bad
practice. Setting the key usage explicitly will generate keys that
allow only signing.
The pkcs11-provider has changed to take a PKCS#11 URI instead of an
object identifier. Change the BIND 9 code accordingly to pass through
the label instead of just the object identifier.
See: https://github.com/latchset/pkcs11-provider/pull/284
Move dns_dnssec_findzonekeys from the dnssec.{c,h} source code to
zone.{c,h} (the header file already commented that this should be done
inside dns_zone_t).
Alter the function in such a way, that keys are searched for in the
key stores if a 'dnssec-policy' (kasp) is attached to the zone,
otherwise keep using the zone's key-directory.
When writing key files to disk, use the internally stored directory.
Add an access function 'dst_key_directory()'.
Most calls to keymgr functions no longer need to provide the
key-directory value. Only 'dns_keymgr_run' still needs access to
the zone's key-directory in case the key-store is set to the built-in
key-directory.
Refactor dns_dnssec_findmatchingkeys and dns_dnssec_keylistfromrdataset
to take into account the key store directories in case the zone is using
dnssec-policy (kasp). Add 'kasp' and 'keystores' parameters.
This requires the keystorelist to be stored inside the zone structure.
The calls to these functions in the DNSSEC tools can use NULL as the
kasp value, as dnssec-signzone does not (yet) support dnssec-policy,
and key collision is checked inside the directory where it is created.
If there is a keystore configured with a PKCS#11 URI, zones that
are using a dnssec-policy that uses such a keystore should create keys
via the PKCS#11 interface. Those keys are generally stored inside an
HSM.
Some changes to the code are required, to store the engine reference
into the keystore.
When creating the kasp structure, instead of storing the name of the
key store on keys, store a reference to the key store object instead.
This requires to build the keystore list prior to creating the kasp
structures, in the dnssec tools, the check code and the server code.
We will create a builtin keystore called "key-directory" which means
use the zone's key-directory as the key store.
The check code changes, because now the keystore is looked up before
creating the kasp structure (and if the keystore is not found, this
is an error). Instead of looking up the keystore after all
'dnssec-policy' clauses have been read.
Similar to key-directory, check for zones in different views and
different key and signing policies. Zones must be using different key
directories to store key files on disk.
Now that a key directory can be linked with a dnssec-policy key, the
'keydirexist' checking needs to be reshuffled.
Add tests for bad configuration examples, named-checkconf should catch
those. Also add test cases for a mix of key-directory and key-store
directory.
Similar to key-directory, check if the key-store directory exists and
if it is an actual directory.
This commit fixes an accidental test bug in checkconf where if
the "warn key-dir" test failed, the result was ignored.
Add checkconf check to ensure that the used key-store in the keys
section exists. Error if that is not the case. We also don't allow
the special keyword 'key-directory' as that is internally used to
signal that the zone's key-directory should be used.
Add code for configuring keystore objects. Add this to the "kaspconf"
code, as it is related to 'dnssec-policy' and it is too small to create
a separate file for it.
Add new configuration for setting key stores. The new 'key-store'
statement allows users to configure key store backends. These can be
of type 'file' (that works the same as 'key-directory') or of type
'pkcs11'. In the latter case, keys should be stored in a HSM that is
accessible through a PKCS#11 interface.
Keys configured within 'dnssec-policy' can now also use the 'key-store'
option to set a specific key store.
Update the checkconf test to accomodate for the new configuration.
cmocka.h and jemalloc.h/malloc_np.h has conflicting macro definitions.
While fixing them with push_macro for only malloc is done below, we only
need the non-standard mallocx interface which is easy to just define by
ourselves.
Because we don't use jemalloc functions directly, but only via the
libisc library, the dynamic linker might pull the jemalloc library
too late when memory has been already allocated via standard libc
allocator.
Add a workaround round isc_mem_create() that makes the dynamic linker
to pull jemalloc earlier than libc.
This commit improves TLS messages framing by avoiding an extra call to
SSL_write_ex(). Before that we would use an extra SSL_write_ex() call
to pass DNS message length to OpenSSL. That could create an extra TLS
frame, increasing number of bytes sent due to frame header and
padding.
This commit fixes that by making the code pass both DNS message length
and data at once, just like old TLS code did.
It should improve compatibility with some buggy clients that expect
both DNS message length and data to be in one TLS frame.
Older TLS DNS code worked like this, too.
The statistics channel does not expose the current number of TCP clients
connected, only the highwater. Therefore, users did not have an easy
means to collect statistics about TCP clients served over time. This
information could only be measured as a seperate mechanism via rndc by
looking at the TCP quota filled.
In order to expose the exact current count of connected TCP clients
(tracked by the "tcp-clients" quota) as a statistics counter, an
extra, dedicated Network Manager callback would need to be
implemented for that purpose (a counterpart of ns__client_tcpconn()
that would be run when a TCP connection is torn down), which is
inefficient. Instead, track the number of currently-connected TCP
clients separately for IPv4 and IPv6, as Network Manager statistics.
This commit removes wrong INSIST() condition as the assumption that if
'csock->recv_cb != NULL' iff 'csock->statichandle != NULL' is wrong.
There is no direct relation between 'csock->statichandle' and
'csock->recv_cb', as 'csock->statichandle' gets set when allocating a
handle regardless of 'csock->recv_cb' not being NULL, as it is
possible to attach to the handle without starting a read operation (at
the very least, it is correct to start writing before reading).
That condition made `cipher-suites` system test fail with crash on
some platforms in FIPS mode (namely, Oracle Linux 9) despite not being
related to FIPS at all.
The older version of the code was reporting that listeners are going
to be of the same type after reconfiguration when switching from DoT
to HTTPS listener, making BIND abort its executions.
That was happening due to the flaw in logic due to which the code
could consider a current listener and a configuration for the new one
to be of the same type (DoT) even when the new listener entry is
explicitly marked as HTTP.
The checks for PROXY in between the configuration were masking that
behaviour, but when porting it to 9.18 (when there is no PROXY
support), the behaviour was exposed.
Now the code mirrors the logic in 'interface_setup()' closely (as it
was meant to).
When processing UPDATE request DNSKEY, CDNSKEY and CDS record that
are managed by named are filtered out. The log message has been
updated to report the actual type rather that just DNSKEY.
This commit ensures that listeners are recreated on reconfiguration in
the case when their type changes (or when PROXY protocol type changes,
too).
Previously, if a "listen-on" statement was modified to represent a
different transport, BIND would not pick-up the change on
reconfiguration if listener type changes (e.g. DoH -> DoT) for a given
interface address and port combination. This commit fixes that by
recreating the listener.
Initially, that worked for most of the new transports as we would
recreate listeners on each reconfiguration for DoH and DoT. But at
some point we changed that in such a way that listeners were not
recreated to avoid rebinding a port as on some platforms only root can
do that for port numbers <1000, making some ports binding possible
only on start-up. We chose to asynchronously update listener socket
settings (like TLS contexts, HTTP settings) instead.
Now, we both avoid recreating the sockets if unnecessary and recreate
listeners when listener type changes.
This commits adds low-level wrappers on top of
'SSL_CTX_set_ciphersuites()'. These are going to be a foundation
behind the 'cipher-suites' option of the 'tls' statement.
The conn_shutdown() function is called whenever a control channel
connection is supposed to be closed, e.g. after a response to the client
is sent or when named is being shut down. That function calls
isccc_ccmsg_invalidate(), which resets the magic number in the structure
holding the messages exchanged over a given control channel connection
(isccc_ccmsg_t). The expectation here is that all operations related to
the given control channel connection will have been completed by the
time the connection needs to be shut down.
However, if named shutdown is initiated while a control channel message
is still in flight, some netmgr callbacks might still be pending when
conn_shutdown() is called and isccc_ccmsg_t invalidated. This causes
the REQUIRE assertion checking the magic number in ccmsg_senddone() to
fail when the latter function is eventually called, resulting in a
crash.
Fix by splitting up isccc_ccmsg_invalidate() into two separate
functions:
- isccc_ccmsg_disconnect(), which initiates TCP connection shutdown,
- isccc_ccmsg_invalidate(), which cleans up magic number and buffer,
and then:
- replacing all existing uses of isccc_ccmsg_invalidate() with calls
to isccc_ccmsg_disconnect(),
- only calling isccc_ccmsg_invalidate() when all netmgr callbacks are
guaranteed to have been run.
Adjust function comments accordingly.
Instead of issuing a separate isc_async_run() call for every RBTDB node
that triggers tree pruning, maintain a list of nodes from which tree
pruning can be started from and only issue an isc_async_run() call if
pruning has not yet been triggered by another RBTDB node.
In some older BIND 9 branches, the extra queuing overhead eliminated by
this change could be remotely exploited to cause excessive memory use.
Due to architectural shift, this branch is not vulnerable to that issue,
but applying the fix to the latter is nevertheless deemed prudent for
consistency and to make the code future-proof.
If we are in the process of looking for the A records as part of
dns64 processing and the server-stale timeout triggers, redo the
dns64 changes that had been made to the orignal qctx.
The wrong result value was being saved for resumption with
nxdomain-redirect when performing the fetch. This lead to an assert
when checking that RFC 1918 reverse queries where not leaking to
the global internet.
When parsing messages use a hashmap instead of a linear search to reduce
the amount of work done in findname when there's more than one name in
the section.
There are two hashmaps:
1) hashmap for owner names - that's constructed for each section when we
hit the second name in the section and destroyed right after parsing
that section;
2) per-name hashmap - for each name in the section, we construct a new
hashmap for that name if there are more than one rdataset for that
particular name.
1) Restart the process with a write lock if we discover an expired key
while holding the read lock.
2) Move incrementing the key reference inside the lock block of code.
isc/atomic.h and its defined macros should be preferred over
stdatomic.h and explicit atomic operations.
Fix the redundant stdatomic.h header in histo.c found by the introduced
check.
If the DNSKEY, CDNSKEY or CDS RRset had different TTLs then the
filtering of these RRset resulted in dns_diff_apply failing with
"not exact". Identify tuple pairs that are just TTL changes and
allow them through the filter.
Clang Static Analyzer is unable to grasp that when dns_rbt_addnode()
returns ISC_R_EXISTS, it always sets the pointer passed to it via its
'nodep' parameter to a non-NULL value. Add an extra safety check in the
conditional expression used in dns_rbt_addname() to silence that
warning.
it was possible for fix_iterator() to get stuck in a loop while
trying to find the predecessor of a missing node. this has been
fixed and a regression test has been added.
the fix_iterator() function moves an iterator so that it points
to the predecessor of the searched-for name when that name doesn't
exist in the database. the tests only checked the correctness of
the top of the stack, however, and missed some cases where interior
branches in the stack could be missing or duplicated. in these
cases, the iterator would produce inconsistent results when walked.
the predecessors test case in qp_test has been updated to walk
each iterator to the end and ensure that the expected number of
nodes are found.
When building NSEC3 chains update the NSEC3PARAM TTL to match
the SOA minimum. Delete all records using the old TTL then
re-add them using the new TTL.
Just remove the key from consideration as it is being removed.
The old code could leak a key reference as dst_free_key was not
called every time we continued. This simplification will address
this as well.
Check the tid and cancel the request immediately or pass it to the
appropriate loop for processing. Call request->cb directly from
req_sendevent as it is now always called with the correct tid.
The xfrin_end() function is run when a zone transfer is finished or
canceled. One of the actions it takes for incremental transfers (IXFR)
is calling dns_journal_destroy() on the zone journal structure that is
stored in the relevant zone transfer context (xfr->ixfr.journal). That
immediately invalidates that structure as it is not reference-counted.
However, since the changes present in the IXFR stream are applied to the
journal asynchronously (via isc_work_enqueue()), it is possible that
some zone changes may still be in the process of being written to the
journal by the time xfrin_end() destroys the relevant structure. Such a
scenario leads to crashes.
Fix by not destroying the zone journal structure until the entire zone
transfer context is destroyed. xfrin_destroy() already conditionally
calls dns_journal_destroy() and when the former is called, all
asynchronous work for a given zone transfer process is guaranteed to be
complete.
Multiple zones should be able to read the same key and signing policy
at the same time. Since writing the kasp lock only happens during
reconfiguration, and the complete kasp list is being replaced, there
is actually no need for a lock. Reference counting ensures that a kasp
structure is not destroyed when still being attached to one or more
zones.
This significantly improves the load configuration time.
When kasp support was added 'inception' was used as a proxy for
'now' and resulted in signatures not being generated or the wrong
signatures being generated. 'inception' is the time to be set
in the signatures being generated and is usually in the past to
allow for clock skew. 'now' determines what keys are to be used
for signing.
Remove the CFG_CLAUSEFLAG_EXPERIMENTAL flag from the
"trust-anchor-telemetry" statement as the behavior of the latter has not
been changed since its initial implementation and there are currently no
plans to do so. This silences a relevant log message that was emitted
even when the feature was explicitly disabled.
Each function queuing a do_nsfetch() call using isc_async_run() is
expected to increase the given zone's internal reference count
(zone->irefs), which is then correspondingly decreased in either
do_nsfetch() itself (when the dns_resolver_createfetch() fails) or in
nsfetch_done() (when recursion is finished).
However, do_nsfetch() can also return early if either the zone itself or
the relevant view's resolver object is being shut down. In that case,
do_nsfetch() simply returns without decreasing the internal reference
count for the zone. This leaves a dangling zone reference around, which
leads to hangs during named shutdown.
Fix by executing the same cleanup code for early returns from
do_nsfetch() as for a failed dns_resolver_createfetch() call in that
function as the reference count will not be decreased in nsfetch_done()
in any of these cases.
The atomic_init() function makes sense to use with structure's
members when creating a new instance of a strucutre. In other
places, use atomic store operations instead, in order to avoid
data races.
Move the code to find the predecessor into one function, as it is shares
quite some similarities: In both cases we first need to find the
immediate predecessor/successor, then we need to find the immediate
predecessor if the iterator is not already pointing at it.
This one is similar to the bug when searching for a key, reaching a
dead-end branch that doesn't match, because the branch offset point
is after the point where the search key differs.
This fixes the case where we are multiple levels deep. In other
words, we had a more-than-one matches *after* the point where the
search key differs.
For example, consider the following qp-trie:
branch: "[e]", "[m]":
- leaf: "a.b.c.d.e"
- branch: "moo[g]", "moo[k]", "moo[n]":
- leaf: "moog"
- branch: "mook[e]", "mook[o]"
- leaf: "mooker"
- leaf: "mooko"
- leaf: "moon"
If searching for a key "monky", we would reach the branch with
twigs "moo[k]" and "moo[n]". The key matches on the 'k' on offset=4,
and reaches the branch with twigs "mook[e]" and "mook[o]". This time
we cannot find a twig that matches our key at offset=5, there is no
twig for 'y'. The closest name we found was "mooker".
Note that on a branch it can't detect it is on a dead branch because the
key is not encapsulated in a branch node.
In the previous code we considered "mooker" to be the successor of
"monky" and so we needed to the predecessor of "mooker" to find the
predecessor for "monky". However, since the search key alread differed
before entering this branch, this is not enough. We would be left with
"moog" as the predecessor of "monky", while in this example "a.b.c.d.e"
is the actual predecessor.
Instead, we need to go up a level, find the predecessor and check
again if we are on the right branch, and repeat the process until we
are.
Unit tests to cover the scenario are now added.
There was yet another edge case in which an iterator could be
positioned at the wrong node after dns_qp_lookup(). When searching for
a key, it's possible to reach a leaf that matches at the given offset,
but because the offset point is *after* the point where the search key
differs from the leaf's contents, we are now at the wrong leaf.
In other words, the bug fixed the previous commit for dead-end branches
must also be applied on matched leaves.
For example, if searching for the key "monpop", we could reach a branch
containing "moop" and "moor". the branch offset point - i.e., the point
after which the branch's leaves differ from each other - is the
fourth character ("p" or "r"). The search key matches the fourth
character "p", and takes that twig to the next node (which can be
a branch for names starting with "moop", or could be a leaf node for
"moop").
The old code failed to detect this condition, and would have
incorrectly left the iterator pointing at some successor, and not
at the predecessor of the "moop".
To find the right predecessor in this case, we need to get to the
previous branch and get the previous from there.
This has been fixed and the unit test now includes several new
scenarios for testing search names that match and unmatch on the
offset but have a different character before the offset.
As we are in overmem state we want to free more memory than we are
adding so we need to add in an allowance for the rbtnodes that may
have been added and the names stored with them. There is the node
for the owner name and a possible ENT node if there was a node split.
Only cleanup headers that are less than equal to the rbt's last_used
time. Adjust the rbt's last_used time when the target cleaning was
not achieved to the oldest value of the remaining set of headers.
When updating delegating NS and glue records last_used was not being
updated when it should have been.
When adding zero TTL records to the tail of the LRU lists set
last_used to rbtdb->last_used + 1 rather than now. This appoximately
preserves the lists LRU order.
these options control default timing of retries in the resolver
for experimental purposes; they are not known to useful in production
environments. they will be removed in the future; for now, we
only log a warning if they are used.
there was another edge case in which an iterator could be positioned at
the wrong node after dns_qp_lookup(). when searching for a key, it's
possible to reach a dead-end branch that doesn't match, because the
branch offset point is *after* the point where the search key differs
from the branch's contents.
for example, if searching for the key "mop", we could reach a branch
containing "moon" and "moor". the branch offset point - i.e., the
point after which the branch's leaves differ from each other - is the
fourth character ("n" or "r"). however, both leaves differ from the
search key at position *three* ("o" or "p"). the old code failed to
detect this condition, and would have incorrectly left the iterator
pointing at some lower value and not at "moor".
this has been fixed and the unit test now includes this scenario.
in some cases it was possible for the iterator to be positioned in the
wrong place by dns_qp_lookup(). previously, when a leaf node was found
which matched the search key at its parent branch's offset point, but
did not match after that point, the code incorrectly assumed the leaf
it had found was a successor to the searched-for name, and stepped the
iterator back to find a predecessor. however, it was possible for the
non-matching leaf to be the predecessor, in which case stepping the
iterator back was wrong.
(for example: a branch contains "aba" and "abcd", and we are searching
for "abcde". we step down to the twig matching the letter "c" in
position 3. "abcd" is the predecessor of "abcde", so the iterator is
already correctly positioned, but because the twig was an exact match,
we would have moved it back one step to "aba".)
this previously went unnoticed due to a mistake in the qp_test unit
test, which had the wrong expected result for the test case that should
have detected the error. both the code and the test have been fixed.
the 'predecessor' argument to dns_qp_lookup() turns out not to
be sufficient for our needs: the predecessor node in a QP database
could have become "empty" (for the current version) because of an
update or because cache data expired, and in that case the caller
would have to iterate more than one step back to find the predecessor
node that it needs.
it may also be necessary for a caller to iterate forward, in
order to determine whether a node has any children.
for both of these reasons, we now replace the 'predecessor'
argument with an 'iter' argument. if set, this points to memory
with enough space for a dns_qpiter object.
when an exact match is found by the lookup, the iterator will be
pointing to the matching node. if not, it will be pointing to the
lexical predecessor of the nae that was searched for.
a dns_qpiter_current() method has been added for examining
the current value of the iterator without moving it in either
direction.
During initialisation or reconfiguration, it is possible that multiple
threads are trying to create a TLS context and associated data (like
TLS certs store) concurrently. In some cases, a thread might be too
late to add newly created data to the TLS contexts cache, in which
case it needs to be discarded. In the code that handles that case, it
was not taken into account that, in some cases, the TLS certs store
could not have been created or should not be deleted, as it is being
managed by the TLS contexts cache already. Deleting the store in such
cases might lead to crashes.
This commit fixes the issue.
The main intention of PROXY protocol is to pass endpoints information
to a back-end server (in our case - BIND). That means that it is a
valid way to spoof endpoints information, as the addresses and ports
extracted from PROXYv2 headers, from the point of view of BIND, are
used instead of the real connection addresses.
Of course, an ability to easily spoof endpoints information can be
considered a security issue when used uncontrollably. To resolve that,
we introduce 'allow-proxy' and 'allow-proxy-on' ACL options. These are
the only ACL options in BIND that work with real PROXY connections
addresses, allowing a DNS server operator to specify from what clients
and on which interfaces he or she is willing to accept PROXY
headers. By default, for security reasons we do not allow to accept
them.
This commit modifies TLS Stream and DNS-over-HTTPS transports so that
they do not use the "sock->iface" and "sock->peer" of the lower level
transport directly.
That did not cause any problems before, as things worked as expected,
but with the introduction of PROXYv2 support we use handles to store
the information in both PROXY Stream and UDP Proxy
transports. Therefore, in order to propagate the information (like
addresses), extracted from PROXYv2 headers, from the lower level
transports to the higher-level ones, we need to get that information
from the lower-level handles rather than sockets. That means that we
should get the peer and interface addresses using the intended
APIs ("isc_nmhandle_peeraddr()" and "isc_nmhandle_localaddr()").
This commit extends "listen-on" statement with "proxy" options that
allows one to enable PROXYv2 support on a dedicated listener. It can
have the following values:
- "plain" to send PROXYv2 headers without encryption, even in the case
of encrypted transports.
- "encrypted" to send PROXYv2 headers encrypted right after the TLS
handshake.
Add the new isc__nm_dump_active_manager() function that can be used
for debugging purposes: it dumps all active sockets withing the
network manager instance.
This commit adds a new transport that supports PROXYv2 over UDP. It is
built on top of PROXYv2 handling code (just like PROXY Stream). It
works by processing and stripping the PROXYv2 headers at the beginning
of a datagram (when accepting a datagram) or by placing a PROXYv2
header to the beginning of an outgoing datagram.
The transport is built in such a way that incoming datagrams are being
handled with minimal memory allocations and copying.
In the previous versions of the NM, detecting the case when worker is
shutting down was not that important and actual status code did not
matter much. However, that might be not the case all the time.
This commit makes necessary modifications to the code.
This commit makes it possible to use PROXY Stream not only over TCP,
but also over TLS. That is, now PROXY Stream can work in two modes as
far as TLS is involved:
1. PROXY over (plain) TCP - PROXYv2 headers are sent unencrypted before
TLS handshake messages. That is the main mode as described in the
PROXY protocol specification (as it is clearly stated there), and most
of the software expects PROXYv2 support to be implemented that
way (e.g. HAProxy);
2. PROXY over (encrypted) TLS - PROXYv2 headers are sent after the TLS
handshake has happened. For example, this mode is being used (only ?)
by "dnsdist". As far as I can see, that is, in fact, a deviation from
the spec, but I can certainly see how PROXYv2 could end up being
implemented this way elsewhere.
This commit modifies TLS Stream to make it possible to use over PROXY
Stream. That is required to add PROVYv2 support into TLS-based
transports (DNS over HTTP, DNS over TLS).
This commit adds a new stream-based transport with an interface
compatible with TCP. The transport is built on top of TCP transport
and the new PROXYv2 handling code. Despite being built on top of TCP,
it can be easily extended to work on top of any TCP-like stream-based
transport. The intention of having this transport is to add PROXYv2
support into all existing stream-based DNS transport (DNS over TCP,
DNS over TLS, DNS over HTTP) by making the work on top of this new
transport.
The idea behind the transport is simple after accepting the connection
or connecting to a remote server it enters PROXYv2 handling mode: that
is, it either attempts to read (when accepting the connection) or send
(when establishing a connection) a PROXYv2 header. After that it works
like a mere wrapper on top of the underlying stream-based
transport (TCP).
This commit adds a set of utilities for dealing with PROXYv2 headers,
both parsing and generating them. The code has no dependencies from
the networking code and is (for the most part) a "separate library".
The part responsible for handling incoming PROXYv2 headers is
structured as a state machine which accepts data as input and calls a
callback to notify the upper-level code about the data processing
status.
Such a design, among other things, makes it easy to write a thorough
unit test suite for that, as there are fewer dependencies as well as
will not stand in the way of any changes in the networking code.
BIND 9 will now treat the response as insecure when processing NSEC3
records with iterations larger than 50.
Earlier, we limited the number of iterations to 150 (in #2445).
RFC 9276 says: Because there has been a large growth of open (public)
DNSSEC validating resolvers that are subject to compute resource
constraints when handling requests from anonymous clients, this
document recommends that validating resolvers reduce their iteration
count limits over time. Specifically, validating resolver operators and
validating resolver software implementers are encouraged to continue
evaluating NSEC3 iteration count deployment trends and lower their
acceptable iteration limits over time.
After evaluation, we decided that the next major BIND release should
lower the maximum allowed NSEC3 iterations to 50, which should be
fine for 99,87% of the domain names.
With shared name memory pools (f5af981831)
the message needs to be destroyed before the view is detached which
in turn detaches the resolver which checks that all resources have
been returned.
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.
when transferring in a non-inline-signing secondary for the first time,
we previously never set the value of zone->loadtime, so it remained
zero. this caused a test failure in the statschannel system test,
and that test case was temporarily disabled. the value is now set
correctly and the test case has been reinstated.
The maximum DNS message size is 65535 octets. Check that the buffer
being passed to dns_message_renderbegin does not exceed this as the
compression code assumes that all offsets are no bigger than this.
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.
Please see the 998765fea5 commit for
the description of the original issue. The commit had fixed the
logic error, but it was reintroduced again later with the
a1afa31a5a commit, where the check of
the 'db_registered' flag was removed in dns__catz_update_cb(). The
check was removed, because the registration function was made
idempotent, so double registration is not an issue, but the check
also prevented from unneeded registration, on which the original
fix relied.
This commit just removes the update callback registration code from
the dns__catz_update_cb() function instead of bringing back the check,
because after code flow analysis, it is now clear that it's not required
at all. The "call onupdate() artificially" comment (which was mentioned
by the removed code) is speaking about the dns_catz_dbupdate_callback()
function, which is called by server.c on (re)configuration, and that
function already takes care of update callback's registration since the
998765fea5 commit was applied, so there
is no need to do that here again.
Concurrent threads can access a hashmap for reading by creating and
then destroying an iterator, in which case the integer number of the
active iterators is increased or decreased from different threads,
introducing a data race. Use atomic operations to protect the variable.
The AES algorithm for DNS cookies was being kept for legacy reasons,
and it can be safely removed in the next major release. Mark is as
deprecated, so the `named-checkconf` prints a warning when in use.
The 'dns_tsigkeyring_t' structure has a read/write lock to protect
its 'keys' member, which is a 'isc_hashmap_t' pointer and needs to
be protected.
The dns_tsigkeyring_dump() function, however, doesn't use the lock,
which can introduce a race with another thread, if the other thread
tries to modify the hashmap.
Add a read lock around the code, which iterates over the hashmap.
EXPERIMENT: when DNS_DB_GLUEOK is set, dns_view_find() will now return
glue if it is found it a local zone database, without checking to see
if a better answer has been cached previously.
If a child zone is served by the same servers as a parent zone and
a NS query is made for the zone name then the addresses of the
nameservers are returned in the additional section are tagged as
trust additional.
Commit 4db150437e incorrectly removed the
call to isc_mem_setwater() from dns_cache_setcachesize(). The water()
function is a no-op, but we still need to set high- and low-water marks
in the memory context, otherwise overmem conditions will not be
detected.
When serve-stale is enabled and recursive resolution fails, the fallback
to lookup stale data always happens in the cache database. Any
authoritative data is ignored, and only information learned through
recursive resolution is examined.
If there is data in the cache that could lead to an answer, and this can
be just the root delegation, the resolver will iterate further, getting
closer to the answer that can be found by recursing down the root, and
eventually puts the final response in the cache.
Change the fallback to serve-stale to use 'query_getdb()', that finds
out the best matching database for the given query.
Increasing the initial and freemax sizes for dns_message memory pools
restores the root zone performance. The former sizes were suited for
per-dns_message memory pools and we need to bump the sizes up for
per-thread memory pools.
The lock-file configuration (both from configuration file and -X
argument to named) has better alternatives nowadays. Modern process
supervisor should be used to ensure that a single named process is
running on a given configuration.
Alternatively, it's possible to wrap the named with flock(1).
This is first step in removing the lock-file configuration option, it
marks both the `lock-file` configuration directive and -X option to
named as deprecated.
The `.reader` member of dns_qpmulti_t was accessed without RCU
protection; reader_open() calls rcu_dereference() on it, and this
call needs to be inside an RCU critical section.
A similar problem was identified in the dns_qpmulti_snapshot() - the
RCU critical section was completely missing.
These are relicts of the isc_qsbr - in the QSBR mode the rcu_read_lock()
and rcu_read_unlock() are no-ops and whole event loop is a critical section.
Do a light refactoring and cleanups that replaces common list walking
patterns with ISC_LIST_FOREACH macros and split some nested loops into
separate static functions to reduce the nesting depth.
Add complementary macros to ISC_LIST_FOREACH(_SAFE) that walk the lists
in reverse.
* ISC_LIST_FOREACH_REV(list, elt, link) - walk the static list from
tail to head
* ISC_LIST_FOREACH_REV_SAFE(list, elt, link, next) - walk the list
from tail to head in a manner that's safe against list member
deletions
There was a lot of internal code looking like this:
INSIST(dns_rdataset_isassociated(rdataset));
dns_rdataset_disassociated(rdataset)
isc_mempool_put(msg->rdspool, rdataset);
Deduplicate the code into local dns__message_puttemprdataset() routine,
and drop the INSIST() which is checked in dns_rdataset_disassociate().
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.
The xfrin_recv_done() was accessing xfr->result where we stored the
result of the offloaded work from a thread that could receive data while
processing the transfer on the offloaded thread.
Completely remove the offloaded result from the dns_xfrin_t structure
and keep it local for *xfr_apply() and *xfr_apply_done() as the failure
is already recorded in .shutdown_result and we now that the processing
has failed because .shuttingdown has been already set.
The dns__catz_update_cb() does not expect that 'catzs->zones'
can become NULL during shutdown.
Add similar checks in the dns__catz_update_cb() and dns_catz_zone_get()
functions to protect from such a case. Also add an INSIST in the
dns_catz_zone_add() function to explicitly state that such a case
is not expected there, because that function is called only during a
reconfiguration.
To reduce the amount of log spam when root servers change their
addresses keep a table of upcoming changes by expected date and time
and suppress reporting differences for them until then.
Add initial entry for B.ROOT-SERVERS.NET, Nov 27, 2023.
Instead of processing received data synchronously, store the incoming
differences in the list and process them asynchronously when we need to
commit the data into the database and/or journal.
Instead of locking the struct dns_xfrin members that get accessed from
the statistics, convert those into atomic types and use atomic accesses
to prevent ThreadSanitizer from blowing up.
In fact, even the atomic operations are not really needed here, because
all writes are done from a single thread and we don't really require
consistency from the statistics. It's easier to use atomics here, but
it is slightly confusing as it suggests there might be multithreaded
accesses to those variables while in fact, the only off-thread access
happens when collecting the statistics.
The ixfr_putdata() and axfr_putdata() had a logic to apply dns_diff when
the number of pending tuples went over 100. Since we are going to
offload the XFR data processing, we don't need to do that anymore.
OpenSSL 1.1 has already reached end-of-life and since we are
experiencing a weird memory leak in the mirror system test on just
Ubuntu 20.04 (Focal) with OpenSSL 1.1, we disable the legacy code for
enabling memory contexts for OpenSSL < 3.0.0 in this commit.
Drop timeout before resending a UDP request from 15 seconds to 5
seconds and add 1 second to the total time to allow for the reply
to the third request to arrive. This will speed up the time it
takes for named to recover from a lost packet when refreshing a
zone and for it to determine that a primary is down.
Update the function 'set_resigntime()' so that raw versions of
inline-signing zones are not scheduled to be resigned.
Also update the check in the same function for zone is dynamic, there
exists a function 'dns_zone_isdynamic()' that does a similar thing
and is more complete.
Also in 'zone_postload()' check whether the zone is not the raw
version of an inline-signing zone, preventing calculating the next
resign time.
The dns_aclenv_t contains two dns_acl_t - localhost and localnets that
can be swapped with a different ACLs as we configure BIND 9. Instead of
protecting those two pointers with heavyweight read-write lock, use RCU
mechanism to dereference and swap the pointers.
In units that support detailed reference tracing via ISC_REFCOUNT
macros, we were doing:
/* Define to 1 for detailed reference tracing */
#undef <unit>_TRACE
This would prevent using -D<unit>_TRACE=1 in the CFLAGS.
Convert the above mentioned snippet with just a comment how to enable
the detailed reference tracing:
/* Add -D<unit>_TRACE=1 to CFLAGS for detailed reference tracing */
It was possible to reach add_link() without visiting an
intermediate node first, and the check for a duplicate entry
could then cause a crash.
Credit to OSS-Fuzz for discovering this error.
There was a microoptimization for smoothing srtt with bitshifts. Revert
the code to use * 98 / 100, it doesn't really make that difference on
modern CPUs, for comparison here:
muldiv:
imul eax, edi, 98
imul rax, rax, 1374389535
shr rax, 37
ret
shift:
mov eax, edi
sal eax, 9
sub eax, edi
shr eax, 9
ret
If factor == DNS_ADB_RTTADJAGE and addr->entry->lastage == now we would
load value into new_srtt and then immediatelly store it back which
triggers the synchronization between threads using .srtt values.
Use atomics on couple of ADB entry members (.srtt, .flags, .expires, and
.lastage) to remove ADB entry locking from couple of hot spots. The
most prominent place is copy_namehook_lists() that gets called under ADB
name lock and if the namehook list is long it acquires-releases quite a
few ADB entry locks. Changing those ADB entry members to atomics
allowed us to new_adbaddrinfo() not require locked ADB entry and since
adbentry_overquota() already used atomics and handling lame information
was dropped in the previous commit, we could not make the
copy_namehook_lists() lockless.
The other hotspot is dns_adb_adjustsrtt() and dns_adb_agesrtt() that can
now use atomics because .srtt is already atomic_uint.
And the last place that could now use atomics is dns_adb_changeflags().
Keeping the information about lame server in the ADB was done in !322 to
fix following security issue:
[CVE-2021-25219] Disable "lame-ttl" cache
The handling of the lame servers needs to be redesigned and it is not
going to be enabled any time soon, and the current code is just dead
code that takes up space, code and stands in the way of making ADB work
faster.
Remove all the internals needed for handling the lame servers in the ADB
for now. It might get reintroduced later if and when we redesign ADB.
now that we're using qpmulti for the summary database, we
no longer need to hold search_lock for it. we do still need
it for the radix tree and the trigger counts.
now that we have the QP chain mechanism, we can convert the
RPZ summary database to use a QP trie instead of an RBT.
also revised comments throughout the file accordingly, and
incidentally cleaned up calls to new_node(), which can no
longer fail.
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.)
All these pointers are guaranteed to be non-NULL.
Additionally, update a comment to remove obviously outdated
information about the function's requirements.
Move the block on the error path, where the link is checked, to a place
where it makes sense, to avoid accessing an unitialized link when
jumping to the 'cleanup_query' label from 4 different places. The link
is initialized only after those jumps happen.
In addition, initilize the link when creating the object, to avoid
similar errors.
