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.
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));
The isc_refcount_decrement() was either used as:
if (isc_refcount_decrement() == 1) { destroy(); }
or
if (isc_refcount_decrement() != 1) { return; } destroy();
This commits eradicates the last usage of the later, so the code is unified to
use the former.
when looking for a possible wildcard match in the RPZ summary database,
use an rbtnodechain to walk up label by label, rather than using the
node's parent pointer.
In dns_rpz_update_from_db we call setup_update which creates the db
iterator and calls dns_dbiterator_first. This unpauses the iterator and
might cause db->tree_lock to be acquired. We then do isc_task_send(...)
on an event to do quantum_update, which (correctly) after each iteration
calls dns_dbiterator_pause, and re-isc_task_sends itself.
That's an obvious bug, as we're holding a lock over an async task send -
if a task requesting write (e.g. prune_tree) is scheduled on the same
workers queue as update_quantum but before it, it will wait for the
write lock indefinitely, resulting in a deadlock.
To fix it we have to pause dbiterator in setup_update.
As pointed out in !813 db_registered is sort of redundant. It is
set to `true` only in `dns_zone_rpz_enable_db()` right before the
`dns_rpz_dbupdate_callback()` callback is registered. It is only
required in that callback and it is the only place that the callback
is registered. Therefore there is no path that that `REQUIRE` can
fail.
The `db_registered` variable is only set to `false` in
`dns_rpz_new_zone`, so it is not like the variable is unset again
later.
The only other place where `db_registered` is checked is in
`rpz_detach()`. If `true`, it will call
`dns_db_updatenotify_unregister()`. However if that happens, the
`db_registered` is not set back to `false` thus this implies that
this may happen multiple times. If called a second time, most
likely the unregister function will return `ISC_R_NOTFOUND`, but
the return value is not checked anyway. So it can do without the
`db_registered` check.
This may happen when loading an RPZ failed and the code path skips
calling dns_db_endload(). The dns_rpz_zone_t object is still kept
marked as having registered db. So when this object is finally
destroyed in rpz_detach(), this code will incorrectly call
`dns_db_updatenotify_unregister()`:
if (rpz->db_registered)
dns_db_updatenotify_unregister(rpz->db,
dns_rpz_dbupdate_callback, rpz);
and trigger this assertion failure:
REQUIRE(db != NULL);
To fix this, only call `dns_db_updatenotify_unregister()` when
`rpz->db` is not NULL.
While implementing the new unit testing framework cmocka, it was found that the
BIND 9 code doesn't compile when assertions are disabled or replaced with any
function (such as mock_assert() from cmocka unit testing framework) that's not
directly recognized as assertion by the compiler.
This made the compiler to complain about blocks of code that was recognized as
unreachable before, but now it isn't.
The changes in this commit include:
* assigns default values to couple of local variables,
* moves some return statements around INSIST assertions,
* adds __builtin_unreachable(); annotations after some INSIST assertions,
* fixes one broken assertion (= instead of ==)
If an RPZ zone is to be freed during an update, canceling the
update_quantum() event is not enough because the resources released when
an update completes also need to be accounted for. Failure to do this
results in a hang upon shutdown. Fix by copying cleanup code from the
end of update_quantum() to rpz_detach().
If another RPZ update is pending when processing the previous one nears
completion and min-update-interval is set to 0, isc_timer_reset() gets
called with 'interval' set to 0, which triggers an assertion failure.
To prevent such a scenario from causing a crash, queue the update event
directly instead of asking the timer thread to do it.
This properly orders clearing the freed pointer and calling isc_refcount_destroy
as early as possible to have ability to put proper memory barrier when cleaning
up reference counting.
Replace dns_fixedname_init() calls followed by dns_fixedname_name()
calls with calls to dns_fixedname_initname() where it is possible
without affecting current behavior and/or performance.
This patch was mostly prepared using Coccinelle and the following
semantic patch:
@@
expression fixedname, name;
@@
- dns_fixedname_init(&fixedname);
...
- name = dns_fixedname_name(&fixedname);
+ name = dns_fixedname_initname(&fixedname);
The resulting set of changes was then manually reviewed to exclude false
positives and apply minor tweaks.
It is likely that more occurrences of this pattern can be refactored in
an identical way. This commit only takes care of the low-hanging fruit.
