To reproduce the race - create a task, send two events to it, first one
must take some time. Then, from the outside, pause(), unpause() and detach()
the task.
When the long-running event is processed by the task it is in
task_state_running state. When we called pause() the state changed to
task_state_paused, on unpause we checked that there are events in the task
queue, changed the state to task_state_ready and enqueued the task on the
workers readyq. We then detach the task.
The dispatch() is done with processing the event, it processes the second
event in the queue, and then shuts down the task and frees it (as it's not
referenced anymore). Dispatcher then takes the, already freed, task from
the queue where it was wrongly put, causing an use-after free and,
subsequently, either an assertion failure or a segmentation fault.
The probability of this happening is very slim, yet it might happen under a
very high load, more probably on a recursive resolver than on an
authoritative.
The fix introduces a new 'task_state_pausing' state - to which tasks
are moved if they're being paused while still running. They are moved
to task_state_paused state when dispatcher is done with them, and
if we unpause a task in paused state it's moved back to task_state_running
and not requeued.
This is a bug I encountered when trying to schedule an algorithm
rollover. My plan, for a zone whose maximum TTL is 48h, was to sign
with the new algorithm and schedule a change of CDS records for more
than 48 hours in the future, roughly like this:
$ dnssec-keygen -a 13 -fk -Psync now+50h $zone
$ dnssec-keygen -a 13 $zone
$ dnssec-settime -Dsync now+50h $zone_ksk_old
However the algorithm 13 CDS was published immediately, which could
have made the zone bogus.
To reveal the bug using the `smartsign` test, this change just adds a
KSK with all its times in the future, so it should not affect the
existing checks at all. But the final check (that there are no CDS or
CDSNSKEY records after -Dsync) fails with the old `syncpublish()`
logic, because the future key's sync records appear early. With the
new `syncpublish()` logic the future key does not affect the test, as
expected, and it now passes.
When two threads unreferenced handles coming from one socket while
the socket was being destructed we could get a use-after-free:
Having handle H1 coming from socket S1, H2 coming from socket S2,
S0 being a parent socket to S1 and S2:
Thread A Thread B
Unref handle H1 Unref handle H2
Remove H1 from S1 active handles Remove H2 from S2 active handles
nmsocket_maybe_destroy(S1) nmsocket_maybe_destroy(S2)
nmsocket_maybe_destroy(S0) nmsocket_maybe_destroy(S0)
LOCK(S0->lock)
Go through all children, figure
out that we have no more active
handles:
sum of S0->children[i]->ah == 0
UNLOCK(S0->lock)
destroy(S0)
LOCK(S0->lock)
- but S0 is already gone
We weren't consistent about who should unreference the handle in
case of network error. Make it consistent so that it's always the
client code responsibility to unreference the handle - either
in the callback or right away if send function failed and the callback
will never be called.
If taskmgr is shutting down ns_client_setup will fail to create
a task for the newly created client, we weren't cleaning up already
created/attached things (memory context, server, clientmgr).
In tcp and udp stoplistening code we accessed libuv structures
from a different thread, which caused a shutdown crash when named
was under load. Also added additional DbC checks making sure we're
in a proper thread when accessing uv_ functions.
We had a race in which n UDP socket could have been already closing
by libuv but we still sent data to it. Mark socket as not-active
when stopping listening and verify that socket is not active when
trying to send data to it.
if validator_start() is called with validator->event->message set to
NULL, we can't use message->rcode to decide which negative proofs are
needed, so we use the rdataset attributes instead to determine whether
the rdataset was cached as NXDOMAIN or NODATA.
We pass interface as an opaque argument to tcpdns listening socket.
If we stop listening on an interface but still have in-flight connections
the opaque 'interface' is not properly reference counted, and we might
hit a dead memory. We put just a single source of truth in a listening
socket and make the child sockets use that instead of copying the
value from listening socket. We clean the callback when we stop listening.
- isc__netievent_storage_t was to small to contain
isc__netievent__socket_streaminfo_t on Windows
- handle isc_uv_export and isc_uv_import errors properly
- rewrite isc_uv_export and isc_uv_import on Windows
hp implementation requires an object for each thread accessing
a hazard pointer. previous implementation had a hardcoded
HP_MAX_THREAD value of 128, which failed on machines with lots of
CPU cores (named uses 3n threads). We make isc__hp_max_threads
configurable at startup, with the value set to 4*named_g_cpus.
It's also important for this value not to be too big as we do
linear searches on a list.
it now removes matching trust anchors from from the dslist while leaving
the other trust anchors in place.
also cleaned up the API to remove functions that were never being used.
NOTE: the keytable test is still failing because dns_keytable_deletekey()
is looking for exact matches in keynodes containing dst_key objects,
which no keynode has anymore.
the internal keytable structure has not yet been changed, but
insertion of DS anchors is the only method now available.
NOTE: the keytable unit test is currently failing because of tests
that expect individual keynode objects to contain single DST key
objects.
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.
this function is used by dns_view_untrust() to handle revoked keys, so
it will still be needed after the keytable/validator refactoring is
complete, even though the keytable will be storing DS trust anchors
instead of keys. to simplify the way it's called, it now takes a DNSKEY
rdata struct instead of a DST key.
