After f10f5572ac spatch started reporting
the following warnings:
Impossible: How can diff be null and have not Correct in compare_c? Tag1 ("diff token: ( VS (\nFile \"./lib/dns/include/dns/rdatasetiter.h\", line 109, column 32, charpos = 3103\n around = '(',\n whole content = #define DNS_RDATASETITER_FOREACH(rds) \\\nFile \"/tmp/cocci-output-110376-c54da3-rdatasetiter.h\", line 109, column 32, charpos = 3103\n around = '(',\n whole content = #define DNS_RDATASETITER_FOREACH(rds) \\\n")
Impossible: How can diff be null and have not Correct in compare_c? Tag1 ("diff token: ( VS (\nFile \"./lib/dns/include/dns/dbiterator.h\", line 114, column 30, charpos = 3413\n around = '(',\n whole content = #define DNS_DBITERATOR_FOREACH(rds) \\\nFile \"/tmp/cocci-output-110387-883f2f-dbiterator.h\", line 114, column 30, charpos = 3413\n around = '(',\n whole content = #define DNS_DBITERATOR_FOREACH(rds) \\\n")
See https://github.com/coccinelle/coccinelle/issues/398.
add a Coccinelle patch to ensure the pointer being used by
isc_mem_free() and isc_mem_put() is not explicitly set to NULL (those
mecros are taking care of it).
coccinelle v1.1 reports the following failure:
EXN: Failure("./lib/dns/sdlz.c: 172: try to delete an expanded token: unsigned") in ./lib/dns/sdlz.c
coccinelle v1.2 reports the following failure:
EXN: Failure("./util/models.c: 21: try to delete an expanded token: unsigned") in ./util/models.c
Instead of creating new memory pools for each new dns_message, change
dns_message_create() method to optionally accept externally created
dns_fixedname_t and dns_rdataset_t memory pools. This allows us to
preallocate the memory pools in ns_client and dns_resolver units for the
lifetime of dns_resolver_t and ns_clientmgr_t.
Add a semantic patch to catch all the places where we pass 'char' to the
<ctype.h> family of functions (isalpha() and friends, toupper(),
tolower()). While it generally works because the way how these
functions are constructed in the libc, it's safer to do the explicit
cast.
Use the new isc_mem_c*() calloc-like API for allocations that are
zeroed.
In turn, this also fixes couple of incorrect usage of the ISC_MEM_ZERO
for structures that need to be zeroed explicitly.
There are few places where isc_mem_cput() is used on structures with a
flexible member (or similar).
The aim is to match unsafe patterns of allocation size arithmetic
and turn them into safe calls to the new `isc_mem_cget()`,
`isc_mem_creget()`, and `isc_mem_cput()`.
The SET_IF_NOT_NULL() macro avoids a fair amount of tedious boilerplate,
checking pointer parameters to see if they're non-NULL and updating
them if they are. The macro was already in the dns_zone unit, and this
commit moves it to the <isc/util.h> header.
I have included a Coccinelle semantic patch to use SET_IF_NOT_NULL()
where appropriate. The patch needs an #include in `openssl_shim.c`
in order to work.
The isc_time_now() and isc_time_now_hires() were used inconsistently
through the code - either with status check, or without status check,
or via TIME_NOW() macro with RUNTIME_CHECK() on failure.
Refactor the isc_time_now() and isc_time_now_hires() to always fail when
getting current time has failed, and return the isc_time_t value as
return value instead of passing the pointer to result in the argument.
removed references in code comments, doc/dev documentation, etc, to
isc_task, isc_timer_reset(), and isc_timertype_inactive. also removed a
coccinelle patch related to isc_timer_reset() that was no longer needed.
Add new semantic patch to replace the straightfoward uses of:
ptr = isc_mem_{get,allocate}(..., size);
memset(ptr, 0, size);
with the new API call:
ptr = isc_mem_{get,allocate}x(..., size, ISC_MEM_ZERO);
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.
In couple places, we have missed INSIST(0) or ISC_UNREACHABLE()
replacement on some branches with UNREACHABLE(). Replace all
ISC_UNREACHABLE() or INSIST(0) calls with UNREACHABLE().
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.
The UV_RUNTIME_CHECK() macro requires to keep the function name in sync
like this:
r = func(...);
UV_RUNTIME_CHECK(func, r);
Add semantic patch to keep the function name and return variable in sync
with the previous line.
A typo introduced in f3f1cab05e prevents
execution of the dns_name_copy-with-result.spatch. The replacement
should end with semicolon not a colon:
plus: parse error:
File "cocci/dns_name_copy-with-result.spatch", line 28, column 23, charpos = 421
around = ':',
whole content = + dns_name_copy(E1, E2):
The __builtin_expect() can be used to provide the compiler with branch
prediction information. The Gcc manual says[1] on the subject:
In general, you should prefer to use actual profile feedback for
this (-fprofile-arcs), as programmers are notoriously bad at
predicting how their programs actually perform.
Stop using __builtin_expect() and ISC_LIKELY() and ISC_UNLIKELY() macros
to provide the branch prediction information as the performance testing
shows that named performs better when the __builtin_expect() is not
being used.
1. https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html#index-_005f_005fbuiltin_005fexpect
- isc_mempool_get() can no longer fail; when there are no more objects
in the pool, more are always allocated. checking for NULL return is
no longer necessary.
- the isc_mempool_setmaxalloc() and isc_mempool_getmaxalloc() functions
are no longer used and have been removed.
The dns_message_create() function cannot soft fail (as all memory
allocations either succeed or cause abort), so we change the function to
return void and cleanup the calls.
dns_message_t objects are now being handled using reference counting
semantics, so now dns_message_destroy() is not called directly anymore,
dns_message_detach must be called instead.
Also disable the semantic patch as the code needs tweaks here and there because
some destroy functions might not destroy the object and return early if the
object is still in use.
Our destroy functions usually look like this:
void
foo_destroy(foo_t **foop) {
foo_t foo = *foop;
...destroy the contents of foo...
*foop = NULL;
}
nulling the pointer should be done as soon as possible which is
not always the case. This commit adds simple semantic patch that
changes the example function to:
void
foo_destroy(foo_t **foop) {
foo_t foo = *foop;
*foop = NULL;
...destroy the contents of foo...
}
When a function returns void, it can be used as an argument to return in
function returning also void, e.g.:
void in(void) {
return;
}
void out(void) {
return (in());
}
while this is legal, it should be rewritten as:
void out(void) {
in();
return;
}
The semantic patch just find the occurrences, and they need to be fixed
by hand.
Some semantic patches are meant to be run just once, as they work on
functions with changed prototypes. We keep them for reference, but
disabled them from the CI to save time.
The dns_name_copy() function cannot fail gracefully when the last argument
(target) is NULL. Add RUNTIME_CHECK()s around such calls.
The first semantic patch adds RUNTIME_CHECK() around any call that ignores the
return value and is very safe to apply.
The second semantic patch attempts to properly add RUNTIME_CHECK() to places
where the return value from `dns_name_copy()` is recorded into `result`
variable. The result of this semantic patch needs to be reviewed by hand.
Both patches misses couple places where the code surrounding the
`dns_name_copy(..., NULL)` usage is more complicated and is better suited to be
fixed by a human being that understands the surrounding code.
The isc_mem_get() cannot fail gracefully now, it either gets memory of
assert()s. The added semantic patch cleans all the blocks checking whether
the return value of isc_mem_get() was NULL.
