There's a deadlock in BIND 9 code where (dns_view_t){ .lock } and
(dns_resolver_t){ .buckets[i].lock } gets locked in different order. When
view->weakrefs gets converted to a reference counting we can reduce the locking
in dns_view_weakdetach only to cases where it's the last instance of the
dns_view_t object.
(cherry picked from commit a7c9a52c89)
(cherry picked from commit 232140edae)
Previously isc_thread_join() would return ISC_R_UNEXPECTED on a failure to
create new thread. All such occurences were caught and wrapped into assert
function at higher level. The function was simplified to assert directly in the
isc_thread_join() function and all caller level assertions were removed.
Previously isc_thread_create() would return ISC_R_UNEXPECTED on a failure to
create new thread. All such occurences were caught and wrapped into assert
function at higher level. The function was simplified to assert directly in the
isc_thread_create() function and all caller level assertions were removed.
Using isc_mem_put(mctx, ...) + isc_mem_detach(mctx) required juggling with the
local variables when mctx was part of the freed object. The isc_mem_putanddetach
function can handle this case internally, but it wasn't used everywhere. This
commit apply the semantic patching plus bit of manual work to replace all such
occurrences with proper usage of isc_mem_putanddetach().
The lib/dns/zoneverify.c output was hardwired to stderr, which was inconsistent
with lib/dns/dnssec.c. This commit changes zoneverify.c to print the normal run
information to caller supplied function - same model as in the lib/dns/dnssec.c.
"PST8PDT" is a legacy time zone name whose use in modern code is
discouraged. It so happens that using this time zone with musl libc
time functions results in different output than for other libc
implementations, which breaks the lib/isc/tests/time_test unit test.
Use the "America/Los_Angeles" time zone instead in order to get
consistent output across all tested libc implementations.
For some libc implementations, BUFSIZ is small enough (e.g. 1024 for
musl libc) to trigger compilation warnings about insufficient size of
certain buffers. Since the relevant buffers are used for printing DNS
names, increase their size to '(n + 1) * DNS_NAME_FORMATSIZE', where 'n'
is the number of DNS names which are printed to a given buffer. This
results in somewhat arbitrary, albeit nicely-aligned and large enough
buffer sizes.
Including <sys/errno.h> instead of <errno.h> raises a compiler warning
when building against musl libc. Always include <errno.h> instead of
<sys/errno.h> to prevent that compilation warning from being triggered
and to achieve consistency in this regard across the entire source tree.
Make sure all unit tests include headers in a similar order:
1. Three headers which must be included before <cmocka.h>.
2. System headers.
3. UNIT_TESTING definition, followed by the <cmocka.h> header.
4. libisc headers.
5. Headers from other BIND libraries.
6. Local headers.
Also make sure header file names are sorted alphabetically within each
block of #include directives.
All unit tests define the UNIT_TESTING macro, which causes <cmocka.h> to
replace malloc(), calloc(), realloc(), and free() with its own functions
tracking memory allocations. In order for this not to break
compilation, the system header declaring the prototypes for these
standard functions must be included before <cmocka.h>.
Normally, these prototypes are only present in <stdlib.h>, so we make
sure it is included before <cmocka.h>. However, musl libc also defines
the prototypes for calloc() and free() in <sched.h>, which is included
by <pthread.h>, which is included e.g. by <isc/mutex.h>. Thus, unit
tests including "dnstest.h" (which includes <isc/mem.h>, which includes
<isc/mutex.h>) after <cmocka.h> will not compile with musl libc as for
these programs, <sched.h> will be included after <cmocka.h>.
Always including <cmocka.h> after all other header files is not a
feasible solution as that causes the mock assertion macros defined in
<isc/util.h> to mangle the contents of <cmocka.h>, thus breaking
compilation. We cannot really use the __noreturn__ or analyzer_noreturn
attributes with cmocka assertion functions because they do return if the
tested condition is true. The problem is that what BIND unit tests do
is incompatible with Clang Static Analyzer's assumptions: since we use
cmocka, our custom assertion handlers are present in a shared library
(i.e. it is the cmocka library that checks the assertion condition, not
a macro in unit test code). Redefining cmocka's assertion macros in
<isc/util.h> is an ugly hack to overcome that problem - unfortunately,
this is the only way we can think of to make Clang Static Analyzer
properly process unit test code. Giving up on Clang Static Analyzer
being able to properly process unit test code is not a satisfactory
solution.
Undefining _GNU_SOURCE for unit test code could work around the problem
(musl libc's <sched.h> only defines the prototypes for calloc() and
free() when _GNU_SOURCE is defined), but doing that could introduce
discrepancies for unit tests including entire *.c files, so it is also
not a good solution.
All in all, including <sched.h> before <cmocka.h> for all affected unit
tests seems to be the most benign way of working around this musl libc
quirk. While quite an ugly solution, it achieves our goals here, which
are to keep the benefit of proper static analysis of unit test code and
to fix compilation against musl libc.
- removed some dead code
- dns_zone_setdbtype is now void as it could no longer return
anything but ISC_R_SUCCESS; calls to it no longer check for a result
- controlkeylist_fromconfig() is also now void
- fixed a whitespace error
Commit 9da902a201 removed locking around
the fctx_decreference() call inside resume_dslookup(). This allows
fctx_unlink() to be called without the bucket lock being held, which
must never happen. Ensure the bucket lock is held by resume_dslookup()
before it calls fctx_decreference().
When the unit test is linked with dynamic libraries, the wrapping
doesn't occur, probably because it's different translation unit.
To workaround the issue, we provide thin wrappers with *real* symbol
names that just call the mocked functions.
1. Restore locking in the fctx_decreference() code, because the insides of the
function needs to be protected when fctx->references drops to 0.
2. Restore locking in the dns_resolver_attach() code, because two variables are
accessed at the same time and there's slight chance of data race.
Although the struct dns_resolver.exiting member is protected by stdatomics, we
actually need to wait for whole dns_resolver_shutdown() to finish before
destroying the resolver object. Otherwise, there would be a data race and some
fctx objects might not be destroyed yet at the time we tear down the
dns_resolver object.
This commit changes the BIND cookie algorithms to match
draft-sury-toorop-dnsop-server-cookies-00. Namely, it changes the Client Cookie
algorithm to use SipHash 2-4, adds the new Server Cookie algorithm using SipHash
2-4, and changes the default for the Server Cookie algorithm to be siphash24.
Add siphash24 cookie algorithm, and make it keep legacy aes as
Each individual test opened GeoIP databased but the database handles were never
closed. This commit moves the open/close from the individual unit tests into
the _setup and _teardown methods where they really belong.
Instead of the explicit struct initializer with all member, rely on the fact
that static variables are explicitly initialized to 0 if not explicitly
initialized.
The MSVS C compiler requires every struct to have at least one member.
The dns_geoip_databases_t structure had one set of members for
HAVE_GEOIP and a different set for HAVE_GEOIP2, and none when neither
API is in use.
This commit silences the compiler error by moving the declaration of
dns_geoip_databases_t to types.h as an opaque reference, and commenting
out the contents of geoip.h when neither version of GeoIP is enabled.
Commit b104a9bc50 introduced unconditional
use of the ATOMIC_VAR_INIT() macro in bin/dnssec/dnssec-signzone.c even
though that macro is only defined on Unix platforms. Define it on
Windows systems as well in order to prevent build failures.
The ThreadSanitizer found several possible data races in our rwlock
implementation. This commit changes all the unprotected variables to atomic and
also changes the explicit memory ordering (atomic_<foo>_explicit(..., <order>)
functions to use our convenience macros (atomic_<foo>_<order>).
The SO_BSDCOMPAT socket option is no-op since Linux 2.4, see the manpage:
SO_BSDCOMPAT
Enable BSD bug-to-bug compatibility. This is used by the UDP protocol
module in Linux 2.0 and 2.2. If enabled, ICMP errors received for a UDP
socket will not be passed to the user program. In later kernel
versions, support for this option has been phased out: Linux 2.4
silently ignores it, and Linux 2.6 generates a kernel warning (printk())
if a program uses this option. Linux 2.0 also enabled BSD bug-to-bug
compatibility options (random header changing, skipping of the broadcast
flag) for raw sockets with this option, but that was removed in Linux
2.2.
The 'managed-keys' (and 'trusted-keys') options have been deprecated
by 'dnssec-keys'. Some documentation references to 'managed-keys'
had not yet been marked or noted as such.
Adds a new option to named-checkconf, -i. If set, named-checkconf
will not warn you about deprecated options. This allows people
to use named-checkconf in automated deployment precoesses where an
operator only cares if their conf is valid, even if it is not optimal.
This was added as a request as part of introducing a policy on
removing named.conf options.
- revise mapping of search terms to database types to match the
GeoIP2 schemas.
- open GeoIP2 databases when starting up; close when shutting down.
- clarify the logged error message when an unknown database type
is configured.
- add new geoip ACL subtypes to support searching for continent in
country databases.
- map geoip ACL subtypes to specific MMDB database queries.
- perform MMDB lookups based on subtype, saving state between
queries so repeated lookups for the same address aren't necessary.
- "--with-geoip" is used to enable the legacy GeoIP library.
- "--with-geoip2" is used to enable the new GeoIP2 library
(libmaxminddb), and is on by default if the library is found.
- using both "--with-geoip" and "--with-geoip2" at the same time
is an error.
- an attempt is made to determine the default GeoIP2 database path at
compile time if pkg-config is able to report the module prefix. if
this fails, it will be necessary to set the path in named.conf with
geoip-directory
- Makefiles have been updated, and a stub lib/dns/geoip2.c has been
added for the eventual GeoIP2 search implementation.
- when processing authoritative queries for ./NS, set 'gluedb' so
that glue will be included in the response, regardless of how
'minimal-responses' has been configured.
if "rndc reload" fails, the result code is supposed to be passed to
zone_postload, but for inline-signing zones, the result can be
overwritten first by a call to the ZONE_TRYLOCK macro. this can lead
to the partially-loaded unsigned zone being synced over to the signed
zone instead of being rejected.
Since commit 0771dd3be8, <isc/mem.h> no
longer includes <isc/xml.h>. On some systems (e.g. FreeBSD), this means
that no header included by lib/dns/dnsrps.c (and no header included by
those headers) contains a definition of free() any more, which triggers
a compiler warning as lib/dns/dnsrps.c calls that function. Add the
missing #include directive to prevent that warning from being triggered.
No function called dns_dnssecsignstats_decrement() actually exists.
Putting it into lib/dns/win32/libdns.def.in breaks at least some Windows
builds. Remove the nonexistent function from that file.
The ax_check_openssl m4 macro used OPENSSL_INCLUDES. Rename the
subst variable to OPENSSL_CFLAGS and wrap AX_CHECK_OPENSSL() in
action-if-not-found part of PKG_CHECK_MODULE check for libcrypto.
The json-c have previously leaked into the global namespace leading
to forced -I<include_path> for every compilation unit using isc/xml.h
header. This MR fixes the usage making the caller object opaque.
The libxml2 have previously leaked into the global namespace leading
to forced -I<include_path> for every compilation unit using isc/xml.h
header. This MR fixes the usage making the caller object opaque.
In addition to gather how many times signatures are created per
key in a zone, also count how many of those signature creations are
because of DNSSEC maintenance. These maintenance counters are
incremented if a signature is refreshed (but the RRset did not
changed), when the DNSKEY RRset is changed, and when that leads
to additional RRset / RRSIG updates (for example SOA, NSEC).
The differences between two files are very minimal and most of the
code is common. Merge those two files and use #ifdef WIN32 to include
the right bits on Windows.
Using atomic_int_fast64_t variables with atomic functions on x86 does
not cause Visual Studio to report build errors, but such operations
yield useless results. Since the isc_stat_t type is unconditionally
typedef'd to atomic_int_fast64_t, any code performing atomic operations
on isc_stat_t variables is broken in x86 Windows builds. Fix by using
the atomic_int_fast32_t type for isc_stat_t in x86 Windows builds.
BIND 9.11.0 has bumped DNS_CLIENTINFOMETHODS_VERSION and _AGE to
version 2 and 1 in the dlz_minimal.h because a member was addet to the
dnsclientinfo struct. It was found out that the new member is not
used anywhere and there are no accessor functions therefore the change
was reverted.
Later on, it was found out that the revert caused some problems to the
users of BIND 9, and thus this changes takes a different approach by
syncing the values other way around.
The common construct seen in the BIND 9 source is func(isc_mem_t *mctx, ...).
Unfortunately, the dnstest.{h,c} has been using mctx as a global symbol, which
in turn generated a lot of errors when update.c got included in update_test.c.
As a rule of thumb, we should avoid naming global symbols with generic names
(like mctx) and we should prefix them with "namespace" (like dt_mctx).
The CHECK() macro has been defined both in dnstest.h and update.c
files. This has created a conflict between macro definitions when
including both of the files in update_test.c. While the CHECK() macro
is convenient for the tests, it has been really used in just two
files, so the MR moves them into those respective .c files.
lib/dns/tests/update_test was failing on macOS on random occasions. It
turned out this was a linker problem - it preferred isc_stdtime_get()
from libisc instead of the local version in lib/dns/tests/update_test.c.
Fix by including the original .c file in the unit test. This has two
benefits:
a) linking order may no longer cause issues as symbols found in the
same compilation unit are always preferred,
b) it allows writing tests for static functions in lib/dns/update.c.
We increase recursclients when we attach to recursion quota,
decrease when we detach. In some cases, when we hit soft
quota, we might attach to quota without increasing recursclients
gauge. We then decrease the gauge when we detach from quota,
and it causes the statistics to underflow.
Fix makes sure that we increase recursclients always when we
succesfully attach to recursion quota.
Compiling with -O3 triggers the following warnings with GCC 9.1:
task.c: In function ‘isc_taskmgr_create’:
task.c:1384:43: warning: ‘%04u’ directive output may be truncated writing between 4 and 10 bytes into a region of size 6 [-Wformat-truncation=]
1384 | snprintf(name, sizeof(name), "isc-worker%04u", i);
| ^~~~
task.c:1384:32: note: directive argument in the range [0, 4294967294]
1384 | snprintf(name, sizeof(name), "isc-worker%04u", i);
| ^~~~~~~~~~~~~~~~
task.c:1384:3: note: ‘snprintf’ output between 15 and 21 bytes into a destination of size 16
1384 | snprintf(name, sizeof(name), "isc-worker%04u", i);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
private_test.c: In function ‘private_nsec3_totext_test’:
private_test.c:110:9: warning: array subscript 4 is outside array bounds of ‘uint32_t[1]’ {aka ‘unsigned int[1]’} [-Warray-bounds]
110 | while (*sp == '\0' && slen > 0) {
| ^~~
private_test.c:103:11: note: while referencing ‘salt’
103 | uint32_t salt;
| ^~~~
Prevent these warnings from being triggered by increasing the size of
the relevant array (task.c) and reordering conditions (private_test.c).
write locking.
Unreachable cache in zonemgr is realized as an static LRU list.
When we 'use' an entry we need to update the last-used time, we
can use atomics to do so without the necessity to upgrading
read-lock to write-lock.
- change references to trusted-keys to dnssec-keys with static-key
- rebuild doc/misc/options and other generated grammar doc
- add a "see MANAGED-KEYS" note when building named.conf.docbook
- managed-keys is now deprecated as well as trusted-keys, though
it continues to work as a synonym for dnssec-keys
- references to managed-keys have been updated throughout the code.
- tests have been updated to use dnssec-keys format
- also the trusted-keys entries have been removed from the generated
bind.keys.h file and are no longer generated by bindkeys.pl.
- any use of trusted or static keys for the root zone will now
elicit a warning, regardless of what the keys may be
- ditto for any use of a key for dlv.isc.org, static or managed
- trusted-keys is now flagged as deprecated, but still works
- managed-keys can be used to configure permanent trust anchors by
using the "static-key" keyword in place of "initial-key"
- parser now uses an enum for static-key and initial-key keywords
Since 2008, the cleaning-interval timer has been documented as
"effectively obsolete" and disabled in the default configuration with
a comment saying "now meaningless".
This change deletes all the code that implements the cleaning-interval
timer, except for the config parser in whcih it is now explicitly
marked as obsolete.
I have verified (using the deletelru and deletettl cache stats) that
named still cleans the cache after this change.
Move the macOS section of <isc/endian.h> to a lower spot as it is
believed not to be the most popular platform for running BIND. Add a
comment and remove redundant definitions.
Instead of only supporting Linux, try making <isc/endian.h> support
other GNU platforms as well. Since some compilers define __GNUC__ on
BSDs (e.g. Clang on FreeBSD), move the relevant section to the bottom of
the platform-specific part of <isc/endian.h>, so that it only gets
evaluated when more specific platform determination criteria are not
met. Also include <byteswap.h> so that any byte-swapping macros which
may be defined in that file on older platforms are used in the fallback
definitions of the nonstandard hto[bl]e{16,32,64}() and
[bl]e{16,32,64}toh() conversion functions.
While Solaris does not support the nonstandard hto[bl]e{16,32,64}() and
[bl]e{16,32,64}toh() conversion functions, it does have some
byte-swapping macros available in <sys/byteorder.h>. Ensure these
macros are used in the fallback definitions of the aforementioned
nonstandard functions.
Since the hto[bl]e{16,32,64}() and [bl]e{16,32,64}toh() conversion
functions are nonstandard, add fallback definitions of these functions
to <isc/endian.h>, so that their unavailability does not prevent
compilation from succeeding.
Current versions of DragonFly BSD, FreeBSD, NetBSD, and OpenBSD all
support the modern variants of functions converting values between host
and big-endian/little-endian byte order while older ones might not.
Ensure <isc/endian.h> works properly in both cases.
There's a small possibility of race between udp dispatcher and
socket code - socket code can still hold internal reference to a
socket while dispatcher calls isc_socket_open, which can cause
an assertion failure. Fix it by relaxing the assertion test, and
instead simply locking the socket in isc_socket_open.
qname minimization, even in relaxed mode, can fail on
some very broken domains. In relaxed mode, instead of
asking for "foo.bar NS" ask for "_.foo.bar A" to either
get a delegation or NXDOMAIN. It will require more queries
than regular mode for proper NXDOMAINs.
