There were two errors:
1. get_random() function was returning random number with leading zeros
that could lead the shell to interpret the number as octal value
instead of decimal. The surrounding whitespace was also causing
problems.
2. The calculation of the port was off, it was adding the whole range
and not just the min port to the base.
The introduction of netmgr doubled the number of threads from which
dnstap data may be logged: previously, it could only happen from within
taskmgr worker threads; with netmgr, it can happen both from taskmgr
worker threads and from network threads. Since the argument passed to
fstrm_iothr_options_set_num_input_queues() was not updated to reflect
this change, some calls to fstrm_iothr_get_input_queue() can now return
NULL, effectively preventing some dnstap data from being logged.
Whether this bug is triggered or not depends on thread scheduling order
and packet distribution between network threads, but will almost
certainly be triggered on any recursive resolver sooner or later. Fix
by requesting the correct number of dnstap input queues to be allocated.
Ensure the release checklist reflects our current release process:
- add an additional deadline for introducing code changes ("code
freeze"); only test and documentation tweaks can be applied to
pending releases after this deadline passes,
- notify Support and Marketing about an impending release earlier in
the process so that they have time to schedule a release note review
before the tagging deadline,
- examine current test results on all platforms in advance, to prevent
diagnosing and addressing test failures in the last minute before
the tagging deadline,
- check Perflab results earlier in the process to leave some room for
addressing any potential problems before code freeze,
- ensure empty release notes for the next set of releases are prepared
after public release.
nzf_append is conditionally compiled and this is intended to
catch error introduced by changes to the called functions on all
systems before the changes are run through the CI.
Test zones with various escape sequences and filesystem seperator
characters.
* escaped double quote (\")
* escaped escape (\\)
* escaped decimal byte value (\032)
* slash seperator (/)
Per Current Mechanisms 2.3.5, the curve name is DER-encoded in the
EC_PARAMS attribute, and the public key value is DER-encoded in the
EC_POINT attribute.
The system tests currently uses patchwork of shell scripts which doesn't
offer proper error handling.
This commit introduced option to write new tests in pytest framework
that also allows easier manipulation of DNS traffic (using dnspython),
native XML and JSON manipulation and proper error reporting.
named_os_openfile was being called with switch_user set to true
unconditionally leading to log messages about being unable to
switch user identity from named when regenerating the key.
When running on Linux and system capabilities are available, named will
drop the extra capabilities before loading the configuration. This led
to spurious warnings from `seteuid()` because named already dropped
CAP_SETUID and CAP_GETUID capabilities.
The fix removes setting the effective uid/gid when capabilities are
available, and adds a check that we are running under the user we were
requested to run.
Add recursive "test" and "unit" rules, which execute "make check"
in specific directories - "make test" runs the system tests, and
"make unit" runs the unit tests.
The current script used ephemeral port range which clashed with the
ports used by the tools (dig, ...), and the range always started with
the first port and there was 100 ports allocated for each system test.
In this commit, the first port has been randomized, the get_ports.sh
script outputs the variables (the output has to be eval'ed from run.sh)
and there's less waste in the port range.
There are several improvements over the default/previous behaviour of
the test log driver and log compiler:
* The system-test-driver.sh was dropped (it was used incorrectly)
* The run.sh script is now both log compiler and cli script to run
individual tests
* The custom-test-driver was added as extended version of the automake
test-driver with capability to tee the test output to stdout when
`--verbose yes` is passed to it (you can use LOG_DRIVER_FLAGS to
add the option by default)
* Makefile.am has been extended to honor V=1 for the system tests
test-driver (e.g. V=1 adds `--verbose yes` to AM_LOG_DRIVER_FLAGS)
fstrm_capture is not an essential utility, but its corresponding
Makefile token needs to substituted even if it is not found in PATH or
else the "dnstap" system test will consistently fail.
The bin/tests/wire_test helper program is currently not included in any
Makefile.am file. Move its source code to bin/tests/system and build it
along other helper tools when dnstap support is requested as the
"dnstap" system test needs this tool in order to pass.
Make yaml.load_all() use yaml.SafeLoader to address a warning currently
emitted when bin/tests/system/dnstap/ydump.py is run:
ydump.py:28: YAMLLoadWarning: calling yaml.load_all() without Loader=... is deprecated, as the default Loader is unsafe. Please read https://msg.pyyaml.org/load for full details.
for l in yaml.load_all(f.stdout):
The libirs contained own re-implementations of the getaddrinfo,
getnameinfo and gai_strerror + irs_context and irs_dnsconf API that was
unused anywhere in the BIND 9.
Keep just the irs_resonf API that is being extensively used to parse
/etc/resolv.conf by several of BIND 9 tools.
The 'ephemeral' database implementation was used to provide a
lightweight database implemenation that doesn't cache results, and the
only place where it was really use is "samples" because delv is
overriding this to use "rbtdb" instead. Otherwise it was completely
unused.
* The 'ephemeral' cache DB (ecdb) implementation. An ecdb just provides
* temporary storage for ongoing name resolution with the common DB interfaces.
* It actually doesn't cache anything. The implementation expects any stored
* data is released within a short period, and does not care about the
* scalability in terms of the number of nodes.
The three libdns tests directly include ../dst_internal.h which
in turn directly include openssl headers, thus there was a missing
path and build failure on systems where OpenSSL is not in the default
include path.
Right before the release API version (LIBINTERFACE, LIBREVISION, LIBAGE)
for older and newer libraries tends to be the same. Given that, commit
hash can't be the determining factor here, Unix time of the commit
should suit us better and is placed after the API version. The commit
hash is preserved as it's useful to see it in the actual report.
'-nosymtbl' versions of libraries are not produced in Automake builds.
this addresses a race that could occur during shutdown or when
reconfiguring to remove RPZ zones.
this change should ensure that the rpzs structure and the incremental
updates don't interfere with each other: rpzs->zones entries cannot
be set to NULL while an update quantum is running, and the
task should be destroyed and its queue purged so that no subsequent
quanta will run.
The rewrite of BIND 9 build system is a large work and cannot be reasonable
split into separate merge requests. Addition of the automake has a positive
effect on the readability and maintainability of the build system as it is more
declarative, it allows conditional and we are able to drop all of the custom
make code that BIND 9 developed over the years to overcome the deficiencies of
autoconf + custom Makefile.in files.
This squashed commit contains following changes:
- conversion (or rather fresh rewrite) of all Makefile.in files to Makefile.am
by using automake
- the libtool is now properly integrated with automake (the way we used it
was rather hackish as the only official way how to use libtool is via
automake
- the dynamic module loading was rewritten from a custom patchwork to libtool's
libltdl (which includes the patchwork to support module loading on different
systems internally)
- conversion of the unit test executor from kyua to automake parallel driver
- conversion of the system test executor from custom make/shell to automake
parallel driver
- The GSSAPI has been refactored, the custom SPNEGO on the basis that
all major KRB5/GSSAPI (mit-krb5, heimdal and Windows) implementations
support SPNEGO mechanism.
- The various defunct tests from bin/tests have been removed:
bin/tests/optional and bin/tests/pkcs11
- The text files generated from the MD files have been removed, the
MarkDown has been designed to be readable by both humans and computers
- The xsl header is now generated by a simple sed command instead of
perl helper
- The <irs/platform.h> header has been removed
- cleanups of configure.ac script to make it more simpler, addition of multiple
macros (there's still work to be done though)
- the tarball can now be prepared with `make dist`
- the system tests are partially able to run in oot build
Here's a list of unfinished work that needs to be completed in subsequent merge
requests:
- `make distcheck` doesn't yet work (because of system tests oot run is not yet
finished)
- documentation is not yet built, there's a different merge request with docbook
to sphinx-build rst conversion that needs to be rebased and adapted on top of
the automake
- msvc build is non functional yet and we need to decide whether we will just
cross-compile bind9 using mingw-w64 or fix the msvc build
- contributed dlz modules are not included neither in the autoconf nor automake
With the introduction of dnssec-policy, the aforementioned tools were
either rendered obsolete, or they will be replaced with dnssec-policy
based tools. Remove the tools and the requirement to have Python
installed. Python 3 is still being used for tests, so keep the autoconf
test, but make it much simpler.
Previously, the code would do:
REQUIRE(alg == CURVE1 || alg == CURVE2);
[...]
if (alg == CURVE1) { /* code for CURVE1 */ }
else { /* code for CURVE2 */ }
This approach is less extensible and also more prone to errors in case
the initial REQUIRE() is forgotten. The code has been refactored to
use:
REQUIRE(alg == CURVE1 || alg == CURVE2);
[...]
switch (alg) {
case CURVE1: /* code for CURVE1 */; break;
case CURVE2: /* code for CURVE2 */; break;
default: INSIST(0);
}
The pk11/constants.h header contained static CK_BYTE arrays and
we had to use #defines to pull only those we need. This commit
changes the constants to only define byte arrays with the content
and either use them directly or define the CK_BYTE arrays locally
where used.
Coverity showed that the return value of `dst_key_gettime` was
unchecked in INITIALIZE_STATE. If DST_TIME_CREATED was not set we
would set the state to be initialized to a weird last changed time.
This would normally not happen because DST_TIME_CREATED is always
set. However, we would rather set the time to now (as the comment
also indicates) not match the creation time.
The comment on INITIALIZE_STATE also needs updating as we no
longer always initialize to HIDDEN.
Revert the change from ad03c22e97 as
further testing has shown that with hyper-threading disabled, named with
ISC rwlocks outperforms named with pthread rwlocks in cold cache testing
scenarios. Since building named with pthread rwlocks might still be a
better choice for some workloads, keep the compile-time option which
enables that.
Add two tests that checks that dynamic zones
can be updated and will be signed appropriately.
One zone covers an update with freeze/thaw, the
other covers an update through nsupdate.
When dnssec-policy was introduced, it implicitly set inline-signing.
But DNSSEC maintenance required either inline-signing to be enabled,
or a dynamic zone. In other words, not in all cases you want to
DNSSEC maintain your zone with inline-signing.
Change the behavior and determine whether inline-signing is
required: if the zone is dynamic, don't use inline-signing,
otherwise implicitly set it.
You can also explicitly set inline-signing to yes with dnssec-policy,
the restriction that both inline-signing and dnssec-policy cannot
be set at the same time is now lifted.
However, 'inline-signing no;' on a non-dynamic zone with a
dnssec-policy is not possible.
The yamlget.py file was changed in !3311 as part of making the
python code pylint and flake8 compliant. This omitted setting
'item' to 'item[key]' which caused the digdelv yaml tests to fail.
Also, the pretty printing is not really necessary, so remove
the "if key not in item; print error" logic.
Change 5332 renamed "dnssec-keys" configuration statement to the
more descriptive "trust-anchors". Not all occurrences in the
documentation had been updated.
All our MSVS Project files share the same intermediate directory. We
know that this doesn't cause any problems, so we can just disable the
detection in the project files.
Example of the warning:
warning MSB8028: The intermediate directory (.\Release\) contains files shared from another project (dnssectool.vcxproj). This can lead to incorrect clean and rebuild behavior.
There was a missing indirection for the pluginlist_cb_t *callback in the
declaration of the cfg_pluginlist_foreach function. Reported by MSVC as:
lib\isccfg\parser.c(4057): warning C4028: formal parameter 4 different from declaration
Due to a way the stdatomic.h shim is implemented on Windows, the MSVC
always things that the outside type is the largest - atomic_(u)int_fast64_t.
This can lead to false positives as this one:
lib\dns\adb.c(3678): warning C4477: 'fprintf' : format string '%u' requires an argument of type 'unsigned int', but variadic argument 2 has type 'unsigned __int64'
We workaround the issue by loading the value in a scoped local variable
with correct type first.
MSVC documentation states: "This warning can be caused when a pointer to
a const or volatile item is assigned to a pointer not declared as
pointing to const or volatile."
Unfortunately, this happens when we dynamically allocate and deallocate
block of atomic variables using isc_mem_get and isc_mem_put.
Couple of examples:
lib\isc\hp.c(134): warning C4090: 'function': different 'volatile' qualifiers [C:\builds\isc-projects\bind9\lib\isc\win32\libisc.vcxproj]
lib\isc\hp.c(144): warning C4090: 'function': different 'volatile' qualifiers [C:\builds\isc-projects\bind9\lib\isc\win32\libisc.vcxproj]
lib\isc\stats.c(55): warning C4090: 'function': different 'volatile' qualifiers [C:\builds\isc-projects\bind9\lib\isc\win32\libisc.vcxproj]
lib\isc\stats.c(87): warning C4090: 'function': different 'volatile' qualifiers [C:\builds\isc-projects\bind9\lib\isc\win32\libisc.vcxproj]
The InterlockedOr8() and InterlockedAnd8() first argument was cast
to (atomic_int_fast8_t) instead of (atomic_int_fast8_t *), this was
reported by MSVC as:
warning C4024: '_InterlockedOr8': different types for formal and actual parameter 1
warning C4024: '_InterlockedAnd8': different types for formal and actual parameter 1
Our vcxproj files set the WarningLevel to Level3, which is too verbose
for a code that needs to be portable. That basically leads to ignoring
all the errors that MSVC produces. This commits downgrades the
WarningLevel to Level1 and enables treating warnings as errors for
Release builds. For the Debug builds the WarningLevel got upgraded to
Level4, and treating warnings as errors is explicitly disabled.
We should eventually make the code clean of all MSVC warnings, but it's
a long way to go for Level4, so it's more reasonable to start at Level1.
For reference[1], these are the warning levels as described by MSVC
documentation:
* /W0 suppresses all warnings. It's equivalent to /w.
* /W1 displays level 1 (severe) warnings. /W1 is the default setting
in the command-line compiler.
* /W2 displays level 1 and level 2 (significant) warnings.
* /W3 displays level 1, level 2, and level 3 (production quality)
warnings. /W3 is the default setting in the IDE.
* /W4 displays level 1, level 2, and level 3 warnings, and all level 4
(informational) warnings that aren't off by default. We recommend
that you use this option to provide lint-like warnings. For a new
project, it may be best to use /W4 in all compilations. This option
helps ensure the fewest possible hard-to-find code defects.
* /Wall displays all warnings displayed by /W4 and all other warnings
that /W4 doesn't include — for example, warnings that are off by
default.
* /WX treats all compiler warnings as errors. For a new project, it
may be best to use /WX in all compilations; resolving all warnings
ensures the fewest possible hard-to-find code defects.
1. https://docs.microsoft.com/en-us/cpp/build/reference/compiler-option-warning-level?view=vs-2019
Windows BIND releases produced by GitLab CI are built from Git
repositories, not from release tarballs, which means the "srcid" file is
not present in the top source directory when MSBuild is invoked. This
causes the Git commit hash for such builds to be set to "unset_id".
Enable win32utils/Configure to try determining the commit hash for a
build by invoking Git on the build host if the "srcid" file is not
present (which is what its Unix counterpart does).
Our python code didn't adhere to any coding standard. In this commit, we add
flame8 (https://pypi.org/project/flake8/), and pylint (https://www.pylint.org/).
There's couple of exceptions:
- ans.py scripts are not checked, nor fixed as part of this MR
- pylint's missing-*-docstring and duplicate-code checks have
been disabled via .pylintrc
Both exceptions should be removed in due time.
The assembly code generated by MSVC for at least some signed comparisons
involving atomic variables incorrectly uses unsigned conditional jumps
instead of signed ones. In particular, the checks in isc_log_wouldlog()
are affected in a way which breaks logging on Windows and thus also all
system tests involving a named instance. Work around the issue by
assigning the values returned by atomic_load_acquire() calls in
isc_log_wouldlog() to local variables before performing comparisons.
Resolve "DNS rebinding protection is ineffective when BIND is configured as a forwarding DNS server"
Closes#1574
See merge request isc-projects/bind9!3342
This test asserts that option "deny-answer-aliases" works correctly
when forwarding requests.
As a matter of example, the behavior expected for a forwarder BIND
instance, having an option such as deny-answer-aliases { "domain"; }
is that when forwarding a request for *.anything-but-domain, it is
expected that it will return SERVFAIL if any answer received has a CNAME
for "*.domain".
(cherry picked from commit 9bdb960a16a69997b08746e698b6b02c8dc6c795)
BIND wasn't honoring option "deny-answer-aliases" when configured to
forward queries.
Before the fix it was possible for nameservers listed in "forwarders"
option to return CNAME answers pointing to unrelated domains of the
original query, which could be used as a vector for rebinding attacks.
The fix ensures that BIND apply filters even if configured as a forwarder
instance.
(cherry picked from commit af6a4de3d5ad6c1967173facf366e6c86b3ffc28)
Increate the DNSKEY TTL of the migrate.kasp zone for the following
reason: The key states are initialized depending on the timing
metadata. If a key is present long enough in the zone it will be
initialized to OMNIPRESENT. Long enough here is the time when it
was published (when the setup script was run) plus DNSKEY TTL.
Otherwise it is set to RUMOURED, or to HIDDEN if no timing metadata
is set or the time is still in the future.
Since the TTL is "only" 5 minutes, the DNSKEY state may be
initialized to OMNIPRESENT if the test is slow, but we expect it
to be in RUMOURED state. If we increase the TTL to a couple of
hours it is very unlikely that it will be initialized to something
else than RUMOURED.
This fixes another intermittent failure in the kasp system test.
It does not happen often, except for in the Windows platform tests
where it takes a long time to run the tests.
In the "kasp" system test, there is an "rndc reconfig" call which
triggers a new rekey event. check_next_key_event() verifies the time
remaining from the moment "rndc reconfig" is called until the next key
event. However, the next key event time is calculated from the key
times provided during key creation (i.e. during test setup). Given
this, if "rndc reconfig" is called a significant amount of time after
the test is started, some check_next_key_event() checks will fail.
Fix by calculating the time passed since the start of the test and
when 'rndc reconfig' happens. Substract this time from the
calculated next key event.
This only needs to be done after an "rndc reconfig" on zones where
the keymgr needs to wait for a period of time (for example for keys
to become OMNIPRESENT, or HIDDEN). This is on step 2 and step 5 of
the algorithm rollover. In step 2 there is a waiting period before
the DNSKEY is OMNIPRESENT, in step 5 there is a waiting period
before the DNSKEY is HIDDEN.
In step 1 new keys are created, in step 3 and 4 key states just
entered OMNIPRESENT, and in step 6 we no longer care because the
key lifetime is unlimited and we default to checking once per hour.
Regardless of our indifference about the next key event after step 6,
change some of the key timings in the setup script to better
reflect reality: DNSKEY is in HIDDEN after step 5, DS times have
changed when the new DS became active.
In case of normal fetch, the .recursionquota is attached and
ns_statscounter_recursclients is incremented when the fetch is created. Then
the .recursionquota is detached and the counter decremented in the
fetch_callback().
In case of prefetch or rpzfetch, the quota is attached, but the counter is not
incremented. When we reach the soft-quota, the function returns early but don't
detach from the quota, and it gets destroyed during the ns_client_endrequest(),
so no memory was leaked.
But because the ns_statscounter_recursclients is only incremented during the
normal fetch the counter would be incorrectly decremented on two occassions:
1) When we reached the softquota, because the quota was not properly detached
2) When the prefetch or rpzfetch was cancelled mid-flight and the callback
function was never called.
Rather than group key ids together, group key id with its
corresponding counters. This should make growing / shrinking easier
than having keyids then counters.
Add a statschannel test case for DNSSEC sign metrics that has more
keys than there are allocated stats counters for. This will produce
gibberish, but at least it should not crash.
The first attempt to add DNSSEC sign statistics was naive: for each
zone we allocated 64K counters, twice. In reality each zone has at
most four keys, so the new approach only has room for four keys per
zone. If after a rollover more keys have signed the zone, existing
keys are rotated out.
The DNSSEC sign statistics has three counters per key, so twelve
counters per zone. First counter is actually a key id, so it is
clear what key contributed to the metrics. The second counter
tracks the number of generated signatures, and the third tracks
how many of those are refreshes.
This means that in the zone structure we no longer need two separate
references to DNSSEC sign metrics: both the resign and refresh stats
are kept in a single dns_stats structure.
Incrementing dnssecsignstats:
Whenever a dnssecsignstat is incremented, we look up the key id
to see if we already are counting metrics for this key. If so,
we update the corresponding operation counter (resign or
refresh).
If the key is new, store the value in a new counter and increment
corresponding counter.
If all slots are full, we rotate the keys and overwrite the last
slot with the new key.
Dumping dnssecsignstats:
Dumping dnssecsignstats is no longer a simple wrapper around
isc_stats_dump, but uses the same principle. The difference is that
rather than dumping the index (key tag) and counter, we have to look
up the corresponding counter.
Resolve "Changing from auto-dnssec maintain to dnssec-policy x immediately deletes existing keys"
Closes#1706
See merge request isc-projects/bind9!3322
Add a test to ensure migration from 'auto-dnssec maintain;' to
dnssec-policy works even if the algorithm is changed. The existing
keys should not be removed immediately, but their goal should be
changed to become hidden, and the new keys with the different
algorithm should be introduced immediately.
If we initialize goals on all keys, superfluous keys that match
the policy all desire to be active. For example, there are six
keys available for a policy that needs just two, we only want to
set the goal state to OMNIPRESENT on two keys, not six.
Migrating from 'auto-dnssec maintain;' to dnssec-policy did not
work properly, mainly because the legacy keys were initialized
badly. Earlier commit deals with migration where existing keys
match the policy. This commit deals with migration where existing
keys do not match the policy. In that case, named must not
immediately delete the existing keys, but gracefully roll to the
dnssec-policy.
However, named did remove the existing keys immediately. This is
because the legacy key states were initialized badly. Because
those keys had their states initialized to HIDDEN or RUMOURED, the
keymgr decides that they can be removed (because only when the key
has its states in OMNIPRESENT it can be used safely).
The original thought to initialize key states to HIDDEN (and
RUMOURED to deal with existing keys) was to ensure that those keys
will go through the required propagation time before the keymgr
decides they can be used safely. However, those keys are already
in the zone for a long time and making the key states represent
otherwise is dangerous: keys may be pulled out of the zone while
in fact they are required to establish the chain of trust.
Fix initializing key states for existing keys by looking more closely
at the time metadata. Add TTL and propagation delays to the time
metadata and see if the DNSSEC records have been propagated.
Initialize the state to OMNIPRESENT if so, otherwise initialize to
RUMOURED. If the time metadata is in the future, or does not exist,
keep initializing the state to HIDDEN.
The added test makes sure that new keys matching the policy are
introduced, but existing keys are kept in the zone until the new
keys have been propagated.
A few kasp system test tweaks to improve test failure debugging and
deal with tests related to migration to dnssec-policy.
1. When clearing a key, set lifetime to "none". If "none", skip
expect no lifetime set in the state file. Legacy keys that
are migrated but don't match the dnssec-policy will not have a
lifetime.
2. The kasp system test prints which key id and file it is checking.
Log explicitly if we are checking the id or a file.
3. Add quotes around "ID" when setting the key id, for consistency.
4. Fix a typo (non -> none).
5. Print which key ids are found, this way it is easier to see what
KEY[1-4] failed to match one of the key files.
Migrating from 'auto-dnssec maintain;' to dnssec-policy did not
work properly, mainly because the legacy keys were initialized
badly. Several adjustments in the keymgr are required to get it right:
- Set published time on keys when we calculate prepublication time.
This is not strictly necessary, but it is weird to have an active
key without the published time set.
- Initalize key states also before matching keys. Determine the
target state by looking at existing time metadata: If the time
data is set and is in the past, it is a hint that the key and
its corresponding records have been published in the zone already,
and the state is initialized to RUMOURED. Otherwise, initialize it
as HIDDEN. This fixes migration to dnssec-policy from existing
keys.
- Initialize key goal on keys that match key policy to OMNIPRESENT.
These may be existing legacy keys that are being migrated.
- A key that has its goal to OMNIPRESENT *or* an active key can
match a kasp key. The code was changed with CHANGE 5354 that
was a bugfix to prevent creating new KSK keys for zones in the
initial stage of signing. However, this caused problems for
restarts when rollovers are in progress, because an outroducing
key can still be an active key.
The test for this introduces a new KEY property 'legacy'. This is
used to skip tests related to .state files.
The rwlock introduced to protect the .logconfig member of isc_log_t
structure caused a significant performance drop because of the rwlock
contention. It was also found, that the debug_level member of said
structure was not protected from concurrent read/writes.
The .dynamic and .highest_level members of isc_logconfig_t structure
were actually just cached values pulled from the assigned channels.
We introduced an even higher cache level for .dynamic and .highest_level
members directly into the isc_log_t structure, so we don't have to
access the .logconfig member in the isc_log_wouldlog() function.
After an RPZ zone is updated via zone transfer, the RPZ summary
database is updated, inserting the newly added names in the policy
zone and deleting the newly removed ones. The first part of this
was quantized so it would not run too long and starve other tasks
during large updates, but the second part was not quantized, so
that an update in which a large number of records were deleted
could cause named to become briefly unresponsive.
We could have a race between handle closing and processing async
callback. Deactivate the handle before issuing the callback - we
have the socket referenced anyway so it's not a problem.
We introduce a isc_quota_attach_cb function - if ISC_R_QUOTA is returned
at the time the function is called, then a callback will be called when
there's quota available (with quota already attached). The callbacks are
organized as a LIFO queue in the quota structure.
It's needed for TCP client quota - with old networking code we had one
single place where tcp clients quota was processed so we could resume
accepting when the we had spare slots, but it's gone with netmgr - now
we need to notify the listener/accepter that there's quota available so
that it can resume accepting.
Remove unused isc_quota_force() function.
The isc_quote_reserve and isc_quota_release were used only internally
from the quota.c and the tests. We should not expose API we are not
using.
ORACLE MySQL 8.0 has dropped the my_bool type, so we need to reinstate
it back when compiling with that version or higher. MariaDB is still
keeping the my_bool type. The numbering between the two (MariaDB 5.x
jumped to MariaDB 10.x) doesn't make the life of the developer easy.
Most build/test job names already contain a "clang", "gcc", or "msvc"
prefix which indicates the compiler used for a given job. Apply that
naming convention to all build/test job names.
Multiple YAML keys have identical values for both TSAN unit test job
definitions. Extract these common keys to a YAML anchor and use it in
TSAN unit test job definitions to reduce code duplication.
Definitions of jobs running unit tests under TSAN contain an
"after_script" YAML key. Since the "unit_test_job" anchor is included
in those job definitions before "after_script" is defined, the
job-specific value of that key overrides the one defined in the included
anchor. This prevents "kyua report-html" from being run for TSAN unit
test jobs. Moving the invocation of "kyua report-html" to the "script"
key in the "unit_test_job" anchor is not acceptable as it would cause
the exit code of that command to determine the result of all unit test
jobs and we need that to be the exit code of "make unit". Instead, add
"kyua report-html" invocations to the "after_script" key of TSAN unit
test job definitions to address the problem without affecting other job
definitions.
Multiple YAML keys have identical values for both TSAN system test job
definitions. Extract these common keys to a YAML anchor and use it in
TSAN system test job definitions to reduce code duplication.
Both "system_test_job" and "unit_test_job" YAML anchors contain a
"before_script" key. TSAN job definitions first specify their own value
of the "before_script" key and then include the aforementioned YAML
anchors, which results in the value of the "before_script" key being
overridden with the value specified by the included anchor. Given this,
remove "before_script" definitions specific to TSAN jobs as they serve
no practical purpose.
All assignments for the TSAN_OPTIONS variable are identical across the
entire .gitlab-ci.yml file. Define a global TSAN_OPTIONS_COMMON
variable and use it in job definitions to reduce code duplication.
The custom builds (oot, asan, tsan) were mostly built using Debian sid
amd64 image. The problem was that this image broke too easily, because
it's Debian "unstable" after all.
This commit introduces "base_image" that should be most stable with
extra bits on top (clang, coccinelle, cppcheck, ...). Currently, that
would be Debian buster amd64.
Other changes introduced by this commit:
* Change the default clang version to 10
* Run both ASAN and TSAN with both gcc and clang compilers
* Remove Clang Debian stretch i386 job
These are mostly false positives, the clang-analyzer FAQ[1] specifies
why and how to fix it:
> The reason the analyzer often thinks that a pointer can be null is
> because the preceding code checked compared it against null. So if you
> are absolutely sure that it cannot be null, remove the preceding check
> and, preferably, add an assertion as well.
The 4 warnings reported are:
dnssec-cds.c:781:4: warning: Access to field 'base' results in a dereference of a null pointer (loaded from variable 'buf')
isc_buffer_availableregion(buf, &r);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/builds/isc-projects/bind9/lib/isc/include/isc/buffer.h:996:36: note: expanded from macro 'isc_buffer_availableregion'
^
/builds/isc-projects/bind9/lib/isc/include/isc/buffer.h:821:16: note: expanded from macro 'ISC__BUFFER_AVAILABLEREGION'
(_r)->base = isc_buffer_used(_b); \
^~~~~~~~~~~~~~~~~~~
/builds/isc-projects/bind9/lib/isc/include/isc/buffer.h:152:29: note: expanded from macro 'isc_buffer_used'
((void *)((unsigned char *)(b)->base + (b)->used)) /*d*/
^~~~~~~~~
1 warning generated.
--
byname_test.c:308:34: warning: Access to field 'fwdtable' results in a dereference of a null pointer (loaded from variable 'view')
RUNTIME_CHECK(dns_fwdtable_add(view->fwdtable, dns_rootname,
^~~~~~~~~~~~~~
/builds/isc-projects/bind9/lib/isc/include/isc/util.h:318:52: note: expanded from macro 'RUNTIME_CHECK'
^~~~
/builds/isc-projects/bind9/lib/isc/include/isc/error.h:50:21: note: expanded from macro 'ISC_ERROR_RUNTIMECHECK'
((void)(ISC_LIKELY(cond) || \
^~~~
/builds/isc-projects/bind9/lib/isc/include/isc/likely.h:23:43: note: expanded from macro 'ISC_LIKELY'
^
1 warning generated.
--
./rndc.c:255:6: warning: Dereference of null pointer (loaded from variable 'host')
if (*host == '/') {
^~~~~
1 warning generated.
--
./main.c:1254:9: warning: Access to field 'sctx' results in a dereference of a null pointer (loaded from variable 'named_g_server')
sctx = named_g_server->sctx;
^~~~~~~~~~~~~~~~~~~~
1 warning generated.
References:
1. https://clang-analyzer.llvm.org/faq.html#null_pointer
The 3 warnings reported are:
os.c:872:7: warning: Although the value stored to 'ptr' is used in the enclosing expression, the value is never actually read from 'ptr'
if ((ptr = strtok_r(command, " \t", &last)) == NULL) {
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
--
rpz.c:1117:10: warning: Although the value stored to 'zbits' is used in the enclosing expression, the value is never actually read from 'zbits'
return (zbits &= x);
^ ~
1 warning generated.
--
openssleddsa_link.c:532:10: warning: Although the value stored to 'err' is used in the enclosing expression, the value is never actually read from 'err'
while ((err = ERR_get_error()) != 0) {
^ ~~~~~~~~~~~~~~~
1 warning generated.
tcpdns used transport-specific functions to operate on the outer socket.
Use generic ones instead, and select the proper call in netmgr.c.
Make the missing functions (e.g. isc_nm_read) generic and add type-specific
calls (isc__nm_tcp_read). This is the preparation for netmgr TLS layer.
There are several reason why remove Debian 8 from the CI:
* Debian 8 ("jessie") has been superseded by Debian 9 ("stretch").
* Regular security support updates have been discontinued as of
June 17th, 2018.
* Jessie LTS is supported from 17th June 2018 to June 30, 2020.
In other words, it's no longer officially supported by Debian security
team, but by the volunteer/paid contributor composed LTS team. And the
release will be discontinued in three months from now. We can use the
freed CI resources to bring new platforms or just to make the jobs run a
bit faster.
The reference BIND version used in the ABI check CI job is not
determined automatically - it needs to be updated after each BIND
release. Reflect that fact in the release checklist to make sure the
ABI check CI job is always comparing current code with the latest BIND
release on a given branch.
The files configure.ac and version are already up to date.
Updated CHANGES with 9.17.0 release line.
Fixed CHANGES by adding GitLab reference to entry 5357 and fix
grammar mistakes.
Add missing /util/check-make-install.in to .gitattributes.
The lib/*/api are already updated to match the new ranges.
I listed two new features under BIND 9.17 features that to me
seemed noteworthy.
The release notes look good to me.
When unit test fails, core file is created. Kyua's 'debug' command can
run GDB on it and provide backtrace. Unfortunately Kyua is picky about
location of these core files we opt to use custom Kyua fork and copy
core files from Kyua working directory to source tree and make it
available in GitLab.
The tkey test was not adapted to dynamic ports, so we had to run it in
sequence. This commit adds support for dynamic ports, and also makes
all the scripts shellcheck clean.
The eddsa test was not adapted to dynamic ports, so we had to run it in
sequence. This commit adds support for dynamic ports, and also makes
all the scripts shellcheck clean.
The ecdsa test was not adapted to dynamic ports, so we had to run it in
sequence. This commit adds support for dynamic ports, and also makes
all the scripts shellcheck clean.
The environment variable MAKE has been replaced with MAKE_COMMAND,
because overriding MAKE variable also changed the definition of the MAKE
inside the Makefiles, and we want only a single wrapper around the whole
build process.
Previously, setting `MAKE` to `bear make` meant that `bear make` would
be run at every nested make invocation, which messed up the upcoming
automake transition as compile_commands.json would be generated in every
subdirectory instead of just having one central file at the top of the
build tree.
All jobs now use solely the newer needs configuration to declare
dependencies between jobs:
needs:
- job: <foo>
artifacts: true
instead of combination of dependencies and needs which is deprecated.
This change completely unbundles the stages (alas the stages still needs
to stay because the job graph has to stay acyclic between the stages).
In isc_log_woudlog() the .logconfig member of isc_log_t structure was
accessed unlocked on the merit that there could be just a race when
.logconfig would be NULL, so the message would not be logged. This
turned not to be true, as there's also data race deeper. The accessed
isc_logconfig_t object could be in the middle of destruction, so the
pointer would be still non-NULL, but the structure members could point
to a chunk of memory no longer belonging to the object. Since we are
only accessing integer types (the log level), this would never lead to
a crash, it leads to memory access to memory area no longer belonging to
the object and this a) wrong, b) raises a red flag in thread-safety tools.
The isc_mem API now crashes on memory allocation failure, and this is
the next commit in series to cleanup the code that could fail before,
but cannot fail now, e.g. isc_result_t return type has been changed to
void for the isc_log API functions that could only return ISC_R_SUCCESS.
On Windows, C11 localtime_r() and gmtime_r() functions are not
available. While localtime() and gmtime() functions are already thread
safe because they use Thread Local Storage, it's quite ugly to #ifdef
around every localtime_r() and gmtime_r() usage to make the usage also
thread-safe on POSIX platforms.
The commit adds wrappers around Windows localtime_s() and gmtime_s()
functions.
NOTE: The implementation of localtime_s and gmtime_s in Microsoft CRT
are incompatible with the C standard since it has reversed parameter
order and errno_t return type.
The <isc/md.h> header directly included <openssl/evp.h> header which
enforced all users of the libisc library to explicitly list the include
path to OpenSSL and link with -lcrypto. By hiding the specific
implementation into the private namespace, we no longer enforce this.
In the long run, this might also allow us to switch cryptographic
library implementation without affecting the downstream users.
While making the isc_md_type_t type opaque, the API using the data type
was changed to use the pointer to isc_md_type_t instead of using the
type directly.
This new option was added to fill a gap in RPZ configuration
options.
It was possible to instruct BIND wheter NSIP rewritting rules would
apply or not, as long as the required data was already in cache or not,
respectively, by means of the option nsip-wait-recurse.
A value of yes (default) could incur a little processing cost, since
BIND would need to recurse to find NS addresses in case they were not in
the cache.
This behavior could be changed by setting nsip-wait-recurse value to no,
in which case BIND would promptly return some error code if the NS IP addresses
data were not in cache, then BIND would start a recursive query
in background, so future similar requests would have the required data
(NS IPs) in cache, allowing BIND to apply NSIP rules accordingly.
A similar feature wasn't available for NSDNAME triggers, so this commit
adds the option nsdname-wait-recurse to fill this gap, as it was
expected by couple BIND users.
To get rid of the currently used FreeBSD-specific executor, move FreeBSD
CI jobs to libvirt-based executors. Make the necessary tag and variable
adjustments.
Waiting for the reply message will ensure that all messages being
looked for exist in the logs at the time of checking. When the
test was only waiting for the send message there was a race between
grep and the ns1 instance of named logging that it had seen the
request.
Save 'i' to 'locknum' and use that rather than using
'header->node->locknum' when performing the deferred
unlock as 'header->node->locknum' can theoretically be
different to 'i'.
The <isc/md.h> header directly included <openssl/hmac.h> header which
enforced all users of the libisc library to explicitly list the include
path to OpenSSL and link with -lcrypto. By hiding the specific
implementation into the private namespace, we no longer enforce this.
In the long run, this might also allow us to switch cryptographic
library implementation without affecting the downstream users.
The two "functions" that isc/safe.h declared before were actually simple
defines to matching OpenSSL functions. The downside of the approach was
enforcing all users of the libisc library to explicitly list the include
path to OpenSSL and link with -lcrypto. By hiding the specific
implementation into the private namespace changing the defines into
simple functions, we no longer enforce this. In the long run, this
might also allow us to switch cryptographic library implementation
without affecting the downstream users.
The previous commit removed the code related to the internal symbol
table. On platforms where available, we can now use backtrace_symbols()
to print more verbose symbols table to the output.
As there's now general availability of backtrace() and
backtrace_symbols() functions (see below), the commit also removes the
usage of glibc internals and the custom stack tracing.
* backtrace(), backtrace_symbols(), and backtrace_symbols_fd() are
provided in glibc since version 2.1.
* backtrace(), backtrace_symbols(), and backtrace_symbols_fd() first
appeared in Mac OS X 10.5.
* The backtrace() library of functions first appeared in NetBSD 7.0 and
FreeBSD 10.0.
The kasp system test is timing critical. The test passes on all
Linux based machines, but fails frequently on Windows. The test
takes a lot more time on Windows and at the final checks fail
because the expected next key event is too far off. For example:
I:kasp:check next key event for zone step2.algorithm-roll.kasp (570)
I:kasp:error: bad next key event time 20909 for zone \
step2.algorithm-roll.kasp (expect 21600)
I:kasp:failed
This is because the kasp system test calculates the time when the
next key event should occur based on the policy. This assumes that
named is able to do key management within a minute. But starting,
named, doing key management for other zones, and reconfiguring takes
much more time on Windows and thus the next key event on Windows is
much shorter than anticipated.
That this happens is a good thing because this means that the
correct next key event is used, but is not so nice for testing, as
it is hard to determine how much time named needed before finishing
the current key event.
Disable the kasp test on Windows now because it is blocking the
release. We know the cause of these test failures, and it is clear
that this is a fault in the test, not the code. Therefore we feel
comfortable disabling the test right now and work on a fix while
unblocking the release.
A data race was happening while BIND was starting due to
isc_log_wouldlog function accessing lctx->logconfig without a lock.
To prevent that without incurring much costs, that variable was made
atomic.
ABI checker tools generate HTML and TXT API compatibility reports of
BIND libraries. Comparison is being done between two bind source trees
which hold built BIND.
In the CI one version is the reference version defined by
BIND_BASELINE_VERSION variable, the latter one is the HEAD of branch
under test.
When configuring the same dnssec-policy for two zones with the same
name but in different views, there is a race condition for who will
run the keymgr first. If running sequential only one set of keys will
be created, if running parallel two set of keys will be created.
Lock the kasp when running looking for keys and running the key
manager. This way, for the same zone in different views only one
keyset will be created.
The dnssec-policy does not implement sharing keys between different
zones.
OpenBSD virtual machines seem to affected particularly badly by other
activity happening on the host. This causes trouble around release
time: when multiple tags are pushed to the repository, a large number of
jobs is started concurrently on all CI runners. In extreme cases, this
causes the system test suite to run for about an hour (!) on OpenBSD
VMs, with multiple tests failing. We investigated the test artifacts
for all such cases in the past and the outcome was always the same: test
failures were caused by extremely slow I/O on the guest. We tried
various tricks to work around this problem, but nothing helped.
Given the above, stop running OpenBSD system test jobs for pending BIND
releases to prevent the results of these jobs from affecting the
assessment of a given release's readiness for publication. This change
does not affect OpenBSD build jobs. OpenBSD system test jobs will still
be run for scheduled and web-requested pipelines, to make sure we catch
any severe issues with test code on that platform sooner or later.
Some comments started with a lowercased letter. Capitalized them to
be more consistent with the rest of the comments.
Add some newlines between `set_*` calls and check calls, also to be
more consistent with the other test cases.
There is a failure mode which gets triggered on heavily loaded
systems. A key change is scheduled in 5 seconds to make ZSK2 inactive
and ZSK3 active, but `named` takes more than 5 seconds to progress
from `rndc loadkeys` to the query check. At this time the SOA RRset
is already signed by the new ZSK which is not expected to be active
at that point yet.
Split up the checks to test the case where RRsets are signed
correctly with the offline KSK (maintained the signature) and
the active ZSK. First run, RRsets should be signed with the still
active ZSK2, second run RRsets should be signed with the new active
ZSK3.
This commit fixes isc_glob function on windows environments.
The file_list_t * object pointed to by pglob->reserved was missing
ISC_LIST_INIT intialization macro.
We may be checking the algorithm steps too fast: the reconfig
command may still be in progress. Make sure the zones are signed
and loaded by digging the NSEC records for these zones.
Algorithm rollover waited too long before introducing zone
signatures. It waited to make sure all signatures were resigned,
but when introducing a new algorithm, all signatures are resigned
immediately. Only add the sign delay if there is a predecessor key.
Algorithm rollover was stuck on submitting DS because keymgr thought
it would move to an invalid state. It did not match the current
key because it checked it against the current key in the next state.
Fixed by when checking the current key, check it against the desired
state, not the existing state.
Add a test case for algorithm rollover. This is triggered by
changing the dnssec-policy. A new nameserver ns6 is introduced
for tests related to dnssec-policy changes.
This requires a slight change in check_next_key_event to only
check the last occurrence. Also, change the debug log message in
lib/dns/zone.c to deal with checks when no next scheduled key event
exists (and default to loadkeys interval 3600).
Algorithm rollover will require four keys so introduce KEY4.
Also it requires to look at key files for multiple algorithms so
change getting key ids to be algorithm rollover agnostic (adjusting
count checks). The algorithm will be verified in check_key so
relaxing 'get_keyids' is fine.
Replace '${_alg_num}' with '$(key_get KEY[1-4] ALG_NUM)' in checks
to deal with multiple algorithms.
HAVE_UV_IMPORT and other config.h macros must not be set unconditionally
because no existing libuv release exposes uv_import() and/or uv_export()
yet. Windows builds not passing an explicit path to libuv to
win32utils/Configure are currently broken because of this, so comment
out the offending lines and describe when the aforementioned config.h
macros should be set.
Send AXFR instead of requested IXFR if the size of the incremental transfer is too large to sensibly IXFR
Closes#1375 and #1515
See merge request isc-projects/bind9!3113
- change name of 'bytes' to 'xfrsize' in dns_db_getsize() parameter list
and related variables; this is a more accurate representation of what
the function is doing
- change the size calculations in dns_db_getsize() to more accurately
represent the space needed for a *XFR message or journal file to contain
the data in the database. previously we returned the sizes of all
rdataslabs, including header overhead and offset tables, which
resulted in the database size being reported as much larger than the
equivalent *XFR or journal.
- map files caused a particular problem here: the fullname can't be
determined from the node while a file is being deserialized, because
the uppernode pointers aren't set yet. so we store "full name length"
in the dns_rbtnode structure while serializing, and clear it after
deserialization is complete.
the call initailizing a journal iterator can now optionally return
to the caller the size in bytes of an IXFR message (not including
DNS header overhead, signatures etc) containing the differences from
the beginning to the ending serial number.
this is calculated by scanning the journal transaction headers to
calculate the transfer size. since journal file records contain a length
field that is not included in IXFR messages, we subtract out the length
of those fields from the overall transaction length.
this necessitated adding an "RR count" field to the journal transaction
header, so we know how many length fields to subract. NOTE: this will
make existing journal files stop working!
Fixes a race between ns_client_killoldestquery and ns_client_endrequest -
killoldestquery takes a client from `recursing` list while endrequest
destroys client object, then killoldestquery works on a destroyed client
object. Prevent it by holding reclist lock while cancelling query.
- Define the SLOT environment variable before starting the test. This
variable defaults to 0 and that does not work with SoftHSM 2.
- The system test expects the PIN environment variable to be set to
"1234" while bin/tests/prepare-softhsm2.sh sets it to "0000".
Update bin/tests/prepare-softhsm2.sh so that it sets the PIN to
"1234".
- Move contents of bin/tests/system/pkcs11/prereq.sh to
bin/tests/system/pkcs11/setup.sh as the former was creating a file
called "supported" that was getting removed by the latter before
bin/tests/system/pkcs11/tests.sh could access it.
- Fix typo in "have_ecx".
- no longer exclude these entries when dumping the NTA table
- indicate "validate-except" entries with the keyword "permanent" in
place of an expiry date
- add a test for this feature, and update other tests to account for
the presence of extra lines in some rndc outputs
- incidentally removed the unused function dns_ntatable_dump()
- CHANGES, release note
There was a very slim chance of a race between isc_socket_detach and
process_fd: isc_socket_detach decrements references to 0, and before it
calls destroy gets preempted. Second thread calls process_fd, increments
socket references temporarily to 1, and then gets preempted, first thread
then hits assertion in destroy() as the reference counter is now 1 and
not 0.
With RRSIG records no longer being signed with the full
sig-validity-interval we need to ensure the zone->resigntime
as it may need to be set to a earlier time.
Previously badcache used one single mutex for everything, which
was causing performance issues. Use one global rwlock for the whole
hashtable and per-bucket mutexes.
When --with-zlib is passed to ./configure (or when the latter
autodetects zlib's presence), libisc uses certain zlib functions and
thus libisc's users should be linked against zlib in that case. Adjust
Makefile variables appropriately to prevent shared build failures caused
by underlinking.
mem.c:add_trace_entry() -> isc_hash_function() -> isc_siphash24()
129 for (; in != end; in += 8) {
6. byte_swapping: Performing a byte swapping operation on
in implies that it came from an external source, and is
therefore tainted.
130 uint64_t m = U8TO64_LE(in);
sending each group of queries simultaneously, and then checking the
output after the last one finishes, reduces the runtime of the
serve-stale test by about six minutes.
"yes" and "no" are permissible synonyms for "on" and "off", which
use exactly the same code paths. making sure they work isn't a good
use of 80 seconds of test time.
Fix a potential assertion failure on shutdown in ns__client_endrequest.
Scenario:
1. We are shutting down, interface->clientmgr is gone.
2. We receive a packet, it gets through ns__client_request
3. mgr == NULL, return
4. isc_nmhandle_detach calls ns_client_reset_cb
5. ns_client_reset_cb calls ns_client_endrequest
6. INSIST(client->state == NS_CLIENTSTATE_WORKING ||
client->state == NS_CLIENTSTATE_RECURSING) is not met
- we haven't started processing this packet so
client->state == NS_CLIENTSTATE_READY.
As a solution - don't do anything in ns_client_reset_cb if the client
is still in READY state.
- the configuration summary reported zlib compression was not
supported even when it was.
- when bind.keys.h was regenerated it violated clang-format style.
The change introduced by commit be159f5565
was not fully complete. Adjust ./configure summary so that it reflects
the new way the --with-tuning switch works, fixing the Autoconf variable
used for determining the value of that switch. Fix win32utils/Configure
so that it behaves the same way as its Unix counterpart.
* ctx needs to be destroyed before it is regenerated.
* emit the name of the signature to be replaced.
* cleanup memory before asserting so post longjump doesn't detect a
memory leak.
* comment code.
If a filename (the last argument) is not provided for named-checkzone or
named-compilezone, or if it is a single dash "-" character,
zone data will be read from stdin.
Example of invocation:
cat /etc/zone_name.db | named-compilezone -f text -F raw \
-o zone_name.raw zone_name
BSD sed does not recognize \s as a whitespace matching token. Make the
sed script in doc/arm/Makefile.in which ensures GitLab identifiers are
not split across lines portable by replacing \s with [[:space:]].
Artifacts generated by the docs:sid:amd64 job need to be retained longer
than for other jobs as they are used for building bind.isc.org contents.
If these artifacts are removed too quickly, pipelines in the pages/bind
GitLab project start failing, preventing content updates from being
published. Increase lifetime of the relevant job artifacts to prevent
this from happening.
We were using our own versions of isc_uv_{export,import} functions
for multithreaded TCP listeners. Upcoming libuv version will
contain proper uv_{export,import} functions - use them if they're
available.
Upcoming version of libuv will suport uv_recvmmsg and uv_sendmmsg. To
use uv_recvmmsg we need to provide a larger buffer and be able to
properly free it.
isc_task_pause/unpause were inherently thread-unsafe - a task
could be paused only once by one thread, if the task was running
while we paused it it led to races. Fix it by making sure that
the task will pause if requested to, and by using a 'pause reference
counter' to count task pause requests - a task will be unpaused
iff all threads unpause it.
Don't remove from queue when pausing task - we lock the queue lock
(expensive), while it's unlikely that the task will be running -
and we'll remove it anyway in dispatcher
this corrects some style glitches such as:
```
long_function_call(arg, arg2, arg3, arg4, arg5, "str"
"ing");
```
...by adjusting the penalties for breaking strings and call
parameter lists.
While testing BIND 9 on arm64 8+ core machine, it was discovered that
the weak variants in fact does spuriously fail, we haven't observed that
on other architectures.
This commit replaces all non-loop usage of atomic_compare_exchange_weak
with atomic_compare_exchange_strong.
This commit simplifies a bit the lock management within dns_resolver_prime()
and prime_done() functions by means of turning resolver's attribute
"priming" into an atomic_bool and by creating only one dependent object on the
lock "primelock", namely the "primefetch" attribute.
By having the attribute "priming" as an atomic type, it save us from having to
use a lock just to test if priming is on or off for the given resolver context
object, within "dns_resolver_prime" function.
The "primelock" lock is still necessary, since dns_resolver_prime() function
internally calls dns_resolver_createfetch(), and whenever this function
succeeds it registers an event in the task manager which could be called by
another thread, namely the "prime_done" function, and this function is
responsible for disposing the "primefetch" attribute in the resolver object,
also for resetting "priming" attribute to false.
It is important that the invariant "priming == false AND primefetch == NULL"
remains constant, so that any thread calling "dns_resolver_prime" knows for sure
that if the "priming" attribute is false, "primefetch" attribute should also be
NULL, so a new fetch context could be created to fulfill this purpose, and
assigned to "primefetch" attribute under the lock protection.
To honor the explanation above, dns_resolver_prime is implemented as follow:
1. Atomically checks the attribute "priming" for the given resolver context.
2. If "priming" is false, assumes that "primefetch" is NULL (this is
ensured by the "prime_done" implementation), acquire "primelock"
lock and create a new fetch context, update "primefetch" pointer to
point to the newly allocated fetch context.
3. If "priming" is true, assumes that the job is already in progress,
no locks are acquired, nothing else to do.
To keep the previous invariant consistent, "prime_done" is implemented as follow:
1. Acquire "primefetch" lock.
2. Keep a reference to the current "primefetch" object;
3. Reset "primefetch" attribute to NULL.
4. Release "primefetch" lock.
5. Atomically update "priming" attribute to false.
6. Destroy the "primefetch" object by using the temporary reference.
This ensures that if "priming" is false, "primefetch" was already reset to NULL.
It doesn't make any difference in having the "priming" attribute not protected
by a lock, since the visible state of this variable would depend on the calling
order of the functions "dns_resolver_prime" and "prime_done".
As an example, suppose that instead of using an atomic for the "priming" attribute
we employed a lock to protect it.
Now suppose that "prime_done" function is called by Thread A, it is then preempted
before acquiring the lock, thus not reseting "priming" to false.
In parallel to that suppose that a Thread B is scheduled and that it calls
"dns_resolver_prime()", it then acquires the lock and check that "priming" is true,
thus it will consider that this resolver object is already priming and it won't do
any more job.
Conversely if the lock order was acquired in the other direction, Thread B would check
that "priming" is false (since prime_done acquired the lock first and set "priming" to false)
and it would initiate a priming fetch for this resolver.
An atomic variable wouldn't change this behavior, since it would behave exactly the
same, depending on the function call order, with the exception that it would avoid
having to use a lock.
There should be no side effects resulting from this change, since the previous
implementation employed use of the more general resolver's "lock" mutex, which
is used in far more contexts, but in the specifics of the "dns_resolver_prime"
and "prime_done" it was only used to protect "primefetch" and "priming" attributes,
which are not used in any of the other critical sections protected by the same lock,
thus having zero dependency on those variables.
- add util/cformat.sh, which runs clang-format on all C files with
the default .clang-format, and on all header files with a slightly
modified version.
- use correct bracing after multi-line control statements
- stop aligning variable declarations to avoid problems with pointer
alignment, but retain aligned declarations in header files so that
struct definitions look cleaner.
- static function prototypes in C files can skip the line break after
the return type, but function prototypes in header files still have
the line break.
- don't break-before-brace in function definitions. ISC style calls
for braces on the same line when function parameters fit on a single
line, and a line break if they don't, but clang-format doesn't yet
support that distinction. one-line function definitions are about
four times more common than multi-line, so let's use the option that
deviates less.
Both clang-tidy and uncrustify chokes on statement like this:
for (...)
if (...)
break;
This commit uses a very simple semantic patch (below) to add braces around such
statements.
Semantic patch used:
@@
statement S;
expression E;
@@
while (...)
- if (E) S
+ { if (E) { S } }
@@
statement S;
expression E;
@@
for (...;...;...)
- if (E) S
+ { if (E) { S } }
@@
statement S;
expression E;
@@
if (...)
- if (E) S
+ { if (E) { S } }
Submissions to Coverity Scan should be limited to those originated from
release branches and only from a specific schedule which holds
COVERITY_SCAN_PROJECT_NAME and COVERITY_SCAN_TOKEN variables.
This job requires two CI variables to be set:
- COVERITY_SCAN_PROJECT_NAME: project name, which is associated with
the BIND branch for which this job is executed, e.g. "bind-master",
- COVERITY_SCAN_TOKEN: project token.
There was a hard limit set on number of uvreq and nmhandles
that can be allocated by a pool, but we don't handle a situation
where we can't get an uvreq. Don't limit the number at all,
let the OS deal with it.
The memory ordering in the rwlock was all wrong, I am copying excerpts
from the https://en.cppreference.com/w/c/atomic/memory_order#Relaxed_ordering
for the convenience of the reader:
Relaxed ordering
Atomic operations tagged memory_order_relaxed are not synchronization
operations; they do not impose an order among concurrent memory
accesses. They only guarantee atomicity and modification order
consistency.
Release-Acquire ordering
If an atomic store in thread A is tagged memory_order_release and an
atomic load in thread B from the same variable is tagged
memory_order_acquire, all memory writes (non-atomic and relaxed atomic)
that happened-before the atomic store from the point of view of thread
A, become visible side-effects in thread B. That is, once the atomic
load is completed, thread B is guaranteed to see everything thread A
wrote to memory.
The synchronization is established only between the threads releasing
and acquiring the same atomic variable. Other threads can see different
order of memory accesses than either or both of the synchronized
threads.
Which basically means that we had no or weak synchronization between
threads using the same variables in the rwlock structure. There should
not be a significant performance drop because the critical sections were
already protected by:
while(1) {
if (relaxed_atomic_operation) {
break;
}
LOCK(lock);
if (!relaxed_atomic_operation) {
WAIT(sem, lock);
}
UNLOCK(lock)l
}
I would add one more thing to "Don't do your own crypto, folks.":
- Also don't do your own locking, folks.
The code for specifying OpenSSL PKCS#11 engine as part of the label
(e.g. -l "pkcs11:token=..." instead of -E pkcs11 -l "token=...")
was non-functional. This commit just cleans the related code.
Also disable the semantic patch as the code needs tweaks here and there because
some destroy functions might not destroy the object and return early if the
object is still in use.
Our destroy functions usually look like this:
void
foo_destroy(foo_t **foop) {
foo_t foo = *foop;
...destroy the contents of foo...
*foop = NULL;
}
nulling the pointer should be done as soon as possible which is
not always the case. This commit adds simple semantic patch that
changes the example function to:
void
foo_destroy(foo_t **foop) {
foo_t foo = *foop;
*foop = NULL;
...destroy the contents of foo...
}
On OpenBSD, the bin/tests/system/pipelined/ans5/ans.py script does not
shut down when it is sent the SIGTERM signal. What seems to be
happening is that starting the UDP listening thread somehow makes the
accept() calls in the script's main thread uninterruptible and thus the
SIGTERM signal sent to the main thread does not get processed until a
TCP connection is established with the script's TCP socket. Work around
the issue by setting a timeout for operations performed on the script's
TCP socket, so that each accept() call in the main thread's infinite
loop returns after at most 1 second, allowing termination signals sent
to the script to be processed.
The key-directory keyword actually does nothing right now but may
be useful in the future if we want to differentiate between key
directories or HSM keys, or if we want to speficy different
directories for different keys or policies. Make it optional for
the time being.
The keyword 'unlimited' can be used instead of PT0S which means the
same but is more comprehensible for users.
Also fix some redundant "none" parameters in the kasp test.
Creation of EVP_MD_CTX and EVP_PKEY is quite expensive, until
we fix the code to reuse the context and key we'll use our own
implementation of siphash.
Add checks to the kasp system test to verify CDNSKEY publication.
This test is not entirely complete, because when there is a CDNSKEY
available but there should not be one for KEY N, it is hard to tell
whether the existing CDNSKEY actually belongs to KEY N or another
key.
The check works if we expect a CDNSKEY although we cannot guarantee
that the CDNSKEY is correct: The test verifies existence, not
correctness of the record.
When you do a restart or reconfig of named, or rndc loadkeys, this
triggers the key manager to run. The key manager will check if new
keys need to be created. If there is an active key, and key rollover
is scheduled far enough away, no new key needs to be created.
However, there was a bug that when you just start to sign your zone,
it takes a while before the KSK becomes an active key. An active KSK
has its DS submitted or published, but before the key manager allows
that, the DNSKEY needs to be omnipresent. If you restart named
or rndc loadkeys in quick succession when you just started to sign
your zone, new keys will be created because the KSK is not yet
considered active.
Fix is to check for introducing as well as active keys. These keys
all have in common that their goal is to become omnipresent.
In system tests on Windows tool's local port can sometimes clash with
'named'. On Unix the system is poked for the minimal local port,
otherwise is set to 32768 as a sane minimum. For Windows we don't
poke but set a hardcoded limit; this change aligns the limit with
Unix and changes it to 32768.
10067 cleanup:
CID 1452683 (#1 of 1): Dereference before null check (REVERSE_INULL)
check_after_deref: Null-checking dispatch suggests that it
may be null, but it has already been dereferenced on all
paths leading to the check.
10068 if (dispatch != NULL)
10069 isc_mem_put(server->mctx, dispatch, sizeof(*dispatch));
1549 cleanup:
1550 if (dctx->dbiter != NULL)
1551 dns_dbiterator_destroy(&dctx->dbiter);
1552 if (dctx->db != NULL)
1553 dns_db_detach(&dctx->db);
CID 1452686 (#1 of 1): Dereference before null check (REVERSE_INULL)
check_after_deref: Null-checking dctx suggests that it may
be null, but it has already been dereferenced on all paths
leading to the check.
1554 if (dctx != NULL)
1555 isc_mem_put(mctx, dctx, sizeof(*dctx));
707 complete_allnds:
CID 1452689 (#1 of 1): Dereference before null check (REVERSE_INULL)
check_after_deref: Null-checking dir_list suggests that it
may be null, but it has already been dereferenced on all
paths leading to the check.
708 if (dir_list != NULL) {
709 /* clean up entries from list. */
389 else
CID 1452695 (#1 of 1): Dereference before null check (REVERSE_INULL)
check_after_deref: Null-checking lcfg suggests that it may
be null, but it has already been dereferenced on all paths
leading to the check.
390 if (lcfg != NULL)
391 isc_logconfig_destroy(&lcfg);
122 cleanup:
CID 1452696 (#1 of 1): Dereference before null check (REVERSE_INULL)
check_after_deref: Null-checking s suggests that it may be
null, but it has already been dereferenced on all paths
leading to the check.
123 if (s != NULL)
124 isc_mem_free(mctx, s);
255 flag_fail:
256 /* get rid of what was build of the query list */
CID 1452697 (#1 of 1): Dereference before null check (REVERSE_INULL)
check_after_deref: Null-checking tql suggests that it may
be null, but it has already been dereferenced on all paths
leading to the check.
257 if (tql != NULL)
258 destroy_querylist(mctx, &tql);
6412 cleanup:
6413 dns_rdataset_disassociate(&neg);
6414 dns_rdataset_disassociate(&negsig);
CID 1452700 (#1 of 1): Dereference before null check (REVERSE_INULL)
check_after_deref: Null-checking closest suggests that it
may be null, but it has already been dereferenced on all
paths leading to the check.
6415 if (closest != NULL)
6416 free_noqname(mctx, &closest);
336 cleanup_mem:
337 /* cleanup memory */
338
339 /* free tmpPath memory */
CID 1452701 (#1 of 1): Dereference before null check (REVERSE_INULL)
check_after_deref: Null-checking tmpPath suggests that it
may be null, but it has already been dereferenced on all
paths leading to the check.
340 if (tmpPath != NULL && result != ISC_R_SUCCESS)
341 isc_mem_free(named_g_mctx, tmpPath);
342
343 /* free tmpPath memory */
344 return (result);
13429 cleanup:
13430 cancel_refresh(zone);
CID 1452702 (#1 of 1): Dereference before null check (REVERSE_INULL)
check_after_deref: Null-checking stub suggests that it may
be null, but it has already been dereferenced on all paths
leading to the check.
13431 if (stub != NULL) {
13432 stub->magic = 0;
6367cleanup:
6368 dns_rdataset_disassociate(&neg);
6369 dns_rdataset_disassociate(&negsig);
CID 1452704 (#1 of 1): Dereference before null check
(REVERSE_INULL) check_after_deref: Null-checking noqname
suggests that it may be null, but it has already been
dereferenced on all paths leading to the check.
6370 if (noqname != NULL)
6371 free_noqname(mctx, &noqname);
11030 cleanup:
CID 1452705 (#1 of 1): Dereference before null check
(REVERSE_INULL) check_after_deref: Null-checking dctx
suggests that it may be null, but it has already been
dereferenced on all paths leading to the check.
11031 if (dctx != NULL)
11032 dumpcontext_destroy(dctx);
11033 return (result);
1401 }
CID 1453455 (#1 of 1): Dereference before null check (REVERSE_INULL)
check_after_deref: Null-checking event suggests that it may be null,
but it has already been dereferenced on all paths leading to the check.
1402 if (event != NULL)
1403 isc_event_free(ISC_EVENT_PTR(&event));
13836 if (zone != NULL)
13837 dns_zone_detach(&zone);
null: At condition dz != NULL, the value of dz must be NULL.
dead_error_condition: The condition dz != NULL cannot be true.
13838 if (dz != NULL) {
CID 1453456 (#1 of 1): Logically dead code (DEADCODE)
dead_error_begin: Execution cannot reach this statement:
dns_zone_detach(&dz->zone);.
13839 dns_zone_detach(&dz->zone);
13840 isc_mem_put(named_g_mctx, dz, sizeof(*dz));
13841 }
128 return (ISC_R_SUCCESS);
129
CID 1456146 (#1 of 1): Structurally dead code (UNREACHABLE)
unreachable: This code cannot be reached: {
if (dst->labels[i] != N....
130 do {
402 ctx->serve_stale_ttl = 0;
notnull: At condition indentctx, the value of indentctx
cannot be NULL. dead_error_condition: The condition indentctx
must be true.
CID 1456147 (#1 of 1): Logically dead code (DEADCODE)
dead_error_line: Execution cannot reach the expression
default_indent inside this statement: ctx->indent = (indentctx
? ....
403 ctx->indent = indentctx ? *indentctx : default_indent;
1636 cleanup:
CID 1458130 (#1 of 1): Dereference before null check (REVERSE_INULL)
check_after_deref: Null-checking buffer suggests that it may be
null, but it has already been dereferenced on all paths leading to
the check.
1637 if (buffer != NULL)
1638 isc_buffer_free(&buffer);
Found by LGTM.com (see below for description), and while it should not
happen as EDNS OPT RDLEN is uint16_t, the fix is easy. A little bit
of cleanup is included too.
> In a loop condition, comparison of a value of a narrow type with a value
> of a wide type may result in unexpected behavior if the wider value is
> sufficiently large (or small). This is because the narrower value may
> overflow. This can lead to an infinite loop.
Increase the short lived record TTL and negative SOA TTL to make
this test less vulnerable to timing issues. The drawback is that we
also have to sleep longer in this test.
Add queries and checks for CAA RRtype in the serve-stale test.
Ensure that the "Others" rrtype stat counter is incremented and
decremented properly if the RRset becomes stale/ancient.
The low max-stale-ttl config option needs to be increased in order
to match the timing when things expire (aka become ancient).
This commit simplifies the cachedb rrset statistics in two ways:
- Introduce new rdtypecounter arithmetics, allowing bitwise
operations.
- Remove the special DLV statistic counter.
New rdtypecounter arithmetics
-----------------------------
"The rdtypecounter arithmetics is a brain twister". Replace the
enum counters with some defines. A rdtypecounter is now 8 bits for
RRtypes and 3 bits for flags:
0 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15
+--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+
| | | | | | S |NX| RRType |
+--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+--+
If the 8 bits for RRtype are all zero, this is an Other RRtype.
Bit 7 is the NXRRSET (NX) flag and indicates whether this is a
positive (0) or a negative (1) RRset.
Then bit 5 and 6 mostly tell you if this counter is for an active,
stale, or ancient RRtype:
S = 0x00 means Active
S = 0x01 means Stale
S = 0x10 means Ancient
Since a counter cannot be stale and ancient at the same time, we
treat S = 0x11 as a special case to deal with NXDOMAIN counters.
S = 0x11 indicates an NXDOMAIN counter and in this case the RRtype
field signals the expiry of this cached item:
RRType = 0 means Active
RRType = 1 means Stale
RRType = 2 means Ancient
This also removes counting the DLV RRtype separately. Since we have
deprecated the lookaside validation it makes no sense to keep this
special statistic counter.
Since OpenBSD 6.6 is the current OpenBSD release, replace OpenBSD 6.5
GitLab CI jobs with their up-to-date counterparts.
As CI jobs for OpenBSD 6.6 will be run by a generalized libvirt executor
rather than an OpenBSD-specific one, make the necessary tag and variable
adjustments as well.
- Add quotes before and after zone name when generating "addzone"
input so avoid "unexpected token" errors.
- Use a hex digest for zone filenames when the zone or view name
contains a slash.
- Test with a domain name containing a slash.
- Incidentally added 'catzhash.py' to contrib/scripts to generate
hash labels for catalog zones, as it was needed to write the test.
The isc_buffer_allocate() function now cannot fail with ISC_R_MEMORY.
This commit removes all the checks on the return code using the semantic
patch from previous commit, as isc_buffer_allocate() now returns void.
The isc_mempool_create() function now cannot fail with ISC_R_MEMORY.
This commit removes all the checks on the return code using the semantic
patch from previous commit, as isc_mempool_create() now returns void.
Each system test can be marked as failed not only due to some tested
component(s) not behaving as expected, but also because of core dumps,
assertion failures, and/or ThreadSanitizer reports being found among its
artifacts. Make the system test summary list the tests which exhibit
such atypical symptoms to more clearly present the nature of problems
found.
The `rndc signing -clear` command cleans up the private-type records
that keep track of zone signing activity, but before this change it
did not tell the secondary servers that the zone has changed.
Resolve "bind 9.14.8 and 9.14.9 aborts when queried for non-existing domain in chaos class"
Closes#1569 and #1540
See merge request isc-projects/bind9!2843
Added test to ensure that NXDOMAIN is returned when BIND is queried for a
non existing domain in CH class (if a view of CHAOS class is configured)
and that it also doesn't crash anymore in those cases.
Function dns_view_findzonecut in view.c wasn't correctly handling
classes other than IN (chaos, hesiod, etc) whenever the name being
looked up wasn't in cache or in any of the configured zone views' database.
That resulted in a NULL fname being used in resolver.c:4900, which
in turn was triggering abort.
When both 'broken' and 'failed' test cases appear in unit test output
...
===> Broken tests
lib/isc/tests/socket_test:main -> broken: Test case timed out [300.022s]
===> Failed tests
lib/isc/tests/time_test:main -> failed: 2 of 6 tests failed [0.006s]
===> Summary
...
spurious '===>' string gets matched, that results in the following
error:
Usage error for command debug: '===>' is not a test case identifier (missing ':'?).
Following change makes sure the string is omitted.
I checked on FreeBSD and OpenBSD that the AWK construct is supported.
If we created a key, mark its SyncPublish time as 'now' and started
bind the key might not be published if the SyncPublish time is in
the same second as the time the zone is loaded. This is mostly
for dnssec system test, as this kind of scenario is very unlikely
in a real world environment.
To reproduce the race - create a task, send two events to it, first one
must take some time. Then, from the outside, pause(), unpause() and detach()
the task.
When the long-running event is processed by the task it is in
task_state_running state. When we called pause() the state changed to
task_state_paused, on unpause we checked that there are events in the task
queue, changed the state to task_state_ready and enqueued the task on the
workers readyq. We then detach the task.
The dispatch() is done with processing the event, it processes the second
event in the queue, and then shuts down the task and frees it (as it's not
referenced anymore). Dispatcher then takes the, already freed, task from
the queue where it was wrongly put, causing an use-after free and,
subsequently, either an assertion failure or a segmentation fault.
The probability of this happening is very slim, yet it might happen under a
very high load, more probably on a recursive resolver than on an
authoritative.
The fix introduces a new 'task_state_pausing' state - to which tasks
are moved if they're being paused while still running. They are moved
to task_state_paused state when dispatcher is done with them, and
if we unpause a task in paused state it's moved back to task_state_running
and not requeued.
This is a bug I encountered when trying to schedule an algorithm
rollover. My plan, for a zone whose maximum TTL is 48h, was to sign
with the new algorithm and schedule a change of CDS records for more
than 48 hours in the future, roughly like this:
$ dnssec-keygen -a 13 -fk -Psync now+50h $zone
$ dnssec-keygen -a 13 $zone
$ dnssec-settime -Dsync now+50h $zone_ksk_old
However the algorithm 13 CDS was published immediately, which could
have made the zone bogus.
To reveal the bug using the `smartsign` test, this change just adds a
KSK with all its times in the future, so it should not affect the
existing checks at all. But the final check (that there are no CDS or
CDSNSKEY records after -Dsync) fails with the old `syncpublish()`
logic, because the future key's sync records appear early. With the
new `syncpublish()` logic the future key does not affect the test, as
expected, and it now passes.
When two threads unreferenced handles coming from one socket while
the socket was being destructed we could get a use-after-free:
Having handle H1 coming from socket S1, H2 coming from socket S2,
S0 being a parent socket to S1 and S2:
Thread A Thread B
Unref handle H1 Unref handle H2
Remove H1 from S1 active handles Remove H2 from S2 active handles
nmsocket_maybe_destroy(S1) nmsocket_maybe_destroy(S2)
nmsocket_maybe_destroy(S0) nmsocket_maybe_destroy(S0)
LOCK(S0->lock)
Go through all children, figure
out that we have no more active
handles:
sum of S0->children[i]->ah == 0
UNLOCK(S0->lock)
destroy(S0)
LOCK(S0->lock)
- but S0 is already gone
We weren't consistent about who should unreference the handle in
case of network error. Make it consistent so that it's always the
client code responsibility to unreference the handle - either
in the callback or right away if send function failed and the callback
will never be called.
If taskmgr is shutting down ns_client_setup will fail to create
a task for the newly created client, we weren't cleaning up already
created/attached things (memory context, server, clientmgr).
In tcp and udp stoplistening code we accessed libuv structures
from a different thread, which caused a shutdown crash when named
was under load. Also added additional DbC checks making sure we're
in a proper thread when accessing uv_ functions.
We had a race in which n UDP socket could have been already closing
by libuv but we still sent data to it. Mark socket as not-active
when stopping listening and verify that socket is not active when
trying to send data to it.
if validator_start() is called with validator->event->message set to
NULL, we can't use message->rcode to decide which negative proofs are
needed, so we use the rdataset attributes instead to determine whether
the rdataset was cached as NXDOMAIN or NODATA.
We pass interface as an opaque argument to tcpdns listening socket.
If we stop listening on an interface but still have in-flight connections
the opaque 'interface' is not properly reference counted, and we might
hit a dead memory. We put just a single source of truth in a listening
socket and make the child sockets use that instead of copying the
value from listening socket. We clean the callback when we stop listening.
- isc__netievent_storage_t was to small to contain
isc__netievent__socket_streaminfo_t on Windows
- handle isc_uv_export and isc_uv_import errors properly
- rewrite isc_uv_export and isc_uv_import on Windows
hp implementation requires an object for each thread accessing
a hazard pointer. previous implementation had a hardcoded
HP_MAX_THREAD value of 128, which failed on machines with lots of
CPU cores (named uses 3n threads). We make isc__hp_max_threads
configurable at startup, with the value set to 4*named_g_cpus.
It's also important for this value not to be too big as we do
linear searches on a list.
it now removes matching trust anchors from from the dslist while leaving
the other trust anchors in place.
also cleaned up the API to remove functions that were never being used.
NOTE: the keytable test is still failing because dns_keytable_deletekey()
is looking for exact matches in keynodes containing dst_key objects,
which no keynode has anymore.
the internal keytable structure has not yet been changed, but
insertion of DS anchors is the only method now available.
NOTE: the keytable unit test is currently failing because of tests
that expect individual keynode objects to contain single DST key
objects.
as initial-key and static-key trust anchors will now be stored as a
DS rrset, code referencing keynodes storing DNSKEY trust anchors will
no longer be reached.
this function is used by dns_view_untrust() to handle revoked keys, so
it will still be needed after the keytable/validator refactoring is
complete, even though the keytable will be storing DS trust anchors
instead of keys. to simplify the way it's called, it now takes a DNSKEY
rdata struct instead of a DST key.
The isc_refcount API that provides reference counting lost DbC checks for
overflows and underflows in the isc_refcount_{increment,decrement} functions.
The commit restores the overflow check in the isc_refcount_increment and
underflows check in the isc_refcount_decrement by checking for the previous
value to not be on the boundary.
This commits removes superfluous checks when using the isc_refcount API.
Examples of superfluous checks:
1. The isc_refcount_decrement function ensures there was not underflow,
so this check is superfluous:
INSIST(isc_refcount_decrement(&r) > 0);
2 .The isc_refcount_destroy() includes check whether the counter
is zero, therefore this is superfluous:
INSIST(isc_refcount_decrement(&r) == 1 && isc_refcount_destroy(&r));
If a connection was closed early (right after accept()) an assertion
that assumed that the connection was still alive could be triggered
in accept_connection. Handle those errors properly and not with
assertions, free all the resources afterwards.
- the socket stat counters have been moved from socket.h to stats.h.
- isc_nm_t now attaches to the same stats counter group as
isc_socketmgr_t, so that both managers can increment the same
set of statistics
- isc__nmsocket_init() now takes an interface as a paramter so that
the address family can be determined when initializing the socket.
- based on the address family and socket type, a group of statistics
counters will be associated with the socket - for example, UDP4Active
with IPv4 UDP sockets and TCP6Active with IPv6 TCP sockets. note
that no counters are currently associated with TCPDNS sockets; those
stats will be handled by the underlying TCP socket.
- the counters are not actually used by netmgr sockets yet; counter
increment and decrement calls will be added in a later commit.
If pktinfo were supported then we could listen on :: for ipv6 and get
the information about the destination address from pktinfo structure passed
in recvmsg but this method is not portable and libuv doesn't support it - so
we need to listen on all interfaces.
We should verify that this doesn't impact performance (we already do it for
ipv4) and either remove all the ipv6pktinfo detection code or think of fixing
libuv.
- use UV_{TC,UD}P_IPV6ONLY for IPv6 sockets, keeping the pre-netmgr
behaviour.
- add a new listening_error bool flag which is set if the child
listener fails to start listening. This fixes a bug where named would
hang if, e.g., we failed to bind to a TCP socket.
For multithreaded TCP listening we need to pass a bound socket to all
listening threads. Instead of using uv_pipe handle passing method which
is quite complex (lots of callbacks, each of them with its own error
handling) we now use isc_uv_export() to export the socket, pass it as a
member of the isc__netievent_tcpchildlisten_t structure, and then
isc_uv_import() it in the child thread, simplifying the process
significantly.
These functions can be used to pass a uv handle between threads in a
safe manner. The other option is to use uv_pipe and pass the uv_handle
via IPC, which is way more complex. uv_export() and uv_import() functions
existed in libuv at some point but were removed later. This code is
based on the original removed code.
The Windows version of the code uses two functions internal to libuv;
a patch for libuv is attached for exporting these functions.
Ensure BIND is continuously tested on Tumbleweed, a pure rolling release
version of openSUSE. This will allow BIND incompatibilities with latest
upstream versions of its dependencies to be caught more quickly.
5339. [bug] With some libmaxminddb versions, named could erroneously
match an IP address not belonging to any subnet defined
in a given GeoIP2 database to one of the existing
entries in that database. [GL #1552]
Only comparing the value of the integer passed as the last argument to
MMDB_lookup_sockaddr() against MMDB_SUCCESS is not enough to ensure that
an MMDB lookup was successful - the 'found_entry' field of the
MMDB_lookup_result_s structure returned by that function also needs to
be true or else the remaining contents of that structure should be
ignored as the lookup failed. Extend the relevant logical condition in
get_entry_for() to ensure the latter does not return incorrect MMDB
entries for IP addresses which do not belong to any subnet defined in a
given GeoIP2 database.
Before this change, there was a missing blank line between the
negative trust anchors for one view, and the heading line for the next
view. This is because dns_ntatable_totext() omits the last newline.
There is an example of the incorrect output below; the fixed output
has a blank line before "Start view auth".
secure roots as of 21-Oct-2019 12:03:23.500:
Start view rec
Secure roots:
./RSASHA256/20326 ; managed
Negative trust anchors:
example.com: expiry 21-Oct-2019 13:03:15.000
Start view auth
Secure roots:
./RSASHA256/20326 ; managed
Negative trust anchors:
example.com: expiry 21-Oct-2019 13:03:07.000
Some unit tests need various managers to be created before they are run.
The interface manager spawned during libns tests listens on a fixed port
number, which causes intermittent issues when multiple tests using an
interface manager are run concurrently. Make the interface manager
listen on a randomized port number to greatly reduce the risk of
multiple unit tests using the same port concurrently.
"rndc signing -serial <value>" could take longer than a second to
complete. Loop waiting for update to succeed.
For tests where "rndc signing -serial <value>" is supposed to not
succeed, repeatedly test that we don't get the new serial, then
test that we have the old value. This should prevent false negatives.
GitLab issue and merge request numbers placed in release notes (in the
form of "#1234" for issues and "!5678" for merge requests) should not be
split across two lines. Extend the shell pipeline generating
doc/arm/notes.txt with a sed invocation which prevents such splitting.
The initial tcp statistics test was not testing tcp-highwater counter,
but only initial number of current TCP clients, so this missing test was
added to ensure initial tcp-highwater value is correct.
After the network manager rewrite, tcp-higwater stats was only being
updated when a valid DNS query was received over tcp.
It turns out tcp-quota is updated right after a tcp connection is
accepted, before any data is read, so in the event that some client
connect but don't send a valid query, it wouldn't be taken into
account to update tcp-highwater stats, that is wrong.
This commit fix tcp-highwater to update its stats whenever a tcp connection
is established, independent of what happens after (timeout/invalid
request, etc).
During BIND startup it scans for network interfaces available, in this
process it ensures that for every interface it will bind and listen to,
at least one socket will be always available accepting connections on
that interface, this way avoiding some DOS attacks that could exploit
tcp quota on some interface and make others unavailable.
In the previous network implementation this initial "reserved" tcp-quota
used by BIND was already been added to the tcp-highwater stats, but with
the new network code it was necesary to add this workaround to ensure
tcp-highwater stats reflect the tcp-quota used by BIND after startup.
- Add a GitLab merge request number to the "trust-anchors" release
note and slightly rephrase its second half.
- Replace tabs with spaces in doc/arm/notes-9.15.7.xml to retain
consistency with other XML files containing release notes.
- Move the "Security Fixes" section for BIND 9.15.6 higher up, for
consistency with release notes for other versions.
- Add a missing release note for TCP high-water. That feature was not
yet merged when the initial version of !2524 was prepared and its
release note was missed when that merge request was later rebased.
- Rephrase the release note for CVE-2019-6477 so that it uses the same
text as its corresponding notes in all other releases.
- Unify whitespace in doc/arm/notes-9.15.6.xml.
Add a GitLab CI job (which is run only if all other jobs in a pipeline
succeed) that builds a BIND release tarball, i.e. fetches the source
tarball from the tarball building job, creates Windows zips, puts
certain parts of BIND documentation into the appropriate places, and
packs it all up into a single tarball whose contents can be subsequently
signed and published.
Add a system test job for binaries created by Visual Studio in the
"Debug" build configuration to GitLab CI so that they can be tested
along their "Release" counterparts when necessary.
Add a Visual Studio build job using the "Debug" build configuration to
GitLab CI without enabling it for every pipeline as it takes about twice
as long to complete as its "Release" counterpart.
Add a set of jobs to GitLab CI that create a BIND source tarball and
then build and test its contents. Run those extra jobs only when a tag
is pushed to the Git repository as they are only meant to be sanity
checks of BIND source tarball contents.
The util/prepare-softhsm2.sh script is useful for initializing a working
SoftHSM environment which can be used by unit tests and system tests.
However, since it is a test-specific script, it does not really belong
in the util/ subdirectory which is mostly pruned during the BIND source
tarball creation process. Move the prepare-softhsm2.sh script to
bin/tests/ so that its location is more appropriate for its purpose and
also so that it does not get removed during the BIND source tarball
creation process, allowing it to be used for setting up test
environments for tarball-based builds.
Convert the logic (currently present in the form of "rm -rf" calls in
util/kit.sh) for removing files and directories which are tracked by Git
but redundant in release tarballs into a set of .gitattributes rules
which allow the same effect to be achieved using "git archive".
Resolve "ThreadSanitizer: data race /home/ondrej/Projects/bind9/lib/isc/netmgr/netmgr.c:1027 in nmhandle_free"
Closes#1473
See merge request isc-projects/bind9!2739
The LC_ALL=C assignments in the "idna" system test, which were only
meant to affect a certain subset of checks, in fact persist throughout
all the subsequent checks in that system test. That affects the test's
behavior and is misleading.
When the "VARIABLE=value command ..." syntax is used in a shell script,
in order for the variable assignment to only apply to "command", the
latter must be an external binary; otherwise, the VARIABLE=value
assignment persists for all subsequent commands in a script:
$ cat foo.sh
#!/bin/sh
foo() {
/bin/sh bar.sh
}
BAR="baz0"
BAR="baz1" /bin/sh bar.sh
echo "foo: BAR=${BAR}"
BAR="baz2" foo
echo "foo: BAR=${BAR}"
$ cat bar.sh
#!/bin/sh
echo "bar: BAR=${BAR}"
$ /bin/sh foo.sh
bar: BAR=baz1
foo: BAR=baz0
bar: BAR=baz2
foo: BAR=baz2
$
Fix by saving the value of LC_ALL before the relevant set of checks in
the "idna" system test, restoring it afterwards, and dropping the
"LC_ALL=C command ..." syntax.
- make tcp listening IPC pipe name saner
- put the pipe in /tmp on unices
- add pid to the pipe name to avoid conflicts between processes
- fsync directory in which the pipe resides to make sure that the
child threads will see it and be able to open it
even when worker is paused (e.g. interface reconfiguration). This is
needed to prevent deadlocks when reconfiguring interfaces - as network
manager is paused then, but we still need to stop/start listening.
- Proper handling of TCP listen errors in netmgr - bind to the socket first,
then return the error code.
When listening for TCP connections we create a socket, bind it
and then pass it over IPC to all threads - which then listen on
in and accept connections. This sounds broken, but it's the
official way of dealing with multithreaded TCP listeners in libuv,
and works on all platforms supported by libuv.
In decrement_reference only test node->down if the tree lock
is held. As node->down is not always tested in
decrement_reference we need to test that it is non NULL in
cleanup_dead_nodes prior to removing the node from the rbt
tree. Additionally it is not always possible to aquire the
node lock and reactivate a node when adding parent nodes.
Reactivate such nodes in cleanup_dead_nodes if required.
Before, the zero system test could get stuck almost infinitely, because
the first test sends > 300 queries with 5 seconds timeout on each in
each pass. If named crashed early, it would took the test more than 4
hours to properly timeout.
This commit introduces a "watchdog" on the dig commands running in the
background and failing the test on timeout, failing any test if any dig
command fails to return successfully, and making the tests.sh script
shellcheck clean.
The kasp system test has a call to sed to retrieve the key identifier
without leading zeros. The sed call could not handle key id 0.
Update the kasp test to also correctly deal with this case.
The autosign test has a test case where a DNSSEC maintaiend zone
has a set of DNSSEC keys without any timing metadata set. It
tests if named picks up the key for publication and signing if a
delayed dnssec-settime/loadkeys event has occured.
The test failed intermittently despite the fact it sleeps for 5
seconds but the triggered key reconfigure action should happen after
3 seconds.
However, the test output showed that the test query came in before
the key reconfigure action was complete (see excerpts below).
The loadkeys command is received:
15:38:36 received control channel command 'loadkeys delay.example.'
The reconfiguring zone keys action is triggered after 3 seconds:
15:38:39 zone delay.example/IN: reconfiguring zone keys
15:38:39 DNSKEY delay.example/NSEC3RSASHA1/7484 (ZSK) is now published
15:38:39 DNSKEY delay.example/NSEC3RSASHA1/7455 (KSK) is now published
15:38:39 writing to journal
Two seconds later the test query comes in:
15:38:41 client @0x7f1b8c0562b0 10.53.0.1#44177: query
15:38:41 client @0x7f1b8c0562b0 10.53.0.1#44177: endrequest
And 6 more seconds later the reconfigure keys action is complete:
15:38:47 zone delay.example/IN: next key event: 05-Dec-2019 15:48:39
This commit fixes the test by checking the "next key event" log has
been seen before executing the test query, making sure that the
reconfigure keys action has been complete.
This commit however does not fix, nor explain why it took such a long
time (8 seconds) to reconfigure the keys.
Clarify in the ARM that TTL-style options can also now take ISO
8601 durations.
Mention the built-in dnssec policies "default" and "none". Mention
that "none" is the default.
Add a file documenting the default dnssec-policy configuration options.
Fix dnssec-policy syntax in ARM (dnssec-policy.grammar.xml).
The first step in all existing setup.sh scripts is to call clean.sh. To
reduce code duplication and ensure all system tests added in the future
behave consistently with existing ones, invoke clean.sh from run.sh
before calling setup.sh.
At the end of each system test suite run, the system test framework
collects all existing test.output files from system test subdirectories
and produces bin/tests/system/systests.output from those files.
However, it does not check whether a test.output file was found for
every executed test. Thus, if the test.output file is accidentally
deleted by the system test itself (e.g. due to an overly broad file
removal wildcard present in clean.sh), its output will not be included
in bin/tests/system/systests.output. Since the result of each system
test suite run is determined by bin/tests/system/testsummary.sh, which
only operates on the contents of bin/tests/system/systests.output, this
can lead to test failures being ignored. Fix by ensuring the number of
test results found in bin/tests/system/systests.output is equal to the
number of tests run and triggering a system test suite failure in case
of a discrepancy between these two values.
Since the role of the bin/tests/system/clean.sh script has now been
reduced to calling a given system test's clean.sh script, remove the
former altogether and replace its only use with a direct invocation of
the latter.
Since files containing system test output are no longer stored in test
subdirectories, bin/tests/system/clean.sh no longer needs to take care
of removing the test.output file for a given test as testsummary.sh
already takes care of that and even if a test suite terminates
abnormally and another one is started, tee invoked without the -a
command line switch overwrites the destination file if it exists, so
leftover test.output.* files from previous test suite runs are not a
concern. Remove the -r command line switch and the code associated with
it from the relevant scripts.
Some clean.sh scripts contain overly broad file deletion wildcards which
cause the test.output file (used by the system test framework for
collecting output) in a given system test's directory to be erroneously
removed immediately after the test is started (due to setup.sh scripts
calling clean.sh at the beginning). This prevents the test's output
from being placed in bin/tests/system/systests.output at the end of a
test suite run and thus can lead to test failures being ignored. Fix by
storing each test's output in a test.output.<test-name> file in
bin/tests/system/, which prevents clean.sh scripts from removing it (as
they should only ever affect files contained in a given system test's
directory).
When a function returns void, it can be used as an argument to return in
function returning also void, e.g.:
void in(void) {
return;
}
void out(void) {
return (in());
}
while this is legal, it should be rewritten as:
void out(void) {
in();
return;
}
The semantic patch just find the occurrences, and they need to be fixed
by hand.
Since the introduction of durations, all ttlval configuration types
are replaced with durations. Duration is an ISO 8601 duration, a
TTL-style value, or a number. These two references were missed and
are now also replaced.
2019-12-06 12:28:40 +01:00
2115 changed files with 102294 additions and 173556 deletions
- [ ]***(QA)*** Inform Support and Marketing of impending release (and give estimated release dates).
- [ ]***(QA)*** Ensure there are no permanent test failures on any platform.
- [ ]***(QA)*** Check Perflab to ensure there has been no unexplained drop in performance for the versions being released.
- [ ]***(QA)*** Check whether all issues assigned to the release milestone are resolved[^1].
- [ ]***(QA)*** Ensure that there are no outstanding merge requests in the private repository[^1] (Subscription Edition only).
- [ ]***(QA)*** Ensure all merge requests marked for backporting have been indeed backported.
## Before the Tagging Deadline
### Before the Tagging Deadline
- [ ]***(QA)*** Inform Support/Marketing of impending release (and give estimated release dates).
- [ ]***(QA)*** Check Perflab to ensure there has been no unexplained drop in performance for the versions being released.
- [ ]***(QA)*** Look for outstanding documentation issues (e.g. `CHANGES` mistakes) and address them if any are found.
- [ ]***(QA)*** Ensure release notes are correct, ask Support and Marketing to check them as well.
- [ ]***(Support)*** Check release notes, ask QA to correct any mistakes found.
- [ ]***(Marketing)*** Check release notes, ask QA to correct any mistakes found.
- [ ]***(SwEng)*** Update API files for libraries with new version information.
- [ ]***(SwEng)*** Change software version and library versions in `configure.ac` (new major release only).
- [ ]***(SwEng)*** Rebuild `configure` using Autoconf on `docs.isc.org`.
@@ -26,28 +32,28 @@
- [ ]***(SwEng)*** Update `version`.
- [ ]***(SwEng)*** Build documentation on `docs.isc.org`.
- [ ]***(QA)*** Check that all the above steps were performed correctly.
- [ ]***(QA)*** Check that the contents of release notes match the merge requests comprising the releases.
- [ ]***(QA)*** Check that the formatting is correct for text, PDF, and HTML versions of release notes.
- [ ]***(SwEng)*** Tag the releases[^2]. (Tags may only be pushed to the public repository for releases which are *not* security releases.)
- [ ]***(SwEng)*** If this is the first tag for a release (e.g. beta), create a release branch named `release_v9_X_Y` to allow development to continue on the maintenance branch whilst release engineering continues.
## Before the ASN Deadline (for ASN Releases) or the Public Release Date (for Regular Releases)
### Before the ASN Deadline (for ASN Releases) or the Public Release Date (for Regular Releases)
- [ ]***(QA)*** Run the `make release` Jenkins jobs to produce the tarballs and zips.
- [ ]***(QA)*** Verify the results of `make release` Jenkins jobs and prepare a QA report for the releases to be published.
- [ ]***(QA)*** Request signatures for the tarballs.
- [ ]***(Signers)*** Sign the tarballs.
- [ ]***(QA)*** Check tarball signatures.
- [ ]***(QA)*** Notify Support that the releases are ready for publication.
- [ ]***(QA)*** Verify GitLab CI results for the tags created and prepare a QA report for the releases to be published.
- [ ]***(QA)*** Request signatures for the tarballs, providing their location and checksums.
- [ ]***(Support)*** Pre-publish ASN and/or Subscription Edition tarballs so that packages can be built.
- [ ]***(QA)*** Build and test ASN and/or Subscription Edition packages.
- [ ]***(QA)*** Notify Support that the releases have been prepared.
- [ ]***(Support)*** Send out ASNs (if applicable).
## On the Day of Public Release
### On the Day of Public Release
- [ ]***(Support)*** Publish the releases according to the release schedule.
- [ ]***(Support)*** Write release email to *bind9-announce*.
- [ ]***(Support)*** Write email to *bind9-users* (if a major release).
- [ ]***(Support)*** Wait for clearance from Security Officer to proceed with the public release (if applicable).
- [ ]***(Support)*** Place tarballs in public location on FTP site.
- [ ]***(Support)*** Publish links to downloads on ISC website.
- [ ]***(Support)*** Write release email to *bind-announce*.
- [ ]***(Support)*** Write email to *bind-users* (if a major release).
- [ ]***(Support)*** Update tickets in case of waiting support customers.
- [ ]***(QA)*** Build and test any outstanding private packages.
- [ ]***(QA)*** Build public packages (`*.deb`, RPMs).
@@ -59,6 +65,8 @@
- [ ]***(QA)*** Ensure all new tags are annotated and signed.
- [ ]***(SwEng)*** Push tags for the published releases to the public repository.
- [ ]***(SwEng)*** Merge the automatically prepared `prep 9.X.Y` commit which updates `version` and documentation on the release branch into the relevant maintenance branch (`v9_X`).
- [ ]***(QA)*** For each maintained branch, update the `BIND_BASELINE_VERSION` variable for the `abi-check:sid:amd64` job in `.gitlab-ci.yml` to the latest published BIND version tag for a given branch.
- [ ]***(QA)*** Prepare empty release notes for the next set of releases.
[^1]: If not, use the time remaining until the tagging deadline to ensure all outstanding issues are either resolved or moved to a different milestone.
|`CC`|The C compiler to use. `configure` tries to figure out the right one for supported systems.|
|`CFLAGS`|C compiler flags. Defaults to include -g and/or -O2 as supported by the compiler. Please include '-g' if you need to set `CFLAGS`. |
|`STD_CINCLUDES`|System header file directories. Can be used to specify where add-on thread or IPv6 support is, for example. Defaults to empty string.|
|`STD_CDEFINES`|Any additional preprocessor symbols you want defined. Defaults to empty string. For a list of possible settings, see the file [OPTIONS](OPTIONS.md).|
|`LDFLAGS`|Linker flags. Defaults to empty string.|
|`BUILD_CC`|Needed when cross-compiling: the native C compiler to use when building for the target system.|
|`BUILD_CFLAGS`|`CFLAGS` for the target system during cross-compiling.|
|`BUILD_CPPFLAGS`|`CPPFLAGS` for the target system during cross-compiling.|
|`BUILD_LDFLAGS`|`LDFLAGS` for the target system during cross-compiling.|
|`BUILD_LIBS`|`LIBS` for the target system during cross-compiling.|
Additional environment variables affecting the build are listed at the
end of the `configure` help text, which can be obtained by running the
command:
$ ./configure --help
#### <a name="macos"> macOS
Building on macOS assumes that the "Command Tools for Xcode" is installed.
This can be downloaded from [https://developer.apple.com/download/more/](https://developer.apple.com/download/more/)
or if you have Xcode already installed you can run `xcode-select --install`.
Specify the port on the server to query\&. The default is 53\&.
.RE
.PP
\-r
.RS4
Non\-recursive query: Setting this option clears the RD (recursion desired) bit in the query\&. This should mean that the name server receiving the query will not attempt to resolve
@@ -269,5 +274,5 @@ runs\&.
\fBInternet Systems Consortium, Inc\&.\fR
.SH"COPYRIGHT"
.br
Copyright \(co 2000-2002, 2004, 2005, 2007-2009, 2014-2019 Internet Systems Consortium, Inc. ("ISC")
Copyright \(co 2000-2002, 2004, 2005, 2007-2009, 2014-2020 Internet Systems Consortium, Inc. ("ISC")
Some files were not shown because too many files have changed in this diff
Show More
Reference in New Issue
Block a user
Blocking a user prevents them from interacting with repositories, such as opening or commenting on pull requests or issues. Learn more about blocking a user.
Witold Kręcicki
Ondřej Surý
Michał Kępień
Evan HuntandMichał Kępień
Ondřej Surý
Aaron ThompsonandOndřej Surý
Mark Andrews
Matthijs Mekking
Michal Nowak
Tinderbox UserandMichał Kępień
Stephen MorrisandMichał Kępień
Petr MenšíkandOndřej Surý
Diego FronzaandOndřej Surý
Tony FinchandEvan Hunt
Logan CamposandOndřej Surý
Witold KręcickiandOndřej Surý