From: Arran Cudbard-Bell Date: Thu, 26 Feb 2026 00:51:36 +0000 (-0700) Subject: Assert if slab elements are not released correctly X-Git-Url: http://git.ipfire.org/gitweb/?a=commitdiff_plain;h=331aa05;p=thirdparty%2Ffreeradius-server.git Assert if slab elements are not released correctly --- diff --git a/src/lib/util/slab.h b/src/lib/util/slab.h index d2320874ef4..0f2c2eb0f32 100644 --- a/src/lib/util/slab.h +++ b/src/lib/util/slab.h @@ -47,6 +47,9 @@ typedef struct { \ ///< number in use has reached max_elements. unsigned int num_children; //!< How many child allocations are expected off each element. size_t child_pool_size; //!< Size of pool space to be allocated to each element. + bool allow_direct_free; //!< Allow in-use elements to be freed with talloc_free. + ///< Normally this is a programming error and triggers an + ///< assertion. Only set for testing the destructor path. fr_time_delta_t interval; //!< Interval between slab cleanup events being fired. } fr_slab_config_t; @@ -96,6 +99,7 @@ typedef struct { \ FR_DLIST_ENTRY(_name ## _slab) entry; \ _name ## _slab_list_t *list; \ TALLOC_CTX *pool; \ + bool being_freed; \ FR_DLIST_HEAD(_name ## _slab_element) reserved; \ FR_DLIST_HEAD(_name ## _slab_element) avail; \ } _name ## _slab_t; \ @@ -203,12 +207,31 @@ DIAG_OFF(unused-function) \ } \ return slab; \ } \ +\ + /** Callback for talloc freeing a slab \ + * \ + * Sets the being_freed flag so that element destructors know this \ + * is a bulk teardown and not an erroneous direct free. \ + * talloc calls this destructor before freeing children. \ + */ \ + static int _ ## _name ## _slab_free(_name ## _slab_t *slab) \ + { \ + slab->being_freed = true; \ + return 0; \ + } \ \ /** Callback for talloc freeing a slab element \ * \ - * Ensure that the element reset and custom destructor is called even if \ - * the element is not released before being freed. \ - * Also ensure the element is removed from the relevant list. \ + * Called during bulk teardown (talloc_free of a slab or slab list). \ + * Ensures the element is cleanly removed from whichever dlist it is on. \ + * \ + * In-use slab elements must NEVER be talloc_free'd directly. Use \ + * the type-specific _slab_release() function instead. Calling \ + * talloc_free permanently frees the element's pool memory, + * progressively degrading slab efficiency. The talloc pool cannot \ + * be fully reclaimed until ALL children are freed, so piecemeal frees \ + * fragment the pool and shrink the usable element count on each reallocation + * cycle. \ */ \ static int _ ## _type ## _element_free(_name ## _slab_element_t *element) \ { \ @@ -217,6 +240,10 @@ DIAG_OFF(unused-function) \ if (!element->slab) return 0; \ slab = element->slab; \ if (element->in_use) { \ + fr_assert_msg(slab->being_freed || slab->list->config.allow_direct_free, \ + "in-use slab element freed with talloc_free - use _slab_release()"); \ + slab->list->in_use--; \ + element->in_use = false; \ _name ## _slab_element_remove(&slab->reserved, element); \ } else { \ _name ## _slab_element_remove(&slab->avail, element); \ @@ -249,6 +276,7 @@ DIAG_OFF(unused-function) \ elem_size = slab_list->config.elements_per_slab * (sizeof(_name ## _slab_element_t) + \ slab_list->config.child_pool_size); \ MEM(slab = talloc_zero_pooled_object(slab_list, _name ## _slab_t, elems, elem_size)); \ + talloc_set_destructor(slab, _ ## _name ## _slab_free); \ _name ## _slab_element_init(&slab->avail); \ _name ## _slab_element_init(&slab->reserved); \ _name ## _slab_insert_head(&slab_list->avail, slab); \ diff --git a/src/lib/util/test/slab_tests.c b/src/lib/util/test/slab_tests.c index cc563a28735..760e00fe417 100644 --- a/src/lib/util/test/slab_tests.c +++ b/src/lib/util/test/slab_tests.c @@ -317,15 +317,17 @@ static void test_reserve_mru(void) } /** Test that talloc freeing an element results in destructor being called - * + * and that accounting is correctly updated. */ static void test_free(void) { test_slab_list_t *test_slab_list; test_element_t *test_element; test_uctx_t test_uctx; + fr_slab_config_t slab_config = def_slab_config; - test_slab_list = test_slab_list_alloc(NULL, NULL, &def_slab_config, NULL, NULL, NULL, true, false); + slab_config.allow_direct_free = true; + test_slab_list = test_slab_list_alloc(NULL, NULL, &slab_config, NULL, NULL, NULL, true, false); TEST_CHECK(test_slab_list != NULL); if (!test_slab_list) return; @@ -337,8 +339,11 @@ static void test_free(void) if (test_element) test_element->name = talloc_strdup(test_element, "Hello there"); if (test_element) test_slab_element_set_destructor(test_element, test_element_free, &test_uctx); + TEST_CHECK_RET(test_slab_num_elements_used(test_slab_list), 1); + if (test_element) talloc_free(test_element); TEST_CHECK(test_uctx.count == 11); + TEST_CHECK_RET(test_slab_num_elements_used(test_slab_list), 0); talloc_free(test_slab_list); } diff --git a/src/modules/rlm_cache/drivers/rlm_cache_memcached/rlm_cache_memcached.c b/src/modules/rlm_cache/drivers/rlm_cache_memcached/rlm_cache_memcached.c index 52014aa5d13..63bd5154f7d 100644 --- a/src/modules/rlm_cache/drivers/rlm_cache_memcached/rlm_cache_memcached.c +++ b/src/modules/rlm_cache/drivers/rlm_cache_memcached/rlm_cache_memcached.c @@ -312,7 +312,7 @@ static int mod_conn_reconnect(void **handle, UNUSED rlm_cache_config_t const *co rlm_cache_memcached_thread_t *t = talloc_get_type_abort(module_thread(driver->mi)->data, rlm_cache_memcached_thread_t); rlm_cache_handle_t *mandle; - talloc_free(*handle); + memcached_slab_release(*handle); mandle = memcached_slab_reserve(t->slab); if (!mandle) { *handle = NULL;