GitLab

The dns_cache_dump() public function was used only internally when destroying
the cache and it checked the validity of the cache object when in fact the cache
object was no longer valid (.references == 0).  This commit changes the
dns_cache_dump() function into static cache_dump() function without the validity
check.

Use isc_refcount_destroy() instead of (void)isc_refcount_current() for memory synchronization where appropriate

Rename view->references to view->ereferences and view->weakrefs to view->references, so we can use ISC_OBJECT_VALID macro

The dns_zone_setmasterswithkeys() had two purposes - to set the list of masters
and to clear them when the argument was empty.  The commit splits the public
function into two public functions dns_zone_setmasterswithkeys() and
dns_zone_clearmasters() and one private function clear_masters().  The
clear_masters() is called internally from the destroy function and thus it
doesn't require valid zone object.

The same applies to dns_zone_setalsonotifydscpkeys() - it was split into
dns_zone_setalsonotifydscpkeys(), dns_zone_clearalsonotify() and private
clear_alsonotify().

The destroy() function for resolver object called public functions that in turn
required valid resolver object magic.  This commit refactors the destroy()
function to call internal destroy_<foo>() functions that don't require valid
resolver object (because it's no longer valid), and the public functions are
just thin wrappers around the internal destroy_<foo>() functions with optional
lock and check for valid resolver object.

dns_view_setnewzones(dns_view_t *, bool allow, ...) into two separate functions.

1. dns_view_allownewzones(dns_view_t *, ...) which internally calls
   clear_newzones() first and then sets the internal state of the view to allow
   adding and deleting new zones at the runtime

2. dns_view_disallownewzones(dns_view_t *) which internally calls
   clear_newzones() that disallows adding and deleting new zones at the runtime

As added bonus, the destroy() method now just calls clear_newzones() internally.

This commit replaces usage of ISC_MAGIC_VALID with ISC_OBJECT_VALID where the
underlying object uses reference counting.  Sometimes that involved renaming the
struct member from .ref or .refcount to .references, with one notable exception
in lib/dns/zone.c where .eref got renamed to .references.  There's also .iref,
but that's not the external reference counting.

The new macro is a superset of ISC_MAGIC_VALID(obj, magic) as it checks whether
the .references member of obj is greater or equal to 1 in addition to checking
the valid magic.  This allows early race detection between the "destroy" which
is usually wrapped in:

    #define OBJ_MAGIC ISC_MAGIC('O', 'b', 'j', 'x');
    #define VALID_OBJ ISC_MAGIC_VALID(obj, OBJ_MAGIC);

    void
    obj_destroy(isc_obj_t *obj) {
    	VALID_OBJ(obj);
    	/* T1:1 */
    	if (isc_refcount_decrement(&obj->references) == 1) {
    		isc_refcount_destroy(&obj->references);
    		/* T1: 2 */
    		obj->magic = 0;
    		/* T1: 3 */
    	}
    }

and common API function that usually have something like this:

    #define OBJ_MAGIC ISC_MAGIC('O', 'b', 'j', 'x');
    #define VALID_OBJ ISC_MAGIC_VALID(obj, OBJ_MAGIC);

    isc_result_t
    obj_foo(isc_obj_t *obj) {
    	/* T2: 1 */
    	VALID_OBJ(obj);
    	/* T2: 2 */
    	/* access members of the object */
    	return (ISC_R_SUCCESS);
    }

There's a race between the threads when the execution order is:

    1. T1: 1 /* obj is valid */
    2. T1: 2 /* obj is no longer valid, but it's still magic */
    3. T2: 1 /* obj is still magic */
    4. T1: 3 /* obj is no longer magic */
    5. T2: 2 /* Ka-Boom */

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));

Refactor lib/dns/keytable.c to reuse dns_keytable_attachkeynode() and use isc_refcount_increment0() for .active_nodes

The isc_refcount API that provides reference counting lost DbC checks for
overflows and underflows in the isc_refcount_{increment,decrement} functions.

The commit restores the overflow check in the isc_refcount_increment and
underflows check in the isc_refcount_decrement by checking for the previous
value to not be on the boundary.

remove unused variable

See merge request !2418

missing RUNTIME_CHECK

See merge request !2265

Use the semantic patch from the previous commit to replace all the calls to
dns_name_copy() with NULL as third argument with dns_name_copynf().

The dns_name_copy() function followed two different semanitcs that was driven
whether the last argument was or wasn't NULL.  This commit splits the function
in two where now third argument to dns_name_copy() can't be NULL and
dns_name_copynf() doesn't have third argument.

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.

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.

Resolve "dig cannot display ACE query if locale is not unicode"

Closes #846

See merge request !1418