From 4ac3a6520e1c8b5a19321a87466269235ff406c6 Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Tue, 17 Mar 2026 11:22:04 +0000 Subject: [PATCH 1/2] Convert dns_dtenv_t reference counting to standard macors Use standard reference counting macros for dns_dtenv_t instead of custom attach/detach functions. --- bin/named/server.c | 4 ++-- lib/dns/dnstap.c | 34 +++++++++------------------ lib/dns/include/dns/dnstap.h | 45 ++++++++++++------------------------ lib/dns/view.c | 2 +- tests/dns/dnstap_test.c | 12 +++++----- 5 files changed, 35 insertions(+), 62 deletions(-) diff --git a/bin/named/server.c b/bin/named/server.c index 0408cfc92f..78176773b2 100644 --- a/bin/named/server.c +++ b/bin/named/server.c @@ -3507,7 +3507,7 @@ configure_dnstap(const cfg_obj_t **maps, dns_view_t *view) { cfg_obj_asstring(obj)); } - dns_dt_attach(named_g_server->dtenv, &view->dtenv); + dns_dtenv_attach(named_g_server->dtenv, &view->dtenv); view->dttypes = dttypes; result = ISC_R_SUCCESS; @@ -9461,7 +9461,7 @@ named_server_destroy(named_server_t **serverp) { #ifdef HAVE_DNSTAP if (server->dtenv != NULL) { - dns_dt_detach(&server->dtenv); + dns_dtenv_detach(&server->dtenv); } #endif /* HAVE_DNSTAP */ diff --git a/lib/dns/dnstap.c b/lib/dns/dnstap.c index 80290c54ad..00f073d0a9 100644 --- a/lib/dns/dnstap.c +++ b/lib/dns/dnstap.c @@ -99,7 +99,7 @@ struct dns_dthandle { struct dns_dtenv { unsigned int magic; - isc_refcount_t refcount; + isc_refcount_t references; isc_mem_t *mctx; isc_loop_t *loop; @@ -125,10 +125,19 @@ typedef struct ioq { struct fstrm_iothr_queue *ioq; } dt__ioq_t; +static void +destroy(dns_dtenv_t *env); + static thread_local dt__ioq_t dt_ioq = { 0 }; static atomic_uint_fast32_t global_generation; +#if DNS_DTENV_TRACE +ISC_REFCOUNT_TRACE_IMPL(dns_dtenv, destroy); +#else +ISC_REFCOUNT_IMPL(dns_dtenv, destroy); +#endif /* DNS_DTENV_TRACE */ + isc_result_t dns_dt_create(isc_mem_t *mctx, dns_dtmode_t mode, const char *path, struct fstrm_iothr_options **foptp, isc_loop_t *loop, @@ -159,7 +168,7 @@ dns_dt_create(isc_mem_t *mctx, dns_dtmode_t mode, const char *path, isc_mem_attach(mctx, &env->mctx); isc_mutex_init(&env->reopen_lock); env->path = isc_mem_strdup(env->mctx, path); - isc_refcount_init(&env->refcount, 1); + isc_refcount_init(&env->references, 1); isc_stats_create(env->mctx, &env->stats, dns_dnstapcounter_max); fwopt = fstrm_writer_options_init(); @@ -441,15 +450,6 @@ dt_queue(dns_dtenv_t *env) { return dt_ioq.ioq; } -void -dns_dt_attach(dns_dtenv_t *source, dns_dtenv_t **destp) { - REQUIRE(VALID_DTENV(source)); - REQUIRE(destp != NULL && *destp == NULL); - - isc_refcount_increment(&source->refcount); - *destp = source; -} - isc_result_t dns_dt_getstats(dns_dtenv_t *env, isc_stats_t **statsp) { REQUIRE(VALID_DTENV(env)); @@ -495,18 +495,6 @@ destroy(dns_dtenv_t *env) { isc_mem_putanddetach(&env->mctx, env, sizeof(*env)); } -void -dns_dt_detach(dns_dtenv_t **envp) { - REQUIRE(envp != NULL && VALID_DTENV(*envp)); - dns_dtenv_t *env = *envp; - *envp = NULL; - - if (isc_refcount_decrement(&env->refcount) == 1) { - isc_refcount_destroy(&env->refcount); - destroy(env); - } -} - static isc_result_t pack_dt(const Dnstap__Dnstap *d, void **buf, size_t *sz) { ProtobufCBufferSimple sbuf; diff --git a/lib/dns/include/dns/dnstap.h b/lib/dns/include/dns/dnstap.h index 3f83e96d22..191d964158 100644 --- a/lib/dns/include/dns/dnstap.h +++ b/lib/dns/include/dns/dnstap.h @@ -118,6 +118,21 @@ struct dns_dtdata { }; #endif /* HAVE_DNSTAP */ +#if DNS_DTENV_TRACE +#define dns_dtenv_ref(ptr) dns_dtenv__ref(ptr, __func__, __FILE__, __LINE__) +#define dns_dtenv_unref(ptr) dns_dtenv__unref(ptr, __func__, __FILE__, __LINE__) +#define dns_dtenv_attach(ptr, ptrp) \ + dns_dtenv__attach(ptr, ptrp, __func__, __FILE__, __LINE__) +#define dns_dtenv_detach(ptrp) \ + dns_dtenv__detach(ptrp, __func__, __FILE__, __LINE__) +ISC_REFCOUNT_TRACE_DECL(dns_dtenv); +#else +ISC_REFCOUNT_DECL(dns_dtenv); +#endif /* DNS_DTENV_TRACE */ +/*% + * Reference counting for dns_dtenv + */ + isc_result_t dns_dt_create(isc_mem_t *mctx, dns_dtmode_t mode, const char *path, struct fstrm_iothr_options **foptp, isc_loop_t *loop, @@ -215,36 +230,6 @@ dns_dt_setversion(dns_dtenv_t *env, const char *version); *\li 'env' is a valid dnstap environment. */ -void -dns_dt_attach(dns_dtenv_t *source, dns_dtenv_t **destp); -/*%< - * Attach '*destp' to 'source', incrementing the reference counter. - * - * Requires: - * - *\li 'source' is a valid dnstap environment. - * - *\li 'destp' is not NULL and '*destp' is NULL. - * - *\li *destp is attached to source. - */ - -void -dns_dt_detach(dns_dtenv_t **envp); -/*%< - * Detach '*envp', decrementing the reference counter. - * - * Requires: - * - *\li '*envp' is a valid dnstap environment. - * - * Ensures: - * - *\li '*envp' will be destroyed when the number of references reaches zero. - * - *\li '*envp' is NULL. - */ - isc_result_t dns_dt_getstats(dns_dtenv_t *env, isc_stats_t **statsp); /*%< diff --git a/lib/dns/view.c b/lib/dns/view.c index 906c5bb549..d31d17161a 100644 --- a/lib/dns/view.c +++ b/lib/dns/view.c @@ -332,7 +332,7 @@ destroy(dns_view_t *view) { } #ifdef HAVE_DNSTAP if (view->dtenv != NULL) { - dns_dt_detach(&view->dtenv); + dns_dtenv_detach(&view->dtenv); } #endif /* HAVE_DNSTAP */ if (view->newzone.cleanup != NULL) { diff --git a/tests/dns/dnstap_test.c b/tests/dns/dnstap_test.c index e39725f55e..4a1c941bb0 100644 --- a/tests/dns/dnstap_test.c +++ b/tests/dns/dnstap_test.c @@ -92,7 +92,7 @@ ISC_LOOP_TEST_IMPL(dns_dt_create) { NULL, &dtenv); assert_int_equal(result, ISC_R_SUCCESS); if (dtenv != NULL) { - dns_dt_detach(&dtenv); + dns_dtenv_detach(&dtenv); } if (fopt != NULL) { fstrm_iothr_options_destroy(&fopt); @@ -108,7 +108,7 @@ ISC_LOOP_TEST_IMPL(dns_dt_create) { NULL, &dtenv); assert_int_equal(result, ISC_R_SUCCESS); if (dtenv != NULL) { - dns_dt_detach(&dtenv); + dns_dtenv_detach(&dtenv); } if (fopt != NULL) { fstrm_iothr_options_destroy(&fopt); @@ -125,7 +125,7 @@ ISC_LOOP_TEST_IMPL(dns_dt_create) { assert_int_equal(result, ISC_R_FAILURE); assert_null(dtenv); if (dtenv != NULL) { - dns_dt_detach(&dtenv); + dns_dtenv_detach(&dtenv); } if (fopt != NULL) { fstrm_iothr_options_destroy(&fopt); @@ -171,7 +171,7 @@ ISC_LOOP_TEST_IMPL(dns_dt_send) { NULL, &dtenv); assert_int_equal(result, ISC_R_SUCCESS); - dns_dt_attach(dtenv, &view->dtenv); + dns_dtenv_attach(dtenv, &view->dtenv); view->dttypes = DNS_DTTYPE_ALL; /* @@ -260,8 +260,8 @@ ISC_LOOP_TEST_IMPL(dns_dt_send) { m); } - dns_dt_detach(&view->dtenv); - dns_dt_detach(&dtenv); + dns_dtenv_detach(&view->dtenv); + dns_dtenv_detach(&dtenv); dns_view_detach(&view); result = dns_dt_open(TAPFILE, dns_dtmode_file, isc_g_mctx, &handle); From 48d7401f0db66cbe9f6fcdffb549488e28110ad8 Mon Sep 17 00:00:00 2001 From: Aram Sargsyan Date: Tue, 17 Mar 2026 11:23:22 +0000 Subject: [PATCH 2/2] Take 'env' reference before async calling perform_reopen() The 'env' pointer is passed to an async function without taking a reference first, which can potentially cause a use-after-free error. Take a reference, then detach in the async function. --- lib/dns/dnstap.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lib/dns/dnstap.c b/lib/dns/dnstap.c index 00f073d0a9..97b903d605 100644 --- a/lib/dns/dnstap.c +++ b/lib/dns/dnstap.c @@ -682,6 +682,8 @@ perform_reopen(void *arg) { LOCK(&env->reopen_lock); env->reopen_queued = false; UNLOCK(&env->reopen_lock); + + dns_dtenv_detach(&env); } /*% @@ -713,6 +715,7 @@ check_file_size_and_maybe_reopen(dns_dtenv_t *env) { * Send an event to roll the output file, then disallow output file * rolling until the roll we queue is completed. */ + dns_dtenv_ref(env); isc_async_run(env->loop, perform_reopen, env); env->reopen_queued = true;