Since the removal of NS_QUERY_QCTX_DESTROYED hook, there is no need for
the `qctx->detach_client` object anymore, as this was designed to tell
the plugin whether the client object is about to be, or is already,
freed from memory. This is not needed anymore, as NS_QUERY_RESET is
called _always_ when the client object is about to be freed from memory.
Remove `detach_client` and tidy up the code a bit by including the
freeing of the qctx object (when allocated) inside the qctx_destroy
function instead of requiring extra calls.
The hook NS_QUERY_QCTX_DESTROY is problematic with zone plugins because
it can be called in some contexts where `qctx->client` is invalid (the
pointer is dangling); which would lead to a use-after-free (spotted by
TSAN build) as `qctx->client` is used to get the zone hooktable, to find
out whether there is an authoritive zone which would have
NS_QUERY_QCTX_DESTROY registered.
This can't easily be fixed, because there is no easy way to know from
query.c code if `client` is still a valid object: `client->reqhandle`,
representing the request from a client, is refcounted, and the `client`
object is freed from memory once its refcounter gets to 0. While
`reqhandle` is attached from query.c code, it can be attached more than
once from asynchronous code and there is no clear path where detaching
it would lead to a client free. Hence, there is no way to know for sure
when to set `qctx->client = NULL` (this is why the pointer remains
dangling).
Back to the original problem; this is why the NS_QUERY_QCTX_DESTROY hook
is incompatible with zone plugins. `qctx->detach_client`, which is used
to tell a plugin that the `client` object is either free or about to be
free can't be use either, because in some cases the client is still
there, and should be used.
Code issue aside, the `qctx` object is really just an aggregate of
various data to pass easily in the various functions and callbacks,
initially stored on the stack, but allocated in some cases (for some
asynchronous flow, when recursion is needed), so the point it gets
created/"destroyed" is really just an implementation "detail", and
providing a higher level hook for the plugin would be beneficial. Hence,
NS_QUERY_RESET and NS_QUERY_INIT are removed, and instead, the existing
NS_QUERY_SETUP can be used as well as the newly introduced
NS_QUERY_RESET (which replaces NS_QUERY_QCTX_DESTROY). The advanage is
that NS_QUERY_RESET is called _only_ when the client object is _always_
about to be freed, which avoids usage of the extra `qctx->detach_client`
usage from the plugin.
The way NS_QUERY_RESET works is that when the `client` is freed, a
callback (from `query.c`) is called. This callback creates a transient
qctx object on the stack with a pointer to the view, and uses that
to call the hook.
Add a new query hook called `NS_QUERY_AUTHZONE_ATTACHED`. This hook is
called whenever an authoritative zone is found and attached during a
query answer.
From code level, this hook is called when `qctx->client->query->authzone`
is attached during a query. This enables zone-specific plugins to
initialize specific states whenever a local zone is found that can
answer a query.
Since the log level has been raised, busy servers can "explode" from
the amount of log messages. Use the usual practice of logging "every
once in a while".
The "RPZ not ready yet" message is logged at debug 3 level. Use the
info level instead for better visibility.
After raising the log level, the rpz_log_fail_helper() function starts
appending " failed: " the the message. Change the log message so it
makes more sense.
In order to not pollute the SERVFAIL cache with the configured
SERVFAIL answers while RPZ is loading, set the NS_CLIENTATTR_NOSETFC
attribute for the client.
The counter in ns_client_t is used to track the maximum number of
recursions in the resolver, but it is created unconditionally when
starting the client and deallocated when resetting it.
This commit defers the allocation of the counter till recursion needs to
actually happen, speeding up authoritative workloads in perflab by
1.5~2%.
By default, when named is started it may start answering to
queries before the response policy zones are completely loaded
and processed. This new feature gives an option to the users to
tell named that incoming requests should result in SERVFAIL anwser
until all the response policy zones are procesed and ready.
> Put a space before opening parentheses only after control statement
> keywords (for/if/while...) except this option doesn’t apply to ForEach
> and If macros. This is useful in projects where ForEach/If macros are
> treated as function calls instead of control statements.
Use dns_rdatatype_none instead of plain '0' for dns_rdatatype_t and
dns_typepair_t manipulation. While plain '0' is technically ok, it
doesn't carry the required semantic meaning, and using the named
dns_rdatatype_none constant makes the code more readable.
All databases in the codebase follow the same structure: a database is
an associative container from DNS names to nodes, and each node is an
associative container from RR types to RR data.
Each database implementation (qpzone, qpcache, sdlz, builtin, dyndb) has
its own corresponding node type (qpznode, qpcnode, etc). However, some
code needs to work with nodes generically regardless of their specific
type - for example, to acquire locks, manage references, or
register/unregister slabs from the heap.
Currently, these generic node operations are implemented as methods in
the database vtable, which creates problematic coupling between database
and node lifetimes. If a node outlives its parent database, the node
destructor will destroy all RR data, and each RR data destructor will
try to unregister from heaps by calling a virtual function from the
database vtable. Since the database was already freed, this causes a
crash.
This commit breaks the coupling by standardizing the layout of all
database nodes, adding a dedicated vtable for node operations, and
moving node-specific methods from the database vtable to the node
vtable.
When the zone is configured with a CNAME override policy, also add the
configured EDE code.
When the zone is contains a wildcard CNAME, also add the configured
EDE code.
A serve-stale refresh is similar to a prefetch, the only difference
is when it triggers. Where a prefetch is done when an RRset is about
to expire, a serve-stale refresh is done when the RRset is already
stale.
This means that the check for the stale-refresh window needs to
move into query_stale_refresh(). We need to clear the
DNS_DBFIND_STALEENABLED option at the same places as where we clear
DNS_DBFIND_STALETIMEOUT.
Now that serve-stale refresh acts the same as prefetch, there is no
worry that the same rdataset is added to the message twice. This makes
some code obsolete, specifically where we need to clear rdatasets from
the message.
RRset ordering is now an enum inside struct rdataset attributes. This
was done to keep size to of the structure to its original value before
this MR.
I expect zero performance impact but it should be easier to deal with
attributes in debuggers and language servers.
As mentioned in the comments block before the changed code block,
the dropped or slipped responses should be logged in the query
category (or rather query-errors category as done in lib/ns/client.c),
so that requests are not silently lost.
Also fix a couple of errors/typos in the code comments.
The ns_client_t struct is reset and zero-ed out on every query,
but some fields (query, message, manager) are preserved.
We observe two things:
- The sendbuf field is going to be overwritten anyway, there's
no need to zero it out.
- The fields are copied out when the struct is zero-ed out, and
then copied back in. For the query field (which is 896 bytes)
this is very inefficient.
This commit makes the reset more efficient avoiding to unnecessary
zero-ing and copy.
If ns__query_start() is called because of a chained query (e.g.
after encountering a CNAME), a previously set DNS_DBFIND_STALETIMEOUT
flag on the query's 'dboptions' field can cause an assertion
failure if the new query's 'stalefirst' value is not true (e.g. if the
target qname is an authoritative zone for the server). Reset the
DNS_DBFIND_STALETIMEOUT flag in the query_lookup() function before
evaluating the 'stalefirst' value, and make sure to assign a fresh
value to the `stalefirst' flag instead of conditionally assigning it
only if the value is 'true'.
dns_resolver_algorithm_supported() has been extended so in addition to
an algorithm number, it can also take a pointer to an RRSIG signature
field in which key information is encoded.
DST algorithm and DNSSEC algorithm values are not necessarily the same
anymore: if the DNSSEC algorithm value is PRIVATEOID or PRIVATEDNS, then
the DST algorithm will be mapped to something else. The conversion is
now done correctly where necessary.
replace the pattern `for (result = dns_rdataset_first(x); result ==
ISC_R_SUCCES; result = dns_rdataset_next(x)` with a new
`DNS_RDATASET_FOREACH` macro throughout BIND.
previously, ISC_LIST_FOREACH and ISC_LIST_FOREACH_SAFE were
two separate macros, with the _SAFE version allowing entries
to be unlinked during the loop. ISC_LIST_FOREACH is now also
safe, and the separate _SAFE macro has been removed.
similarly, the ISC_LIST_FOREACH_REV macro is now safe, and
ISC_LIST_FOREACH_REV_SAFE has also been removed.
The `max-rsa-exponent-size` could limit the exponents of the RSA
public keys during the DNSSEC verification. Instead of providing
a cryptic (not cryptographic) knob, hardcode the max exponent to
be 4096 (the theoretical maximum for DNSSEC).
When we optimised the closest encloser NSEC3 discovery the maxlabels
variable was used in the binary search. The updated value was later
used to add the NO QNAME NSEC3 but that block of code needed the
original value. This resulted in the wrong NSEC3 sometimes being
chosen to perform this role.
When 'stale-answer-client-timeout' is 0, named is allowed to return
a stale answer immediately, while also initiating a new query to get
the real answer. This mode is activated in ns__query_start() by setting
the 'qctx->options.stalefirst' optoin to 'true' before calling the
query_lookup() function, but not when the zone is known to be
authoritative to the server. When the zone is authoritative, and
query_looup() finds out that the requested name is a delegation,
then before proceeding with the query, named tries to look it up
in the cache first. Here comes the issue that it doesn't consider
enabling 'qctx->options.stalefirst' in this case, and so the
'stale-answer-client-timeout 0' setting doesn't work for those
delegated zones - instead of immediately returning the stale answer
(if it exists), named tries to resolve it.
Fix this issue by enabling 'qctx->options.stalefirst' in the
query_zone_delegation() function just before named looks up the name
in the cache using a new query_lookup() call. Also, if nothing was
found in the cache, don't initiate another query_lookup() from inside
query_notfound(), and let query_notfound() do its work, i.e. it will
call query_delegation() for further processing.
use the ISC_LIST_FOREACH pattern in places where lists had
been iterated using a different pattern from the typical
`for` loop: for example, `while (!ISC_LIST_EMPTY(...))` or
`while ((e = ISC_LIST_HEAD(...)) != NULL)`.
the pattern `for (x = ISC_LIST_HEAD(...); x != NULL; ISC_LIST_NEXT(...)`
has been changed to `ISC_LIST_FOREACH` throughout BIND, except in a few
cases where the change would be excessively complex.
in most cases this was a straightforward change. in some places,
however, the list element variable was referenced after the loop
ended, and the code was refactored to avoid this necessity.
also, because `ISC_LIST_FOREACH` uses typeof(list.head) to declare
the list elements, compilation failures can occur if the list object
has a `const` qualifier. some `const` qualifiers have been removed
from function parameters to avoid this problem, and where that was not
possible, `UNCONST` was used.
ISC_LIST_FOREACH and related macros now use 'typeof(list.head)' to
declare the list elements automatically; the caller no longer needs
to do so.
ISC_LIST_FOREACH_SAFE also now implicitly declares its own 'next'
pointer, so it only needs three parameters instead of four.
In the code base it is very common to iterate over all names in a message
section and all rdatasets for each name, but various idioms are used for
iteration.
This commit standardizes them as much as possible to a single idiom,
using the macro MSG_SECTION_FOREACH, similar to the existing
ISC_LIST_FOREACH.
the code in query_dns64() that applies the dns64 prefixes to
an A rdataset has been moved into the dns_dns64 module, and
dns_dns64_destroy() now unlinks the dns64 object from its
containing list. with these changes, we no longer need the
list-manipulation API calls dns_dns64_next() and
dns_dns64_unlink().
If there was an EDNS ZONEVERSION option in the DNS request and the
answer was from a zone, return the zone's serial and number of
labels excluding the root label with the type set to 0 (ZONE-SERIAL).
when searching a DNSKEY or KEY rrset for the key that matches
a particular algorithm and ID, it's a waste of time to convert
every key into a dst_key object; it's faster to compute the key
ID by checksumming the region, and then only do the full key
conversion once we know we've found the correct key.
this optimization was already in use in the validator, but it's
been refactored for code clarity, and is now also used in query.c
and message.c.
Extended DNS Error message EDE 20 (Not Authoritative) is now sent when
client request recursion (RD) but the server has recursion disabled.
RFC 8914 mention EDE 20 should also be returned if the client doesn't
have the RD bit set (and recursion is needed) but it doesn't apply for
BIND as BIND would try to resolve from the "deepest" referral in
AUTHORITY section. For example, if the client asks for "www.isc.org/A"
but the server only knows the root domain, it will returns NOERROR but
no answer for "www.isc.og/A", just the list of other servers to ask.
Answers to an "ANY" query which are processed by the RPZ "passthru"
policy have the response-policy's 'max-policy-ttl' value unexpectedly
applied. Do not change the records' TTL when RPZ uses a policy which
does not alter the answer.
if the NS_QUERY_DONE_BEGIN or NS_QUERY_DONE_SEND hook is
used in a plugin and returns NS_HOOK_RETURN, some of the
cleanup in ns_query_done() can be skipped over, leading
to reference leaks that can cause named to hang on shut
down.
this has been addressed by adding more housekeeping
code after the cleanup: tag in ns_query_done().
the target buffer passed to dns_name_concatenate() was never
used (except for one place in dig, where it wasn't actually
needed, and has already been removed in a prior commit).
we can safely remove the parameter.
The offsets were meant to speed-up the repeated dns_name operations, but
it was experimentally proven that there's actually no real-world
benefit. Remove the offsets and labels fields from the dns_name and the
static offsets fields to save 128 bytes from the fixedname in favor of
calculating labels and offsets only when needed.
After a reconfiguration the old view can be left without a valid
'rpzs' member, because when the RPZ is not changed during the named
reconfiguration 'rpzs' "migrate" from the old view into the new
view, so when a query resumes it can find that 'qctx->view->rpzs'
is NULL which query_resume() currently doesn't expect to happen if
it's recursing and 'qctx->rpz_st' is not NULL.
Fix the issue by adding a NULL-check. In order to not split the log
message to two different log messages depending on whether
'qctx->view->rpzs' is NULL or not, change the message to not log
the RPZ policy's "version" which is just a runtime counter and is
most likely not very useful for the users.
The isc_counter_create() doesn't need the return value (it was always
ISC_R_SUCCESS), use the macros to implement the reference counting,
little style cleanup, and expand the unit test.
