When "rndc reconfig" is run, named first configures a fresh set of views
and then tears down the old views. Consider what happens for a single
view with LMDB enabled; "envA" is the pointer to the LMDB environment
used by the original/old version of the view, "envB" is the pointer to
the same LMDB environment used by the new version of that view:
1. mdb_env_open(envA) is called when the view is first created.
2. "rndc reconfig" is called.
3. mdb_env_open(envB) is called for the new instance of the view.
4. mdb_env_close(envA) is called for the old instance of the view.
This seems to have worked so far. However, an upstream change [1] in
LMDB which will be part of its 0.9.26 release prevents the above
sequence of calls from working as intended because the locktable mutexes
will now get destroyed by the mdb_env_close() call in step 4 above,
causing any subsequent mdb_txn_begin() calls to fail (because all of the
above steps are happening within a single named process).
Preventing the above scenario from happening would require either
redesigning the way we use LMDB in BIND, which is not something we can
easily backport, or redesigning the way BIND carries out its
reconfiguration process, which would be an even more severe change.
To work around the problem, set MDB_NOLOCK when calling mdb_env_open()
to stop LMDB from controlling concurrent access to the database and do
the necessary locking in named instead. Reuse the view->new_zone_lock
mutex for this purpose to prevent the need for modifying struct dns_view
(which would necessitate library API version bumps). Drop use of
MDB_NOTLS as it is made redundant by MDB_NOLOCK: MDB_NOTLS only affects
where LMDB reader locktable slots are stored while MDB_NOLOCK prevents
the reader locktable from being used altogether.
[1] https://git.openldap.org/openldap/openldap/-/commit/2fd44e325195ae81664eb5dc36e7d265927c5ebc
(cherry picked from commit 53120279b5)
Assign and then check node for NULL to address another thread
changing radix->head in the meantime.
Move 'node != NULL' check into while loop test to silence cppcheck
false positive.
Fix pointer != NULL style.
(cherry picked from commit 51f08d2095)
RBTDB node can now appear on the deadnodes lists following the changes
to decrement_reference in 4db29f1f7d to
defer checking of node->down when the tree write lock is not held. The
node should be unlinked instead.
(cherry picked from commit 569cc155b8680d8ed12db1fabbe20947db24a0f9)
We were passing client address to dns_resolver_createfetch as a pointer
and it was saved as a pointer. The client (with its address) could be
gone before the fetch is finished, and in a very odd scenario
log_formerr would call isc_sockaddr_format() which first checks if the
address family is valid (and at this point it still is), then the
sockaddr is cleared, and then isc_netaddr_fromsockaddr is called which
fails an assertion as the address family is now invalid.
(cherry picked from commit 175c4d9055)
In [RT #43367], the BIND 9 changed the strictness of address / prefix
length checks:
Check prefixes in acls to make sure the address and
prefix lengths are consistent. Warn only in
BIND 9.11 and earlier.
Unfortunately, a regression slipped in and the check was made an error
also in the BIND 9.11. This commit fixes the regression, but turning
the error into a warning.
In case of a test failure we weren't tearing down sockets and tasks
properly, causing the test to hang instead of failing nicely.
(cherry picked from commit 4a8d9250cf)
If there are more that 5 NS record for a zone only perform a
maximum of 4 address lookups for all the name servers. This
limits the amount of remote lookup performed for server
addresses at each level for a given query.
lib/dns/sdb.c:1145:warning: Either the condition '(sdb)!=((void*)0)' is
redundant or there is possible null pointer dereference: sdb."
lib/dns/sdb.c:1208: Either the condition '(node)!=((void*)0)' is
redundant or there is possible null pointer dereference: sdbnode.
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.
(cherry picked from commit a66c6fc883)
When we were printing quoted string, the double quotes where unescaped
leading to prematurely ending the quoted string.
(cherry picked from commit b02081d423)
The _getcwd() compatibility function on Microsoft CRT requires direct.h
header to pull a proper function prototype.
The (misleading) warning generated:
bin\named\server.c(1074): warning C4047: '==': 'int' differs in levels of indirection from 'void *'
The MSVC detects mismatch of const qualifiers in cfg_map_{first,next}clause
functions just in BIND 9.11. As this is harmless in this particular
case, just disable the warning for these two functions.
The warnings reported:
lib\isccfg\parser.c(2160): warning C4090: '=': different 'const' qualifiers
lib\isccfg\parser.c(2184): warning C4090: '=': different 'const' qualifiers
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.
(cherry picked from commit b6c2012d93)
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
(cherry picked from commit 789d253e3d)
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.
This is the warning:
In file included from print_test.c:47:
./../print.c:203:9: warning: Although the value stored to 'neg' is used in the enclosing expression, the value is never actually read from 'neg'
dot = neg = space = plus = left = zero = alt = h = l = q = z = 0;
^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.
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.
(cherry picked from commit 262f087bcf)
The libisc LIBINTERFACE bump for 9.11.17 is unnecessary.
A lot of headers were altered but the ABI tool did not report anything.
Trust the ABI tool on this and decrement LIBINTERFACE and increment
LIBREVISION.
Bumped the version file and added release line in CHANGES.
API files:
- lib/bind9/api:
No changes because only changes in comments.
- lib/dns/api:
Increment LIBINTERFACE because of the added field structure in
dns_struct_update.
- lib/isc/api:
Increment LIBINTERFACE because of the PKCS#11 replacement.
- lib/isccc/api:
No changes because no source code changes.
- lib/isccfg/api:
Increment LIBREVISION because of minor source code changes.
- lib/lwres/api:
No changes because no source code changes.
I decided no changes to README.md or the release notes were necessary.
Perflab graphs look sane.
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.
(cherry picked from commit 08f4c7d6c0)
some empty conditional branches which contained a semicolon were
"fixed" by clang-format to contain nothing. add comments to prevent this.
(cherry picked from commit 735be3b816)
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'.
(cherry picked from commit 8dd8d48c9f)
Mark Andrews
Michał Kępień
Witold KręcickiandOndřej Surý
Evan Hunt
Tinderbox UserandMichał Kępień
Diego FronzaandOndřej Surý
