diff --git a/CHANGES b/CHANGES index c83d2c15dd..71704f18b0 100644 --- a/CHANGES +++ b/CHANGES @@ -1,3 +1,5 @@ +6390. [bug] Fix a data race in isc_task_purgeevent(). [GL !8937] + 6389. [bug] dnssec-verify and dnssec-signzone could fail if there was an obscured DNSKEY RRset at a delegatation. [GL #4517] diff --git a/lib/isc/include/isc/timer.h b/lib/isc/include/isc/timer.h index 815cf42dbe..77b7507031 100644 --- a/lib/isc/include/isc/timer.h +++ b/lib/isc/include/isc/timer.h @@ -224,6 +224,21 @@ isc_timer_touch(isc_timer_t *timer); *\li Unexpected error */ +void +isc_timer_purge(isc_timer_t *timer); +/*%< + * Purge timer. + * + * Requires: + * + *\li 'timer' points to a valid timer. + * + * Ensures: + * + *\li Any events already posted by the timer are purged. + * + */ + void isc_timer_destroy(isc_timer_t **timerp); /*%< diff --git a/lib/isc/task.c b/lib/isc/task.c index 439d430790..48c3e790f4 100644 --- a/lib/isc/task.c +++ b/lib/isc/task.c @@ -612,6 +612,15 @@ isc_task_purge(isc_task_t *task, void *sender, isc_eventtype_t type, return (isc_task_purgerange(task, sender, type, type, tag)); } +/* + * The caller is responsible for freeing the event if this function returns + * true. If it returns false, then the event was already processed before it + * could be purged, so the event's action is responsible for freeing the event. + * The caller however must make sure that the event's destroy function, called + * when the event's action calls isc_event_destroy(), doesn't free the event + * while this function is still running. That is, 'event' must remain valid + * throughout the whole execution of this function. + */ bool isc_task_purgeevent(isc_task_t *task, isc_event_t *event) { bool found = false; @@ -625,9 +634,7 @@ isc_task_purgeevent(isc_task_t *task, isc_event_t *event) { /* * If 'event' is on the task's event queue, it will be purged, * unless it is marked as unpurgeable. 'event' does not have to be - * on the task's event queue; in fact, it can even be an invalid - * pointer. Purging only occurs if the event is actually on the task's - * event queue. + * on the task's event queue. * * Purging never changes the state of the task. */ @@ -644,8 +651,6 @@ isc_task_purgeevent(isc_task_t *task, isc_event_t *event) { return (false); } - isc_event_free(&event); - return (true); } diff --git a/lib/isc/timer.c b/lib/isc/timer.c index 40ae929ce8..5d3d3b627a 100644 --- a/lib/isc/timer.c +++ b/lib/isc/timer.c @@ -231,8 +231,23 @@ timer_purge(isc_timer_t *timer) { while ((event = ISC_LIST_HEAD(timer->active)) != NULL) { timerevent_unlink(timer, event); + bool purged = isc_task_purgeevent(timer->task, + (isc_event_t *)event); UNLOCK(&timer->lock); - (void)isc_task_purgeevent(timer->task, (isc_event_t *)event); +#if defined(UNIT_TESTING) + usleep(100); +#endif + if (purged) { + isc_event_free((isc_event_t **)&event); + } else { + /* + * The event was processed while we were trying to + * purge it. The event's action is responsible for + * calling isc_event_free(), which in turn will call + * event->ev_destroy() (timerevent_destroy() here), + * which will unlink and destroy it. + */ + } LOCK(&timer->lock); } } @@ -469,6 +484,15 @@ isc_timer_touch(isc_timer_t *timer) { return (result); } +void +isc_timer_purge(isc_timer_t *timer) { + REQUIRE(VALID_TIMER(timer)); + + LOCK(&timer->lock); + timer_purge(timer); + UNLOCK(&timer->lock); +} + void isc_timer_destroy(isc_timer_t **timerp) { isc_timer_t *timer = NULL; diff --git a/tests/isc/task_test.c b/tests/isc/task_test.c index 973ecc9a99..d718adcff2 100644 --- a/tests/isc/task_test.c +++ b/tests/isc/task_test.c @@ -38,6 +38,7 @@ #include #include "netmgr/uv-compat.h" +#include "timer.c" #include @@ -400,8 +401,18 @@ ISC_RUN_TEST_IMPL(privilege_drop) { } /* - * Basic task functions: + * Basic task variables and functions: */ + +static char one[] = "1"; +static char two[] = "2"; +static char three[] = "3"; +static char four[] = "4"; +static char tick[] = "tick"; +static char tock[] = "tock"; +static char quick[] = "quick"; +static isc_timer_t *ti3 = NULL; + static void basic_cb(isc_task_t *task, isc_event_t *event) { int i, j; @@ -434,7 +445,27 @@ basic_shutdown(isc_task_t *task, isc_event_t *event) { } static void -basic_tick(isc_task_t *task, isc_event_t *event) { +basic_tick1(isc_task_t *task, isc_event_t *event) { + UNUSED(task); + + if (verbose) { + print_message("# %s\n", (char *)event->ev_arg); + } + + /* Test for a race condition with isc_event_free() in basic_quick(). */ + if (!atomic_load(&done)) { + LOCK(&lock); + if (ti3 != NULL) { + isc_timer_purge(ti3); + } + UNLOCK(&lock); + } + + isc_event_free(&event); +} + +static void +basic_tick2(isc_task_t *task, isc_event_t *event) { UNUSED(task); if (verbose) { @@ -444,12 +475,12 @@ basic_tick(isc_task_t *task, isc_event_t *event) { isc_event_free(&event); } -static char one[] = "1"; -static char two[] = "2"; -static char three[] = "3"; -static char four[] = "4"; -static char tick[] = "tick"; -static char tock[] = "tock"; +static void +basic_quick(isc_task_t *task, isc_event_t *event) { + UNUSED(task); + + isc_event_free(&event); +} ISC_RUN_TEST_IMPL(basic) { isc_result_t result; @@ -466,15 +497,22 @@ ISC_RUN_TEST_IMPL(basic) { one, two, three, four, two, three, four, NULL }; int i; + atomic_init(&done, false); + UNUSED(state); - result = isc_task_create(taskmgr, 0, &task1); + /* + * Note: running task1 and task4 on different threads, because they + * test a race condition. + */ + + result = isc_task_create_bound(taskmgr, 0, &task1, 0); assert_int_equal(result, ISC_R_SUCCESS); result = isc_task_create(taskmgr, 0, &task2); assert_int_equal(result, ISC_R_SUCCESS); result = isc_task_create(taskmgr, 0, &task3); assert_int_equal(result, ISC_R_SUCCESS); - result = isc_task_create(taskmgr, 0, &task4); + result = isc_task_create_bound(taskmgr, 0, &task4, 1); assert_int_equal(result, ISC_R_SUCCESS); result = isc_task_onshutdown(task1, basic_shutdown, one); @@ -489,14 +527,21 @@ ISC_RUN_TEST_IMPL(basic) { isc_time_settoepoch(&absolute); isc_interval_set(&interval, 1, 0); result = isc_timer_create(timermgr, isc_timertype_ticker, &absolute, - &interval, task1, basic_tick, tick, &ti1); + &interval, task1, basic_tick1, tick, &ti1); assert_int_equal(result, ISC_R_SUCCESS); ti2 = NULL; isc_time_settoepoch(&absolute); isc_interval_set(&interval, 1, 0); result = isc_timer_create(timermgr, isc_timertype_ticker, &absolute, - &interval, task2, basic_tick, tock, &ti2); + &interval, task2, basic_tick2, tock, &ti2); + assert_int_equal(result, ISC_R_SUCCESS); + + ti3 = NULL; + isc_time_settoepoch(&absolute); + isc_interval_set(&interval, 0, 1000); + result = isc_timer_create(timermgr, isc_timertype_ticker, &absolute, + &interval, task4, basic_quick, quick, &ti3); assert_int_equal(result, ISC_R_SUCCESS); sleep(2); @@ -524,9 +569,14 @@ ISC_RUN_TEST_IMPL(basic) { isc_task_detach(&task3); isc_task_detach(&task4); + atomic_store(&done, true); + sleep(10); isc_timer_destroy(&ti1); isc_timer_destroy(&ti2); + LOCK(&lock); + isc_timer_destroy(&ti3); + UNLOCK(&lock); } /* @@ -1348,6 +1398,9 @@ try_purgeevent(bool purgeable) { purged = isc_task_purgeevent(task, event2_clone); assert_int_equal(purgeable, purged); + if (purged) { + isc_event_free(&event2_clone); + } /* * Unblock the task, allowing event processing.