The dns_message_gettempname(), dns_message_gettemprdata(),
dns_message_gettemprdataset(), and dns_message_gettemprdatalist() always
succeeds because the memory allocation cannot fail now. Change the API
to return void and cleanup all the use of aforementioned functions.
3034 next = ISC_LIST_NEXT(query, link);
3035 } else {
3036 next = NULL;
3037 }
CID 352554 (#1 of 1): Dereference before null check (REVERSE_INULL)
check_after_deref: Null-checking connectquery suggests that it may be null, but it has already been dereferenced on all paths leading to the check.
3038 if (connectquery != NULL) {
3039 query_detach(&connectquery);
3040 }
- var_decl: Declaring variable "tbuf" without initializer
- assign: Assigning: "target.base" = "tbuf", which points to
uninitialized data
- assign: Assigning: "r.base" = "target.base", which points to
uninitialized data
I expect it would correctly initialize length always. Add simple
initialization to silent coverity.
There was a query_detach() call missing in dig, which could lead to
dig hanging on TLS context creation errors. This commit fixes.
The error was introduced because the Strict TLS implementation was
initially made over an older version of the code, where this extra
query_detach() call was not needed.
Man pages for dig/mdig/delv used `.. option:: +[no]bla` to describe two
options at once, and very old Sphinx does not support that [] in option
names.
Solution is to split negative and positive options into `+bla, +nobla`
form. In the end it improves readability because it transforms hard to
read strings with double brackets from
`+[no]subnet=addr[/prefix-length]` to
`+subnet=addr[/prefix-length], +nosubnet`.
As a side-effect it also allows easier linking to dig/mdig/delv options
using their name directly instead of always overriding the link target
to `+[no]bla` form.
Transformation was done using regex:
s/:: +\[no\]\(.*\)/:: +\1, +no\1
... and manual review around occurences matching regex
+no.*=
Fixes: #3301
dig previously set an exit code of 9 when a TCP connection failed
or when a UDP connection timed out, but when the server address is
localhost it's possible for a UDP query to fail with ISC_R_CONNREFUSED.
that code path didn't update the exit code, causing dig to exit with
status 0. we now set the exit code to 9 in this failure case.
In `+nssearch` mode `dig` starts the next query of the followup lookup
using `start_udp()` or `start_tcp()` calls without waiting for the
previous query to complete.
In UDP mode that happens in the `send_done()` callback of the previous
query, but in TCP mode that happens in the `start_tcp()` call of the
previous query (recursion) which doesn't work because `start_tcp()`
attaches the `lookup->current_query` to the query it is starting, so a
recursive call will result in an assertion failure.
Make the TCP mode to start the next query in `send_done()`, just like in
the UDP mode. During that time the `lookup->current_query` is already
detached by the `tcp_connected()`/`udp_ready()` callbacks.
The error code path handling the `ISC_R_CANCELED` code lacks a
`clear_current_lookup()` call, without which dig hangs indefinitely
when handling the error.
Add the missing call to account for all references of the lookup so
it can be destroyed.
In `send_udp()` and `launch_next_query()` functions, when calling
`dighost_printmessage()` to print detailed information about the
sent query, dig always prints the data of the first query in the
lookup's queries list.
The first query in the list can be already finished, having its handles
freed, and accessing this information results in assertion failure.
Print the current query's information instead.
In recv_done(), when dig decides to start the lookup's next query in
the line using `start_udp()` or `start_tcp()`, and for some reason,
no queries get started, dig doesn't cancel the lookup.
This can occur, for example, when there are two queries in the lookup,
one with a regular IP address, and another with a IPv4 mapped IPv6
address. When the regular IP address fails to serve the query, its
`recv_done()` callback starts the next query in the line (in this
case the one with a mapped IP address), but because `dig` doesn't
connect to such IP addresses, and there are no other queries in the
list, no new queries are being started, and the lookup keeps hanging.
After calling `start_udp()` or `start_tcp()` in `recv_done()`, check
if there are no pending/working queries then cancel the lookup instead
of only detaching from the current query.
The `udp_ready()` and `tcp_connected()` functions in dighost.c are
used for similar purposes for UDP and TCP respectively.
Synchronize the `udp_ready()` function entry code to behave like
`tcp_connected()` by adding input validation, debug messages and
early exit code when `cancel_now` is `true`.
When finishing the NSSEARCH task and there is no more followup
lookups to start, dig does not destroy the last lookup, which
causes it to hang indefinitely.
Rename the unused `first_pass` member of `dig_query_t` to `started`
and make it `true` in the first callback after `start_udp()` or
`start_tcp()` of the query to indicate that the query has been
started.
Create a new `check_if_queries_done()` function to check whether
all of the queries inside a lookup have been started and finished,
or canceled.
Use the mentioned function in the TRACE code block in `recv_done()`
to check whether the current query is the last one in the lookup and
cancel the lookup in that case to free the resources.
This commit adds support for Strict/Mutual TLS to dig.
The new command-line options and their behaviour are modelled after
kdig (+tls-ca, +tls-hostname, +tls-certfile, +tls-keyfile) for
compatibility reasons. That is, using +tls-* is sufficient to enable
DoT in dig, implying +tls-ca
If there is no other DNS transport specified via command-line,
specifying any of +tls-* options makes dig use DoT. In this case, its
behaviour is the same as if +tls-ca is specified: that is, the remote
peer's certificate is verified using the platform-specific
intermediate CA certificates store. This behaviour is introduced for
compatibility with kdig.
Previously, it was possible to assign a bit of memory space in the
nmhandle to store the client data. This was complicated and prevents
further refactoring of isc_nmhandle_t caching (future work).
Instead of caching the data in the nmhandle, allocate the hot-path
ns_client_t objects from per-thread clientmgr memory context and just
assign it to the isc_nmhandle_t via isc_nmhandle_set().
C11 has builtin support for _Noreturn function specifier with
convenience noreturn macro defined in <stdnoreturn.h> header.
Replace ISC_NORETURN macro by C11 noreturn with fallback to
__attribute__((noreturn)) if the C11 support is not complete.
Previously, the unreachable code paths would have to be tagged with:
INSIST(0);
ISC_UNREACHABLE();
There was also older parts of the code that used comment annotation:
/* NOTREACHED */
Unify the handling of unreachable code paths to just use:
UNREACHABLE();
The UNREACHABLE() macro now asserts when reached and also uses
__builtin_unreachable(); when such builtin is available in the compiler.
Gcc 7+ and Clang 10+ have implemented __attribute__((fallthrough)) which
is explicit version of the /* FALLTHROUGH */ comment we are currently
using.
Add and apply FALLTHROUGH macro that uses the attribute if available,
but does nothing on older compilers.
In one case (lib/dns/zone.c), using the macro revealed that we were
using the /* FALLTHROUGH */ comment in wrong place, remove that comment.
When encountering a TCP connection error while trying to initiate a
connection to a server, dig erroneously cancels the lookup even when
there are other server(s) to try, which results in an assertion failure.
Cancel the lookup only when there are no more queries left in the
lookup's queries list (i.e. `next` is NULL).
When timing-out or having other types of socket errors during a query,
dig isn't trying to perform the lookup using other servers which exist
in the lookup's queries list.
After configured amount of timeout retries, or after a socket error,
check if there are other queries/servers in the lookup's queries list,
and start the next one if it exists, instead of unconditionally failing.
When a query times out, and `dig` (or `host`) creates a new query
to resend the request, it is being prepended to the lookup's queries
list, which can cause a confusion later, making `dig` (or `host`)
believe that there is another new query in the list, but that is
actually the old one, which was timed out. That mistake will result
in an assertion failure.
That can happen, in particular, when after a timed out request,
the retried request returns a SERVFAIL result, and the recursion
is enabled, and `+nofail` option was used with `dig` (that is the
default behavior in `host`, unless the `-s` option is provided).
Fix the problem by inserting the query just after the current,
timed-out query, instead of prepending to the list.
Before calling start_udp() detach `l->current_query`, like it is
done in another place in the function.
Slightly update a couple of debug messages to make them more
consistent.
After a query results in a SERVFAIL result, and there is another
registered query in the lookup's queries list, `dig` starts the next
query to try another server, but for some reason, reports about that
also when the current query is in the head of the list, even if there
is no other query in the list to try.
Use the same condition for both decisions, and after starting the next
query, jump to the "detach_query" label instead of "next_lookup",
because there is no need to start the next lookup after we just started
a query in the current lookup.
The clang-format-15 has new option InsertBraces that could add missing
branches around single line statements. Use that to our advantage
without switching to not-yet-released LLVM version to add missing braces
in couple of places.
Replace :manpage: with :iscman: to generate internal hyperlinks. That
way reader can use links even when offline, and jumps to man pages
for the same version.
Formerly HTML version of man pages did not have links in See Also
section because :manpage: role in Sphinx can generate only external
hyperlinks - and we do not have that enabled.
Enabling the Sphinx :manpage: linking could reliably create hyperlinks
only to external URLs, but that would take users to another version
of docs.
Generated by:
find bin -name '*.rst' | xargs sed -i -e 's/:manpage:`\([^(]\+\)(\([0-9]\))`/:iscman:`\1(\2) <\1>`/g'
+ hand-edit to revert change for mmencode reference which is
not provided in our source tree.
Use the new role :iscman: to replace all occurences or ``binary``
with :iscman:`binary`, creating a hyperlink to the manual page.
Generated using:
find bin -name *.rst | xargs fgrep --files-with-matches '.. iscman' | xargs -I{} -n1 basename {} .rst > /tmp/progs
for PROG in $(cat /tmp/progs); do find -name '*.rst' | xargs sed -i -e "s/\`\`$PROG\`\`/:iscman:\`$PROG\`/g"; done
Additional hand-edits were done mainly around filter-aaaa and
filter-a which are program names and and option names at the
same time. Couple more edits was neede to fix .rst syntax broken by
automatic replacement.
Sphinx has it's own :program: syntax for refering to program names.
Use it for self-references in manual pages. These self-references are
not clickable and not as eye-cathing as links, which is a good thing.
There is no point in attracting attention to ``dig`` several times on a
single page dedicated to dig itself.
Substituted automatically using:
find bin -name *.rst | xargs fgrep --files-with-matches '.. program' | xargs -n1 bash /tmp/repl.sh
With /tmp/repl.sh being:
BASE=$(basename "$1" .rst)
sed -i -e "s/\`\`$BASE\`\`/:program:\`$BASE\`/g" "$1"
The new directive and role "iscman" allow to tag & reference man pages in
our source tree. Essentially it is just namespacing for ISC man pages,
but it comes with couple benefits.
Differences from .. _man_program label we formerly used:
- Does not expand :ref:`man_program` into full text of the page header.
- Generates index entry with category "manual page".
- Rendering style is closer to ubiquitous to the one produced
by ``named`` syntax.
Differences from Sphinx built-in :manpage: role:
- Supports all builders with support for cross-references.
- Generates internal links (unlike :manpage: which generates external
URLs).
- Checks that target exists withing our source tree.
Side-effect of hyperlinking is that typos in program and option names
are now detected by Sphinx.
Candidate -options were detected using:
find -name *.rst | xargs grep '``-[^`]'
and then modified from ``-o`` to :option:`-o` using regex
s/``\(-[^`]\+\)``/:option:`\1`/
+ manual modifications where necessary.
Non-hyphenated options were detected by looking at context around
program names:
find bin -name *.rst | xargs -I{} -n1 basename {} .rst | sort -u
and grepping for program name with trailing whitespace.
Stand-alone program names like ``named`` are not hyperlinked in this
commit.
The markup allows referencing individual options, and also makes them
more legible (no more thin red text on gray background).
Most of the work was done using regexes:
s/^``-\(.*\)``$/.. option:: -\1\r/
s/^``+\(.*\)``$/.. option:: +\1\r/
on bin/**/*.rst files along with visual inspection and hand-edits,
mostly for positional arguments.
Regex for rndc.rst:
s/^``\(.*\)``/.. option:: \1\r/
+ hand edits to remove extra asterisk and whitespace here and there.
When dig was built without IDN support, it reported an error if the
+noidnin and/or +noidnout options were used. This means the options
were not useful for a script that wants consistent lack of IDN
translation regardless of how BIND is built.
Make dig complain about lack of built-in IDN support only when the
user asks for IDN translation.
Closes#3188
The C17 standard deprecated ATOMIC_VAR_INIT() macro (see [1]). Follow
the suite and remove the ATOMIC_VAR_INIT() usage in favor of simple
assignment of the value as this is what all supported stdatomic.h
implementations do anyway:
* MacOSX.plaform: #define ATOMIC_VAR_INIT(__v) {__v}
* Gcc stdatomic.h: #define ATOMIC_VAR_INIT(VALUE) (VALUE)
1. http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2018/p1138r0.pdf
For the reference, the _cancel_lookup() function iterates through
the lookup's queries list and detaches them. In the ideal scenario,
that should be the last reference and the query will be destroyed
after that, but it is also possible that we are still expecting a
callback, which also holds a reference (for example, _cancel_lookup()
could have been called from recv_done(), when send_done() was still
not executed).
The start_udp() and start_tcp() functions are currently designed in
slightly different ways: start_udp() creates a new query attachment
`connectquery`, to be called in the callback function, while
start_tcp() does not, which is a bug, but is hidden by the fact
that when the query is being erroneously destroyed prematurely (before
_cancel_lookup() is called) in the result of that, it also gets
de-listed from the lookup's queries' list, so _cancel_lookup() doesn't
even try to detach it.
For better understanding, here's an illustration of the query's
references count changes, and from where it was changed:
UDP
---
1. _new_query() -> refcount = 1 (initial)
2. start_udp() -> refcount = 2 (lookup->current_query)
3. start_udp() -> refcount = 3 (connectquery)
4. udp_ready() -> refcount = 4 (readquery)
5. udp_ready() -> refcount = 5 (sendquery)
6. udp_ready() -> refcount = 4 (lookup->current_query)
7. udp_ready() -> refcount = 3 (connectquery)
8. send_done() -> refcount = 2 (sendquery)
9. recv_done() -> refcount = 1 (readquery)
10. _cancel_lookup() -> refcount = 0 (initial)
11. the query gets destroyed and removed from `lookup->q`
TCP, fortunate scenario
-----------------------
1. _new_query() -> refcount = 1 (initial)
2. start_tcp() -> refcount = 2 (lookup->current_query)
3. launch_next_query() -> refcount = 3 (readquery)
4. launch_next_query() -> refcount = 4 (sendquery)
5. tcp_connected() -> refcount = 3 (lookup->current_query)
6. tcp_connected() -> refcount = 2 (bug, there was no connectquery)
7. send_done() -> refcount = 1 (sendquery)
8. recv_done() -> refcount = 0 (readquery)
9. the query gets prematurely destroyed and removed from `lookup->q`
10. _cancel_lookup() -> the query is not in `lookup->q`
TCP, unfortunate scenario, revealing the bug
--------------------------------------------
1. _new_query() -> refcount = 1 (initial)
2. start_tcp() -> refcount = 2 (lookup->current_query)
3. launch_next_query() -> refcount = 3 (readquery)
4. launch_next_query() -> refcount = 4 (sendquery)
5. tcp_connected() -> refcount = 3 (lookup->current_query)
6. tcp_connected() -> refcount = 2 (bug, there was no connectquery)
7. recv_done() -> refcount = 1 (readquery)
8. _cancel_lookup() -> refcount = 0 (the query was in `lookup->q`)
9. we hit an assertion here when trying to destroy the query, because
sendhandle is not detached (which is done by send_done()).
10. send_done() -> this never happens
This commit does the following:
1. Add a `connectquery` attachment in start_tcp(), like done in
start_udp().
2. Add missing _cancel_lookup() calls for error scenarios, which
were possibly missing because before fixing the bug, calling
_cancel_lookup() and then calling query_detach() would cause
an assertion.
3. Log a debug message and call isc_nm_cancelread(query->readhandle)
for every query in the lookup from inside the _cancel_lookup()
function, like it is done in _cancel_all().
4. Add a `canceled` property for the query which becomes `true` when
the lookup (and subsequently, its queries) are canceled.
5. Use the `canceled` property in the network manager callbacks to
know that the query was canceled, and act like `eresult` was equal
to `ISC_R_CANCELED`.
There was a missing UNLOCK_LOOKUP in the recv_done() callback when
the operation had been canceled. That omission could result in a
deadlock situation.
The terms "DNS over HTTPS" and "DNS over TLS" should be hyphenated when
they are used as adjectives and non-hyphenated otherwise. Ensure all
occurrences of these terms in the source tree follow the above rule.
(CHANGES and release notes are intentionally left intact.)
Tweak a related ARM snippet, fixing a typo in the process.
This commit converts the license handling to adhere to the REUSE
specification. It specifically:
1. Adds used licnses to LICENSES/ directory
2. Add "isc" template for adding the copyright boilerplate
3. Changes all source files to include copyright and SPDX license
header, this includes all the C sources, documentation, zone files,
configuration files. There are notes in the doc/dev/copyrights file
on how to add correct headers to the new files.
4. Handle the rest that can't be modified via .reuse/dep5 file. The
binary (or otherwise unmodifiable) files could have license places
next to them in <foo>.license file, but this would lead to cluttered
repository and most of the files handled in the .reuse/dep5 file are
system test files.
Disable IDN2_USE_STD3_ASCII_RULES to the libidn2 conversion because it
broke encoding some non-letter but valid domain names like _tcp or *.
This reverts commit ef8aa91740.
This commit adds an isc_nm_socket_type() function which can be used to
obtain a handle's socket type.
This change obsoletes isc_nm_is_tlsdns_handle() and
isc_nm_is_http_handle(). However, it was decided to keep the latter as
we eventually might end up supporting multiple HTTP versions.
Unify the header guard style and replace the inconsistent include guards
with #pragma once.
The #pragma once is widely and very well supported in all compilers that
BIND 9 supports, and #pragma once was already in use in several new or
refactored headers.
Using simpler method will also allow us to automate header guard checks
as this is simpler to programatically check.
For reference, here are the reasons for the change taken from
Wikipedia[1]:
> In the C and C++ programming languages, #pragma once is a non-standard
> but widely supported preprocessor directive designed to cause the
> current source file to be included only once in a single compilation.
>
> Thus, #pragma once serves the same purpose as include guards, but with
> several advantages, including: less code, avoidance of name clashes,
> and sometimes improvement in compilation speed. On the other hand,
> #pragma once is not necessarily available in all compilers and its
> implementation is tricky and might not always be reliable.
1. https://en.wikipedia.org/wiki/Pragma_once
libidn2 defaults to UseSTD3ASCIIRules=false. That allows arbitrary ASCII
characters to show up in the toASCII output, including space and
underscore. Enable IDN2_USE_STD3_ASCII_RULES to the libidn2 conversion
to disallow additional characters from the conversion (see Validity
Criteria[1]).
