Change the taskmgr (and thus netmgr) in a way that it supports fast and
slow task queues. The fast queue is used for incoming DNS traffic and
it will pass the processing to the slow queue for sending outgoing DNS
messages and processing resolver messages.
In the future, more tasks might get moved to the slow queues, so the
cached and authoritative DNS traffic can be handled without being slowed
down by operations that take longer time to process.
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.
(cherry picked from commit fb7bbbd1be)
in the past there was overlap between the fields used
as resolver fetch options and ADB addrinfo flags. this has
mostly been eliminated; now we can clean up the rest of
it and remove some confusing comments.
(cherry picked from commit 0955cf1af5)
When DNS_FETCHOPT_NOFOLLOW is set DNS_R_DELEGATION needs to be
returned to restart the resolution process rather than converting
it to ISC_R_SUCCESS.
(cherry picked from commit ea11650376)
If we know that the NS RRset for an intermediate label doesn't exist
on cache contents don't query using that name when looking for a
referral.
(cherry picked from commit 80bc0ee075)
There is no harm in aquiring an additional reference to the resolver
after it has started shutting down. All the REQUIRE was doing was
introducing a point of failure when shutting down the server.
If the resolver received a FORMERR response to a request with
an DNS COOKIE option present that echoes the option back, resend
the request without an DNS COOKIE option present.
(cherry picked from commit f3b24ba789)
The number of clients per query is calculated using the pending
fetch responses in the list. The dns_resolver_createfetch() function
includes every item in the list when deciding whether the limit is
reached (i.e. fctx->spilled is true). Then, when the limit is reached,
there is another calculation in fctx_sendevents(), when deciding
whether it is needed to increase the limit, but this time the TRYSTALE
responses are not included in the calculation (because of early break
from the loop), and because of that the limit is never increased.
A single client can have more than one associated response/event in the
list (currently max. two), and calculating them as separate "clients"
is unexpected. E.g. if 'stale-answer-enable' is enabled and
'stale-answer-client-timeout' is enabled and is larger than 0, then
each client will have two events, which will effectively halve the
clients-per-query limit.
Fix the dns_resolver_createfetch() function to calculate only the
regular FETCHDONE responses/events.
Change the fctx_sendevents() function to also calculate only FETCHDONE
responses/events. Currently, this second change doesn't have any impact,
because the TRYSTALE events were already skipped, but having the same
condition in both places will help prevent similar bugs in the future
if a new type of response/event is ever added.
(cherry picked from commit 2ae5c4a674)
There is a lock-order-inversion (potential deadlock) in resolver.c,
because in dns_resolver_shutdown() a resolver bucket lock is locked
while the resolver lock itself is already locked, while in
fctx_sendevents() the resolver lock is locked while a bucket lock
is locked before calling that function in fctx__done_detach().
The resolver lock/unlock in dns_resolver_shutdown() was added back in
the 317e36d47e commit to make sure that
the function is finished before the resolver object is destroyed.
Since res->exiting is atomic, it should be possible to remove the
resolver locking in dns_resolver_shutdown() and add it to the
send_shutdown_events() function which requires it.
Also, since 'res->exiting' is now set while unlocked, the 'INSIST'
in spillattimer_countdown() is wrong, and is removed.
This counter indicates the number of the resolver's spilled
queries due to reaching the clients per query quota.
(cherry picked from commit 04648d7c2f)
When resquery_response() was called with ISC_R_SHUTTINDOWN, the region
argument would be NULL, but rctx_respinit() would try to pass
region->base and region->len to the isc_buffer_init() leading to
a NULL pointer dereference. Properly handle non-ISC_R_SUCCESS by
ignoring the provided region.
(cherry picked from commit 93259812dd)
The reference counting and isc_timer_attach()/isc_timer_detach()
semantic are actually misleading because it cannot be used under normal
conditions. The usual conditions under which is timer used uses the
object where timer is used as argument to the "timer" itself. This
means that when the caller is using `isc_timer_detach()` it needs the
timer to stop and the isc_timer_detach() does that only if this would be
the last reference. Unfortunately, this also means that if the timer is
attached elsewhere and the timer is fired it will most likely be
use-after-free, because the object used in the timer no longer exists.
Remove the reference counting from the isc_timer unit, remove
isc_timer_attach() function and rename isc_timer_detach() to
isc_timer_destroy() to better reflect how the API needs to be used.
The only caveat is that the already executed event must be destroyed
before the isc_timer_destroy() is called because the timer is no longet
attached to .ev_destroy_arg.
(cherry picked from commit ae01ec2823)
Although 'dns_fetch_t' fetch can have two associated events, one for
each of 'DNS_EVENT_FETCHDONE' and 'DNS_EVENT_TRYSTALE' types, the
dns_resolver_cancelfetch() function is designed in a way that it
expects only one existing event, which it must cancel, and when it
happens so that 'stale-answer-client-timeout' is enabled and there
are two events, only one of them is canceled, and it results in an
assertion in dns_resolver_destroyfetch(), when it finds a dangling
event.
Change the logic of dns_resolver_cancelfetch() function so that it
cancels both the events (if they exist), and in the right order.
(cherry picked from commit ec2098ca35)
DSCP has not been fully working since the network manager was
introduced in 9.16, and has been completely broken since 9.18.
This seems to have caused very few difficulties for anyone,
so we have now marked it as obsolete and removed the
implementation.
To ensure that old config files don't fail, the code to parse
dscp key-value pairs is still present, but a warning is logged
that the feature is obsolete and should not be used. Nothing is
done with configured values, and there is no longer any
range checking.
(cherry picked from commit 916ea26ead)
If the isc_task_create_bound() fails in the middle of buckets
initialization - the most common case would be shutdown initialized
during reload, not all tasks would be initialized, but the cleanup
code would try to cleanup all buckets.
Make sure that we cleanup only the initialized buckets by setting
ntasks to the number of already initialized tasks on the error path.
Normally, when a 'resquery_t' object is created in fctx_query(),
we call dns_adb_beginudpfetch() (which increases the ADB quota)
only if it's a UDP query. Then, in fctx_cancelquery(), we call
dns_adb_endudpfetch() to decreases back the ADB quota, again only
if it's a UDP query.
The problem is that a UDP query can become a TCP query, preventing
the quota from adjusting back in fctx_cancelquery() later.
Call dns_adb_beginudpfetch() also when switching the query type
from UDP to TCP.
(cherry picked from commit 53afe1f978)
The dispatches are not thread-bound, and used freely between various
threads (see the dns_resolver and dns_request units for details).
This refactoring make sure that all non-const dns_dispatch_t and
dns_dispentry_t members are accessed under a lock, and both object now
track their internal state (NONE, CONNECTING, CONNECTED, CANCELED)
instead of guessing the state from the state of various struct members.
During the refactoring, the artificial limit DNS_DISPATCH_SOCKSQUOTA on
UDP sockets per dispatch was removed as the limiting needs to happen and
happens on in dns_resolver and limiting the number of UDP sockets
artificially in dispatch could lead to unpredictable behaviour in case
one dispatch has the limit exhausted by others are idle.
The TCP artificial limit of DNS_DISPATCH_MAXREQUESTS makes even less
sense as the TCP connections are only reused in the dns_request API
that's not a heavy user of the outgoing connections.
As a side note, the fact that UDP and TCP dispatch pretends to be same
thing, but in fact the connected UDP is handled from dns_dispentry_t and
dns_dispatch_t acts as a broker, but connected TCP is handled from
dns_dispatch_t and dns_dispatchmgr_t acts as a broker doesn't really
help the clarity of this unit.
This refactoring kept to API almost same - only dns_dispatch_cancel()
and dns_dispatch_done() were merged into dns_dispatch_done() as we need
to cancel active netmgr handles in any case to not leave dangling
connections around. The functions handling UDP and TCP have been mostly
split to their matching counterparts and the dns_dispatch_<function>
functions are now thing wrappers that call <udp|tcp>_dispatch_<function>
based on the socket type.
More debugging-level logging was added to the unit to accomodate for
this fact.
(cherry picked from commit 6f317f27ea)
The various factors like NS_PER_MS are now defined in a single place
and the names are no longer inconsistent. I chose the _PER_SEC names
rather than _PER_S because it is slightly more clear in isolation;
but the smaller units are always NS, US, and MS.
(cherry picked from commit 00307fe318)
When using dual-stack-servers the covering namespace to check whether
answers are in scope or not should be fctx->domain. To do this we need
to be able to distingish forwarding due to forwarders clauses and
dual-stack-servers. A new flag FCTX_ADDRINFO_DUALSTACK has been added
to signal this.
(cherry picked from commit dfbffd77f9)
ARM states that the "eligibility" TTL is the smallest original TTL
value that is accepted for a record to be eligible for prefetching,
but the code, which implements the condition doesn't behave in that
manner for the edge case when the TTL is equal to the configured
eligibility value.
Fix the code to check that the TTL is greater than, or equal to the
configured eligibility value, instead of just greater than it.
(cherry picked from commit 863f51466e)
For UDP queries, after calling dns_adb_beginudpfetch() in fctx_query(),
make sure that dns_adb_endudpfetch() is also called on error path, in
order to adjust the quota back.
(cherry picked from commit 5da79e2be0)
It is currently possible that dns_adb_endudpfetch() is not
called in fctx_cancelquery() for a UDP query, which results
in quotas not being adjusted back.
Always call dns_adb_endudpfetch() for UDP queries.
(cherry picked from commit e4569373ca)
In the cleanup code of fctx_query() function there is a code path
where 'query' is linked to 'fctx' and it is being destroyed.
Make sure that 'query' is unlinked before destroying it.
(cherry picked from commit ac889684c7)
Mostly generated automatically with the following semantic patch,
except where coccinelle was confused by #ifdef in lib/isc/net.c
@@ expression list args; @@
- UNEXPECTED_ERROR(__FILE__, __LINE__, args)
+ UNEXPECTED_ERROR(args)
@@ expression list args; @@
- FATAL_ERROR(__FILE__, __LINE__, args)
+ FATAL_ERROR(args)
(cherry picked from commit ec50c58f52)
Limit the amount of database lookups that can be triggered in
fctx_getaddresses() (i.e. when determining the name server addresses to
query next) by setting a hard limit on the number of NS RRs processed
for any delegation encountered. Without any limit in place, named can
be forced to perform large amounts of database lookups per each query
received, which severely impacts resolver performance.
The limit used (20) is an arbitrary value that is considered to be big
enough for any sane DNS delegation.
(cherry picked from commit 3a44097fd6)
When initially hitting the `fetches-per-zone` value, a log message
is being generated for the event of dropping the first fetch, then
any further log events occur only when another fetch is being dropped
and 60 seconds have been passed since the last logged message.
That logic isn't ideal because when the counter of the outstanding
fetches reaches zero, the structure holding the counters' values will
get deleted, and the information about the dropped fetches accumulated
during the last minute will not be logged.
Improve the fcount_logspill() function to makie sure that the final
values are getting logged before the counter object gets destroyed.
(cherry picked from commit 039871ceb767088205563965f7aae622a3f77082)
Commit 7b2ea97e46 introduced a logic bug
in resume_dslookup(): that function now only conditionally checks
whether DS chasing can still make progress. Specifically, that check is
only performed when the previous resume_dslookup() call invokes
dns_resolver_createfetch() with the 'nameservers' argument set to
something else than NULL, which may not always be the case. Failing to
perform that check may trigger assertion failures as a result of
dns_resolver_createfetch() attempting to resolve an invalid name.
Example scenario that leads to such outcome:
1. A validating resolver is configured to forward all queries to
another resolver. The latter returns broken DS responses that
trigger DS chasing.
2. rctx_chaseds() calls dns_resolver_createfetch() with the
'nameservers' argument set to NULL.
3. The fetch fails, so resume_dslookup() is called. Due to
fevent->result being set to e.g. DNS_R_SERVFAIL, the default branch
is taken in the switch statement.
4. Since 'nameservers' was set to NULL for the fetch which caused the
resume_dslookup() callback to be invoked
(fctx->nsfetch->private->nameservers), resume_dslookup() chops off
one label off fctx->nsname and calls dns_resolver_createfetch()
again, for a name containing one label less than before.
5. Steps 3-4 are repeated (i.e. all attempts to find the name servers
authoritative for the DS RRset being chased fail) until fctx->nsname
becomes stripped down the the root name.
6. Since resume_dslookup() does not check whether DS chasing can still
make progress, it strips off a label off the root name and continues
its attempts at finding the name servers authoritative for the DS
RRset being chased, passing an invalid name to
dns_resolver_createfetch().
Fix by ensuring resume_dslookup() always checks whether DS chasing can
still make progress when a name server fetch fails. Update code
comments to ensure the purpose of the relevant dns_name_equal() check is
clear.
(cherry picked from commit 1a79aeab44)
previously, when an iterative query returned FORMERR, resolution
would be stopped under the assumption that other servers for
the same domain would likely have the same capabilities. this
assumption is not correct; some domains have been reported for
which some but not all servers will return FORMERR to a given
query; retrying allows recursion to succeed.
(cherry picked from commit f6abb80746)
it's a style violation to have REQUIRE or INSIST contain code that
must run for the server to work. this was being done with some
atomic_compare_exchange calls. these have been cleaned up. uses
of atomic_compare_exchange in assertions have been replaced with
a new macro atomic_compare_exchange_enforced, which uses RUNTIME_CHECK
to ensure that the exchange was successful.
(cherry picked from commit a499794984)
When processing a catalog zone member zone make sure that there is no
configured pre-existing forward zone with that name.
Refactor the `dns_fwdtable_find()` function to not alter the
`DNS_R_PARTIALMATCH` result (coming from `dns_rbt_findname()`) into
`DNS_R_SUCCESS`, so that now the caller can differentiate partial
and exact matches. Patch the calling sites to expect and process
the new return value.
(cherry picked from commit 2aff264fb1)
previously fctx_done() detached the fctx but did not clear the pointer
passed into it from the caller. in some conditions, when rctx_done()
was reached while waiting for a validator to complete, fctx_done()
could be called twice on the same fetch, causing a double detach.
fctx_done() now clears the fctx pointer, to reduce the chances of
such mistakes.
(cherry picked from commit b4592d02a1)
The rctx_chaseds() function calls dns_resolver_createfetch(), passing
fctx->task as the target task to run resume_dslookup() from. This
breaks task-based serialization of events as fctx->task is the task that
the dns_resolver_createfetch() caller wants to receive its fetch
completion event in; meanwhile, intermediate fetches started by the
resolver itself (e.g. related to QNAME minimization) must use
res->buckets[bucketnum].task instead. This discrepancy may cause
trouble if the resume_dslookup() callback happens to be run concurrently
with e.g. fctx_doshutdown().
Fix by passing the correct task to dns_resolver_createfetch() in
rctx_chaseds().
(cherry picked from commit 741a7096fc)
Historically, the inline keyword was a strong suggestion to the compiler
that it should inline the function marked inline. As compilers became
better at optimising, this functionality has receded, and using inline
as a suggestion to inline a function is obsolete. The compiler will
happily ignore it and inline something else entirely if it finds that's
a better optimisation.
Therefore, remove all the occurences of the inline keyword with static
functions inside single compilation unit and leave the decision whether
to inline a function or not entirely on the compiler
NOTE: We keep the usage the inline keyword when the purpose is to change
the linkage behaviour.
(cherry picked from commit 20f0936cf2)
Previously, the unreachable code paths would have to be tagged with:
INSIST(0);
ISC_UNREACHABLE();
There was also older parts of the code that used comment annotation:
/* NOTREACHED */
Unify the handling of unreachable code paths to just use:
UNREACHABLE();
The UNREACHABLE() macro now asserts when reached and also uses
__builtin_unreachable(); when such builtin is available in the compiler.
(cherry picked from commit 584f0d7a7e)
Gcc 7+ and Clang 10+ have implemented __attribute__((fallthrough)) which
is explicit version of the /* FALLTHROUGH */ comment we are currently
using.
Add and apply FALLTHROUGH macro that uses the attribute if available,
but does nothing on older compilers.
In one case (lib/dns/zone.c), using the macro revealed that we were
using the /* FALLTHROUGH */ comment in wrong place, remove that comment.
(cherry picked from commit fe7ce629f4)
The fetch can be in the shutting down state when resume_dslookup() is
trying to operate on it.
This is also a security issue, because a malicious actor can set up a
name server which delays certain queries in such a way that the fetch
will time out and shut down, which will cause named to crash.
Add a check to see if the fetch has the shutting down attribute set,
and cancel any further operations on it in such case.
A similar bug had been fixed earlier for the resume_qmin() function,
see [GL #966].
When caching glue, we need to ensure that there is no closer
source of truth for the name. If the owner name for the glue
record would be answered by a locally configured zone, do not
cache.
When caching additional and glue data *not* from a forwarder, we must
check that there is no "forward only" clause covering the owner name
that would take precedence. Such names would normally be allowed by
baliwick rules, but a "forward only" zone introduces a new baliwick
scope.
If we are using a fowarder, in addition to checking that names to
be cached are subdomains of the forwarded namespace, we must also
check that there are no subsidiary forwarded namespaces which would
take precedence. To be safe, we don't cache any responses if the
forwarding configuration has changed since the query was sent.
Commit 21ae6bb1b2 removed most uses of the
'fctx' variable from the rctx_dispfail() function: it is now only needed
by the FCTXTRACE3() macro. However, when --enable-querytrace is not in
effect, that macro evaluates to a list of UNUSED() macros that does not
include "UNUSED(fctx);". This triggers the following compilation
warning when building without --enable-querytrace:
resolver.c: In function 'rctx_dispfail':
resolver.c:7888:21: warning: unused variable 'fctx' [-Wunused-variable]
7888 | fetchctx_t *fctx = rctx->fctx;
| ^~~~
Fix by adding "UNUSED(fctx);" lines to all FCTXTRACE*() macros. This is
safe to do because all of those macros use the 'fctx' local variable, so
there is no danger of introducing new errors caused by use of undeclared
identifiers.
(cherry picked from commit b645e28167)
