The basic scenario for the problem was that in the process of
resolving a query, if any rrset was eligible for prefetching, then it
would trigger a call to query_prefetch(), this call would run in
parallel to the normal query processing.
The problem arises due to the fact that both query_prefetch(), and,
in the original thread, a call to ns_query_recurse(), try to attach
to the recursionquota, but recursing client stats counter is only
incremented if ns_query_recurse() attachs to it first.
Conversely, if fetch_callback() is called before prefetch_done(),
it would not only detach from recursionquota, but also decrement
the stats counter, if query_prefetch() attached to te quota first
that would result in a decrement not matched by an increment, as
expected.
To solve this issue an atomic bool was added, it is set once in
ns_query_recurse(), allowing fetch_callback() to check for it
and decrement stats accordingly.
For a more compreensive explanation check the thread comment below:
https://gitlab.isc.org/isc-projects/bind9/-/issues/1719#note_145857
- clone keynode->dsset rather than return a pointer so that thread
use is independent of each other.
- hold a reference to the dsset (keynode) so it can't be deleted
while in use.
- create a new keynode when removing DS records so that dangling
pointers to the deleted records will not occur.
- use a rwlock when accessing the rdatalist to prevent instabilities
when DS records are added.
(cherry picked from commit e5b2eca1d3)
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.
(cherry picked from commit 78886d4bed)
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.
(cherry picked from commit 0002377dca)
Start enforcing the clang-format rules on changed files
Closes#46
See merge request isc-projects/bind9!3063
(cherry picked from commit a04cdde45d)
d2b5853b Start enforcing the clang-format rules on changed files
618947c6 Switch AlwaysBreakAfterReturnType from TopLevelDefinitions to All
654927c8 Add separate .clang-format files for headers
5777c44a Reformat using the new rules
60d29f69 Don't enforce copyrights on .clang-format
adjust clang-format options to get closer to ISC style
See merge request isc-projects/bind9!3061
(cherry picked from commit d3b49b6675)
0255a974 revise .clang-format and add a C formatting script in util
e851ed0b apply the modified style
Add curly braces using uncrustify and then reformat with clang-format back
Closes#46
See merge request isc-projects/bind9!3057
(cherry picked from commit 67b68e06ad)
36c6105e Use coccinelle to add braces to nested single line statement
d14bb713 Add copy of run-clang-tidy that can fixup the filepaths
056e133c Use clang-tidy to add curly braces around one-line statements
Reformat source code with clang-format
Closes#46
See merge request isc-projects/bind9!2156
(cherry picked from commit 7099e79a9b)
4c3b063e Import Linux kernel .clang-format with small modifications
f50b1e06 Use clang-format to reformat the source files
11341c76 Update the definition files for Windows
df6c1f76 Remove tkey_test (which is no-op anyway)
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.
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.
- ns__client_request() is now called by netmgr with an isc_nmhandle_t
parameter. The handle can then be permanently associated with an
ns_client object.
- The task manager is paused so that isc_task events that may be
triggred during client processing will not fire until after the netmgr is
finished with it. Before any asynchronous event, the client MUST
call isc_nmhandle_ref(client->handle), to prevent the client from
being reset and reused while waiting for an event to process. When
the asynchronous event is complete, isc_nmhandle_unref(client->handle)
must be called to ensure the handle can be reused later.
- reference counting of client objects is now handled in the nmhandle
object. when the handle references drop to zero, the client's "reset"
callback is used to free temporary resources and reiniialize it,
whereupon the handle (and associated client) is placed in the
"inactive handles" queue. when the sysstem is shutdown and the
handles are cleaned up, the client's "put" callback is called to free
all remaining resources.
- because client allocation is no longer handled in the same way,
the '-T clienttest' option has now been removed and is no longer
used by any system tests.
- the unit tests require wrapping the isc_nmhandle_unref() function;
when LD_WRAP is supported, that is used. otherwise we link a
libwrap.so interposer library and use that.
This commit was done by hand to add the RUNTIME_CHECK() around stray
dns_name_copy() calls with NULL as third argument. This covers the edge cases
that doesn't make sense to write a semantic patch since the usage pattern was
unique or almost unique.
This second commit uses second semantic patch to replace the calls to
dns_name_copy() with NULL as third argument where the result was stored in a
isc_result_t variable. As the dns_name_copy(..., NULL) cannot fail gracefully
when the third argument is NULL, it was just a bunch of dead code.
Couple of manual tweaks (removing dead labels and unused variables) were
manually applied on top of the semantic patch.
This commit add RUNTIME_CHECK() around all simple dns_name_copy() calls where
the third argument is NULL using the semantic patch from the previous commit.
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.
- 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.
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.
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.
- Always set is_zonep in query_getdb; previously it was only set if
result was ISC_R_SUCCESS or ISC_R_NOTFOUND.
- Don't reset is_zone for redirect.
- Style cleanup.
(cherry picked from commit a85cc641d7a4c66cbde03cc4e31edc038a24df46)
(cherry picked from commit 486a201149)
- named could return FORMERR if parsing iterative responses
ended with a result code such as DNS_R_OPTERR. instead of
computing a response code based on the result, in this case
we now just force the response to be SERVFAIL.
this restores functionality that was removed in commit 03be5a6b4e,
allowing named to search in authoritative zone databases outside the
current zone for additional data, if and only if recursion is allowed
and minimal-responses is disabled.
in query_respond_any(), the assumption had previously been made that it
was impossible to get past iterating the node with a return value of
ISC_R_NOMORE but not have found any records, unless we were searching
for RRSIG or SIG. however, it is possible for other types to exist but
be hidden, such as when the zone is transitioning from insecure to
secure and DNSSEC types are encountered, and this situation could
trigger an assertion. removed the assertion and reorganized the code.
This change makes rndc dumpdb correctly print the "; stale" line.
It also provides extra information on how long this data may still
be served to clients (in other words how long the stale RRset may
still be used).
- "hook" is now used only for hook points and hook actions
- the "hook" statement in named.conf is now "plugin"
- ns_module and ns_modlist are now ns_plugin and ns_plugins
- ns_module_load is renamed ns_plugin_register
- the mandatory functions in plugin modules (hook_register,
hook_check, hook_version, hook_destroy) have been renamed
- added some hook points that will be needed for a dns64 module later
- moved some code from the beginning of query_respond() to
the end of query_prepresponse(); this has no effect on functionality
but means we can have a hook point at the top of query_respond(),
which seems nicer
- compressed duplicated code into query_zerottl_refetch() function
- added a qctx->answered flag so that a module can prevent
query_addrrset() from being called from query_respond() when
it's already been called from the module.
- this is necessary because adding the same hook to multiple views
causes the ISC_LIST link value to become inconsistent; it isn't
noticeable when only one hook action is ever registered at a
given hook point, but it will break things when there are two.
- eliminate qctx->hookdata and client->hookflags.
- use a memory pool to allocate data blobs in the filter-aaaa module,
and associate them with the client address in a hash table
- instead of detaching the client in query_done(), mark it for deletion
and then call ns_client_detach() from qctx_destroy(); this ensures
that it will still exist when the QCTX_DESTROYED hook point is
reached.
- use a get_hooktab() function to determine the hook table.
- PROCESS_HOOK now jumps to a cleanup tag on failure
- add PROCESS_ALL_HOOKS in query.c, to run all hook functions at
a specified hook point without stopping. this is to be used for
intiialization and destruction functions that must run in every
module.
- 'result' is set in PROCESS_HOOK only when a hook function
interrupts processing.
- revised terminology: a "callback" is now a "hook action"
- remove unused NS_PROCESS_HOOK and NS_PROCESS_HOOK_VOID macros.
- added a 'hookdata' array to qctx to store pointers to up to
16 blobs of data which are allocated by modules as needed.
each module is assigned an ID number as it's loaded, and this
is the index into the hook data array. this is to be used for
holding persistent state between calls to a hook module for a
specific query.
- instead of using qctx->filter_aaaa, we now use qctx->hookdata.
(this was the last piece of filter-aaaa specific code outside the
module.)
- added hook points for qctx initialization and destruction. we get
a filter-aaaa data pointer from the mempool when initializing and
store it in the qctx->hookdata table; return to to the mempool
when destroying the qctx.
- link the view to the qctx so that detaching the client doesn't cause
hooks to fail
- added a qctx_destroy() function which must be called after qctx_init;
this calls the QCTX_DESTROY hook and detaches the view
- general cleanup and comments
