Before the changes from this commit were introduced, the accept
callback function will get called twice when accepting connection
during two of these stages:
* when accepting the TCP connection;
* when handshake has completed.
That is clearly an error, as it should have been called only once. As
far as I understand it the mistake is a result of TLS DNS transport
being essentially a fork of TCP transport, where calling the accept
callback immediately after accepting TCP connection makes sense.
This commit fixes this mistake. It did not have any very serious
consequences because in BIND the accept callback only checks an ACL
and updates stats.
When shutting down, the interface manager can be destroyed
before the `route_connected()` callback is called, which is
unexpected for the latter and can cause a crash.
Move the interface manager attachment code from the callback
to the place before the callback is registered using
`isc_nm_routeconnect()` function, which will make sure that
the interface manager will live at least until the callback
is called.
Make sure to detach the interface manager if the
`isc_nm_routeconnect()` function is not implemented, or when
the callback is called with a result value which differs from
`ISC_R_SUCCESS`.
The `ns_interfacemgr_create()` function, when calling
`isc_nm_routeconnect()`, uses the newly created `ns_interfacemgr_t`
instance before initializing its reference count and the magic value.
Defer the `isc_nm_routeconnect()` call until the initializations
are complete.
The only values that the isc_quota_attach() function (called from
check_recursionquota() via recursionquotatype_attach_soft()) can
currently return are: ISC_R_SUCCESS, ISC_R_SOFTQUOTA, and ISC_R_QUOTA.
Instead of just propagating any other (unexpected) error up the call
stack, assert immediately, so that if the isc_quota_* API gets updated
in the future to return values currently matching the "default"
statement, check_recursionquota() can be promptly updated to handle such
new return values as desired.
Reduce code duplication in check_recursionquota() by extracting its
parts responsible for logging to a separate helper function.
Remove result text from the "no more recursive clients" message because
it always says "quota reached" (as the relevant branch is only evaluated
when 'result' is set to ISC_R_QUOTA) and therefore brings no additional
value.
When the client->recursionquota pointer was overloaded by different
features, each of those features had to be aware of that fact and handle
any updates of that pointer gracefully. Example: prefetch code
initiates recursion, attaching to client->recursionquota, then query
processing restarts due to a CNAME being encountered, then that CNAME is
not found in the cache, so another recursion is triggered, but
client->recursionquota is already attached to; even though it is not
CNAME chaining code that attached to that pointer, that code still has
to handle such a situation gracefully.
However, all features that can initiate recursion have now been updated
to use separate slots in the 'recursions' array, so keeping the old
checks in place means masking future programming errors that could
otherwise be caught - and should be caught because each feature needs to
properly maintain its own quota reference.
Remove outdated recursion quota pointer checks to enable the assertions
in isc_quota_*() functions to detect programming errors in code paths
that can start recursion. Remove an outdated comment to prevent
confusion.
ns_client_endrequest() currently contains code that looks for
outstanding quota references and cleans them up if necessary. This
approach masks programming errors because ns_client_endrequest() is only
called from ns__client_reset_cb(), which in turn is only called when all
references to the client's netmgr handle are released, which in turn
only happens after all recursion completion callbacks are invoked
(because isc_nmhandle_attach() is called before every call to
dns_resolver_createfetch() in lib/ns/query.c and the completion callback
is expected to detach from the handle), which in turn is expected to
happen for all recursions attempts, even those that get canceled.
Furthermore, declaring the prototype of ns_client_endrequest() at the
top of lib/ns/client.c is redundant because the definition of that
function is placed before its first use in that file. Remove the
redundant function prototype.
Finally, remove INSIST assertions ensuring quota pointers are NULL in
ns__client_reset_cb() because the latter calls ns_client_endrequest() a
few lines earlier.
Some functions fail to detach from the recursion quota if an error
occurs while initiating recursion. This causes the recursive client
counter to be off. Add missing recursionquota_detach() calls, reworking
cleanup code where appropriate.
Similarly to how different code paths reused common client handle
pointers and fetch references despite being logically unrelated, they
also reuse client->recursionquota, the field in which a reference to the
recursion quota is stored. This unnecessarily forces all code using
that field to be aware of the fact that it is overloaded by different
features.
Overloading client->recursionquota also causes inconsistent behavior.
For example, if prefetch code triggers recursion and then delegation
handling code also triggers recursion, only one of these code paths will
be able to attach to the recursion quota, but both recursions will be
started anyway. In other words, each code path only checks whether the
recursion quota has not been exceeded if the quota has not yet been
attached to by another code path. This behavior theoretically allows
the configured recursion quota to be slightly exceeded; while it is not
expected to be a real-world operational issue, it is still confusing and
should therefore be fixed.
Extend the structures comprising the 'recursions' array with a new field
holding a pointer to the recursion quota that a given recursion process
attached to. Update all code paths using client->recursionquota so that
they use the appropriate slot in the 'recursions' array. Drop the
'recursionquota' field from ns_client_t.
Previously, multiple code paths reused client->query.fetch, so it was
enough for ns_query_cancel() to issue a single call to
dns_resolver_cancelfetch() with that fetch as an argument. Now, since
each slot in the 'recursions' array can hold a reference to a separate
resolver fetch, ns_query_cancel() needs to handle all of them, so that
all recursion callbacks get a chance to clean up the associated
resources when a query is canceled.
Drop the 'fetchhandle' field from ns_client_t as all code using it has
been migrated to use the recursion-type-specific HANDLE_RECTYPE_*()
macros.
Drop the 'fetch' field from ns_query_t as all code using it has been
migrated to use the recursion-type-specific FETCH_RECTYPE_*() macros.
Async hooks are the last feature using the client->fetchhandle and
client->query.fetch pointers. Update ns_query_hookasync() and
query_hookresume() so that they use a dedicated slot in the 'recursions'
array. Note that async hooks are still not expected to initiate
recursion if one was already started by a prior ns_query_recurse() call,
so the REQUIRE assertion in ns_query_hookasync() needs to check the
RECTYPE_NORMAL slot rather than the RECTYPE_HOOK one.
With prefetch and RPZ code updated to use separate slots in the
'recursions' array, the code responsible for starting recursion in
ns_query_recurse() and resuming query handling in fetch_callback()
should follow suit, so that it does not need to explicitly cooperate
with other code paths that may initiate recursion.
Replace:
- client->fetchhandle with HANDLE_RECTYPE_NORMAL(client)
- client->query.fetch with FETCH_RECTYPE_NORMAL(client)
Also update other functions using client->fetchhandle and
client->query.fetch (ns_query_cancel(), query_usestale()) so that those
two fields can shortly be dropped altogether.
Both prefetch code and RPZ code ignore recursion results (caching the
response notwithstanding). RPZ code has been (ab)using that fact since
commit 08e36aa5a5 by employing
prefetch_done() as the fetch completion callback. This is only
seemingly a simplification as it makes the code harder to follow ("why
is prefetch code used for handling RPZ-triggered recursion?").
Turn prefetch_done() into a new function whose name clearly conveys its
purpose. Add a parameter to its prototype in order to allow callers to
specify which slot in the 'recursions' array it should use. Reintroduce
prefetch_done() as a wrapper for that function. Add rpzfetch_done(), an
RPZ-exclusive wrapper for that function (using a distinct recursion
type).
Since each slot in the 'recursions' array needs to be initialized before
getting cleaned up when recursion completes, rework fetch_and_forget()
so that it takes recursion type rather than extra fetch options as the
last parameter and make it use the requested slot in the 'recursions'
array rather than a fixed slot (RECTYPE_PREFETCH) for all callers. This
makes fetch_and_forget() a logical complement of cleanup_after_fetch().
Collectively, these changes make prefetch and RPZ code logically
separate (except for reusing client->recursionquota, which will be
refactored later).
Replace:
- client->prefetchhandle with HANDLE_RECTYPE_PREFETCH(client)
- client->query.prefetch with FETCH_RECTYPE_PREFETCH(client)
This is preparatory work for separating prefetch code from RPZ code.
When a client waits for a prefetch- or RPZ-triggered recursion to
complete, its netmgr handle is attached to client->prefetchhandle and a
reference to the resolver fetch is stored in client->query.prefetch.
Both of these features use the same fields mentioned above. This makes
the code fragile and hard to follow as its logically distinct parts
become intertwined for no obvious reason.
Furthermore, storing pointers related to a specific recursion process in
two different structures makes their purpose harder to grasp than it has
to be.
To alleviate the problem, extend ns_query_t with an array of structures
containing recursion-related pointers. Each feature able to initiate
recursion is supposed to use its own slot in that array, allowing
logically unrelated code paths to be untangled. Prefetch and RPZ will
be the first users of that array.
Define helper macros for accessing specific recursion-related pointers
in order to improve code readability.
Initialize client->query using a compound literal in order to make the
ns_query_init() function shorter and more readable. This also prevents
the need to explicitly initialize any newly added fields in the future.
query_prefetch() and query_rpzfetch() contain a lot of duplicated code.
Extract the common bits into a separate function whose name clearly
suggests its purpose.
Add a set of new helper functions for attaching to the recursion quota
in order to reduce code duplication and to ensure that the recursive
clients counter is always adjusted properly. Since some callers
(query_prefetch(), query_rpzfetch()) treat exceeding the soft quota as
an error while others (check_recursionquota()) do not, also add two
wrapper functions whose names help convey their purpose, in order to
improve code readability.
Add a new helper function for detaching from the recursion quota in
order to reduce code duplication and to ensure that detaching from that
quota is always accompanied by decreasing the recursive clients counter.
Under specific rare timing circumstances the uv_read_start() could
fail with UV_EINVAL when the connection is reset between the connect (or
accept) and the uv_read_start() call on the nmworker loop. Handle such
situation gracefully by propagating the errors from uv_read_start() into
upper layers, so the socket can be internally closed().
when serve-stale is enabled, NXDOMAIN cache entries are no longer
preserved after the normal negative cache TTL, in order to reduce
unnecessary cache memory consumption.
Commit 9ffb4a7ba1 causes Clang Static
Analyzer to flag a potential NULL dereference in query_nxdomain():
query.c:9394:26: warning: Dereference of null pointer [core.NullDereference]
if (!qctx->nxrewrite || qctx->rpz_st->m.rpz->addsoa) {
^~~~~~~~~~~~~~~~~~~
1 warning generated.
The warning above is for qctx->rpz_st potentially being a NULL pointer
when query_nxdomain() is called from query_resume(). This is a false
positive because none of the database lookup result codes currently
causing query_nxdomain() to be called (DNS_R_EMPTYWILD, DNS_R_NXDOMAIN)
can be returned by a database lookup following a recursive resolution
attempt. Add a NULL check nevertheless in order to future-proof the
code and silence Clang Static Analyzer.
ns_client_getnamebuf() cannot fail (i.e. return NULL) since commit
e31cc1eeb4. Remove redundant NULL checks
performed on the pointer returned by ns_client_getnamebuf().
ns_client_newname() cannot fail (i.e. return NULL) since commit
2ce0de6995 (though it was only made more
apparent by commit 33ba0057a7). Remove
redundant NULL checks performed on the pointer returned by
ns_client_newname().
ns_client_newrdataset() cannot fail (i.e. return NULL) since commit
efb385ecdc (though it was only made more
apparent by commit 33ba0057a7). Remove
redundant NULL checks performed on the pointer returned by
ns_client_newrdataset().
The conversion of `DNS_R_PARTIALMATCH` into `DNS_R_NOTFOUND` is done
in the `dns_rbt_deletename()` function so there is no need to do that
in `dns_fwdtable_delete()`.
Add a possible return value of `ISC_R_NOSPACE` into the header file's
function description comment.
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.
When processing a catalog zone update, skip processing records with
DNSSEC-related and ZONEMD types, because we are not interested in them
in the context of a catalog zone, and processing them will fail and
produce an unnecessary warning message.
It is simply called "compression" now, without any qualifiers. Also,
improve some variable names in dns_name_towire2() so they are not two
letter abbreviations for global something.
It's wasteful to use 20 bytes and a pointer indirection to represent
two bits of information, so turn the struct into an enum. And change
the names of the enumeration constants to make the intent more clear.
This change introduces some inline functions into another header,
which confuses `gcovr` when it is trying to collect code coverage
statistics. So, in the CI job, copy more header files into a directory
where `gcovr` looks for them.
The aim is to get rid of the obsolete term "GLOBAL14" and instead just
refer to DNS name compression.
This is mostly mechanically renaming
from dns_(de)compress_(get|set)methods()
to dns_(de)compress_(get|set)permitted()
and replacing the related enum by a simple flag, because compression
is either on or off.
There was a proposal in the late 1990s that it might, but it turned
out to be unworkable. See RFC 6891, Extension Mechanisms for
DNS (EDNS(0)), section 5, Extended Label Types.
The remnants of the code that supported this in BIND are redundant.
The key lifetime should not be shorter than the time it costs to
introduce the successor key, otherwise keys will be created faster than
they are removed, resulting in a large key set.
The time it takes to replace a key is determined by the publication
interval (Ipub) of the successor key and the retire interval of the
predecessor key (Iret).
For the ZSK, Ipub is the sum of the DNSKEY TTL and zone propagation
delay (and publish safety). Iret is the sum of Dsgn, the maximum zone
TTL and zone propagation delay (and retire safety). The sign delay is
the signature validity period minus the refresh interval: The time to
ensure that all existing RRsets have been re-signed with the new key.
The ZSK lifetime should be larger than both values.
For the KSK, Ipub is the sum of the DNSKEY TTL and zone propagation
delay (and publish safety). Iret is the sum of the DS TTL and parent
zone propagation delay (and retire safety). The KSK lifetime should be
larger than both values.
The signatures-refresh should not near the signatures-validity value,
to prevent operational instability. Same is true when checking against
signatures-validity-dnskey.
The unit tests are now using a common base, which means that
lib/dns/tests/ code now has to include lib/isc/include/isc/test.h and
link with lib/isc/test.c and lib/ns/tests has to include both libisc and
libdns parts.
Instead of cross-linking code between the directories, move the
/lib/<foo>/test.c to /tests/<foo>.c and /lib/<foo>/include/<foo>test.h
to /tests/include/tests/<foo>.h and create a single libtest.la
convenience library in /tests/.
At the same time, move the /lib/<foo>/tests/ to /tests/<foo>/ (but keep
it symlinked to the old location) and adjust paths accordingly. In few
places, we are now using absolute paths instead of relative paths,
because the directory level has changed. By moving the directories
under the /tests/ directory, the test-related code is kept in a single
place and we can avoid referencing files between libns->libdns->libisc
which is unhealthy because they live in a separate Makefile-space.
In the future, the /bin/tests/ should be merged to /tests/ and symlink
kept, and the /fuzz/ directory moved to /tests/fuzz/.
The unit tests contain a lot of duplicated code and here's an attempt
to reduce code duplication.
This commit does several things:
1. Remove #ifdef HAVE_CMOCKA - we already solve this with automake
conditionals.
2. Create a set of ISC_TEST_* and ISC_*_TEST_ macros to wrap the test
implementations, test lists, and the main test routine, so we don't
have to repeat this all over again. The macros were modeled after
libuv test suite but adapted to cmocka as the test driver.
A simple example of a unit test would be:
ISC_RUN_TEST_IMPL(test1) { assert_true(true); }
ISC_TEST_LIST_START
ISC_TEST_ENTRY(test1)
ISC_TEST_LIST_END
ISC_TEST_MAIN (Discussion: Should this be ISC_TEST_RUN ?)
For more complicated examples including group setup and teardown
functions, and per-test setup and teardown functions.
3. The macros prefix the test functions and cmocka entries, so the name
of the test can now match the tested function name, and we don't have
to append `_test` because `run_test_` is automatically prepended to
the main test function, and `setup_test_` and `teardown_test_` is
prepended to setup and teardown function.
4. Update all the unit tests to use the new syntax and fix a few bits
here and there.
5. In the future, we can separate the test declarations and test
implementations which are going to greatly help with uncluttering the
bigger unit tests like doh_test and netmgr_test, because the test
implementations are not declared static (see `ISC_RUN_TEST_DECLARE`
and `ISC_RUN_TEST_IMPL` for more details.
NOTE: This heavily relies on preprocessor macros, but the result greatly
outweighs all the negatives of using the macros. There's less
duplicated code, the tests are more uniform and the implementation can
be more flexible.
Previously, tasks could be created either unbound or bound to a specific
thread (worker loop). The unbound tasks would be assigned to a random
thread every time isc_task_send() was called. Because there's no logic
that would assign the task to the least busy worker, this just creates
unpredictability. Instead of random assignment, bind all the previously
unbound tasks to worker 0, which is guaranteed to exist.
This commit separates TLS context creation code from xfrin_start() as
it has become too large and hard to follow into a new
function (similarly how it is done in dighost.c)
The dead code has been removed from the cleanup section of the TLS
creation code:
* there is no way 'tlsctx' can equal 'found';
* there is no way 'sess_cache' can be non-NULL in the cleanup section.
Also, it fixes a bug in the older version of the code, where TLS
client session context fetched from the cache would not get passed to
isc_nm_tlsdnsconnect().
Typing from libuv structure to isc_region_t is not possible, because
their sizes differ on 64 bit architectures. Little endian machines seems
to be lucky and still result in test passed. But big endian machine such
as s390x fails the test reliably.
Fix by directly creating the buffer as isc_region_t and skipping the
type conversion. More readable and still more correct.
This commit disables periodic interface re-scans timer on Linux where
a kernel-based dynamic interface mechanisms make it a thing of the
past in most cases.
