Fix data race in async master dump/load context publication

Bouncing the offload itself to the target loop let the after-work
callback fire on the target thread and run the user's done callback
before the calling thread had published *dctxp / *lctxp.  Enqueue on
the calling loop and bounce only the done callback instead, so the
publish is sequenced before the cross-thread hand-off by construction
and cannot be reintroduced by reordering the entry-point body.

diff --git a/lib/dns/master.c b/lib/dns/master.c
index c1f5fa9a27940ed4dfd35b9d88344b903eb2005b..59841abf67dd879eee6e3db3ad617025b61f1e43 100644 (file)
--- a/lib/dns/master.c
+++ b/lib/dns/master.c
@@ -108,6 +108,8 @@ struct dns_loadctx {
        dns_loaddonefunc_t done;
        void *done_arg;
 
+       isc_loop_t *loop;
+
        /* Common methods */
        isc_result_t (*openfile)(dns_loadctx_t *lctx, const char *filename);
        isc_result_t (*load)(dns_loadctx_t *lctx);
@@ -2577,33 +2579,38 @@ cleanup:
        return result;
 }
 
+/*
+ * The combination of isc_work_enqueue() on the current loop and callback on
+ * lctx->loop ensures the correct ordering:
+ *
+ * 1. dns_master_loadfileasync() calls isc_work_enqueue() on the current loop.
+ * 2. master_load() runs asynchronously and can finish before the entry point
+ *    returns; master_load_done() is queued on the current loop and cannot run
+ *    until the entry point returns.
+ * 3. The entry point publishes *lctxp.
+ * 4. master_load_done() runs on the current loop and hands off to lctx->loop.
+ * 5. lctx->done() runs on lctx->loop asynchronously.
+ */
 static void
-load(void *arg) {
+master_load(void *arg) {
        dns_loadctx_t *lctx = arg;
        lctx->result = (lctx->load)(lctx);
 }
 
 static void
-load_done(void *arg) {
+master_load_callback(void *arg) {
        dns_loadctx_t *lctx = arg;
 
        (lctx->done)(lctx->done_arg, lctx->result);
+       isc_loop_detach(&lctx->loop);
        dns_loadctx_detach(&lctx);
 }
 
 static void
-load_enqueue(void *lctx) {
-       isc_work_enqueue(isc_loop(), load, load_done, lctx);
-}
+master_load_done(void *arg) {
+       dns_loadctx_t *lctx = arg;
 
-static void
-dns_loadctx_enqueue(isc_loop_t *loop, dns_loadctx_t *lctx) {
-       dns_loadctx_ref(lctx);
-       if (loop == isc_loop()) {
-               load_enqueue(lctx);
-       } else {
-               isc_async_run(loop, load_enqueue, lctx);
-       }
+       isc_async_run(lctx->loop, master_load_callback, lctx);
 }
 
 isc_result_t
@@ -2634,7 +2641,10 @@ dns_master_loadfileasync(const char *master_file, dns_name_t *top,
                return result;
        }
 
-       dns_loadctx_enqueue(loop, lctx);
+       dns_loadctx_ref(lctx);
+       isc_loop_attach(loop, &lctx->loop);
+       isc_work_enqueue(isc_loop(), master_load, master_load_done, lctx);
+
        *lctxp = lctx;
 
        return ISC_R_SUCCESS;

diff --git a/lib/dns/masterdump.c b/lib/dns/masterdump.c
index e73f5d1b00cb6647ce9a817e42238275dd07c5cd..4249e6d6f8633c694252c49ae8d88b7024a37fa9 100644 (file)
--- a/lib/dns/masterdump.c
+++ b/lib/dns/masterdump.c
@@ -234,6 +234,7 @@ static char tabs[N_TABS + 1] = "\t\t\t\t\t\t\t\t\t\t";
 struct dns_dumpctx {
        unsigned int magic;
        isc_mem_t *mctx;
+       isc_loop_t *loop;
        isc_mutex_t lock;
        isc_refcount_t references;
        atomic_bool canceled;
@@ -1450,10 +1451,20 @@ closeandrename(FILE *f, isc_result_t result, const char *temp,
 }
 
 /*
- * This will run in a libuv threadpool thread.
+ * The combination of isc_work_enqueue() on the current loop and callback on
+ * dctx->loop ensures the correct ordering:
+ *
+ * 1. dns_master_dumptostreamasync() (or dns_master_dumpasync()) calls
+ *    isc_work_enqueue() on the current loop.
+ * 2. master_dump() runs asynchronously and can finish before the entry point
+ *    returns; master_dump_done() is queued on the current loop and cannot run
+ *    until the entry point returns.
+ * 3. The entry point publishes *dctxp.
+ * 4. master_dump_done() runs on the current loop and hands off to dctx->loop.
+ * 5. dctx->done() runs on dctx->loop asynchronously.
  */
 static void
-master_dump_cb(void *data) {
+master_dump(void *data) {
        isc_result_t result = ISC_R_UNSET;
        dns_dumpctx_t *dctx = data;
        REQUIRE(DNS_DCTX_VALID(dctx));
@@ -1478,17 +1489,22 @@ master_dump_cb(void *data) {
        dctx->result = result;
 }
 
-/*
- * This will run in a loop manager thread when the dump is complete.
- */
 static void
-master_dump_done_cb(void *data) {
+master_dump_callback(void *data) {
        dns_dumpctx_t *dctx = data;
 
        (dctx->done)(dctx->done_arg, dctx->result);
+       isc_loop_detach(&dctx->loop);
        dns_dumpctx_detach(&dctx);
 }
 
+static void
+master_dump_done(void *data) {
+       dns_dumpctx_t *dctx = data;
+
+       isc_async_run(dctx->loop, master_dump_callback, dctx);
+}
+
 static isc_result_t
 dumpctx_create(isc_mem_t *mctx, dns_db_t *db, dns_dbversion_t *version,
               const dns_master_style_t *style, FILE *f, dns_dumpctx_t **dctxp,
@@ -1707,21 +1723,6 @@ cleanup:
        return result;
 }
 
-static void
-master_dump_enqueue(void *dctx) {
-       isc_work_enqueue(isc_loop(), master_dump_cb, master_dump_done_cb, dctx);
-}
-
-static void
-dns_dumpctx_enqueue(isc_loop_t *loop, dns_dumpctx_t *dctx) {
-       dns_dumpctx_ref(dctx);
-       if (loop == isc_loop()) {
-               master_dump_enqueue(dctx);
-       } else {
-               isc_async_run(loop, master_dump_enqueue, dctx);
-       }
-}
-
 isc_result_t
 dns_master_dumptostreamasync(isc_mem_t *mctx, dns_db_t *db,
                             dns_dbversion_t *version,
@@ -1739,7 +1740,10 @@ dns_master_dumptostreamasync(isc_mem_t *mctx, dns_db_t *db,
        dctx->done = done;
        dctx->done_arg = done_arg;
 
-       dns_dumpctx_enqueue(loop, dctx);
+       dns_dumpctx_ref(dctx);
+       isc_loop_attach(loop, &dctx->loop);
+       isc_work_enqueue(isc_loop(), master_dump, master_dump_done, dctx);
+
        *dctxp = dctx;
 
        return ISC_R_SUCCESS;
@@ -1828,7 +1832,10 @@ dns_master_dumpasync(isc_mem_t *mctx, dns_db_t *db, dns_dbversion_t *version,
        dctx->file = file;
        dctx->tmpfile = tempname;
 
-       dns_dumpctx_enqueue(loop, dctx);
+       dns_dumpctx_ref(dctx);
+       isc_loop_attach(loop, &dctx->loop);
+       isc_work_enqueue(isc_loop(), master_dump, master_dump_done, dctx);
+
        *dctxp = dctx;
 
        return ISC_R_SUCCESS;