From: Arran Cudbard-Bell Date: Sat, 9 Apr 2022 17:36:20 +0000 (-0500) Subject: Cleanup more atexit stuff, and explicitly trigger atexit handlers to avoid ordering... X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=a10cbf9b0ca6d74b8828d0ea875057788f952f13;p=thirdparty%2Ffreeradius-server.git Cleanup more atexit stuff, and explicitly trigger atexit handlers to avoid ordering issues with libraries which install their own atexit handlers --- diff --git a/src/bin/dhcpclient.c b/src/bin/dhcpclient.c index 304d5ea892..cea0126e28 100644 --- a/src/bin/dhcpclient.c +++ b/src/bin/dhcpclient.c @@ -750,5 +750,11 @@ int main(int argc, char **argv) fr_dhcpv4_global_free(); fr_dict_autofree(dhcpclient_dict); - return ret < 0 ? 1 : 0; + /* + * Ensure our atexit handlers run before any other + * atexit handlers registered by third party libraries. + */ + fr_atexit_global_trigger_all(); + + return ret < 0 ? EXIT_FAILURE : EXIT_SUCCESS; } diff --git a/src/bin/fuzzer.c b/src/bin/fuzzer.c index 7829c825d1..d9b65d5713 100644 --- a/src/bin/fuzzer.c +++ b/src/bin/fuzzer.c @@ -44,7 +44,6 @@ static dl_t *dl = NULL; static dl_loader_t *dl_loader; static fr_dict_t *dict = NULL; -static fr_dict_gctx_t const *dict_gctx = NULL; int LLVMFuzzerInitialize(int *argc, char ***argv); int LLVMFuzzerTestOneInput(const uint8_t *buf, size_t len); @@ -53,13 +52,17 @@ static void exitHandler(void) { fr_dict_free(&dict, __FILE__); - if (fr_dict_global_ctx_free(dict_gctx) < 0) fr_perror("fuzzer"); - if (dl && dl->handle) { dlclose(dl->handle); dl->handle = NULL; } talloc_free(dl_loader); + + /* + * Ensure our atexit handlers run before any other + * atexit handlers registered by third party libraries. + */ + fr_atexit_global_trigger_all(); } int LLVMFuzzerInitialize(int *argc, char ***argv) @@ -146,8 +149,7 @@ int LLVMFuzzerInitialize(int *argc, char ***argv) fr_exit_now(EXIT_FAILURE); } - dict_gctx = fr_dict_global_ctx_init(NULL, true, dict_dir); - if (!dict_gctx) { + if (!fr_dict_global_ctx_init(NULL, true, dict_dir)) { fr_perror("dict_global"); fr_exit_now(EXIT_FAILURE); } diff --git a/src/bin/radclient.c b/src/bin/radclient.c index 6be1f43424..9919a0daa9 100644 --- a/src/bin/radclient.c +++ b/src/bin/radclient.c @@ -1642,9 +1642,13 @@ int main(int argc, char **argv) ); } - if ((stats.lost > 0) || (stats.failed > 0)) { - fr_exit_now(EXIT_FAILURE); - } + /* + * Ensure our atexit handlers run before any other + * atexit handlers registered by third party libraries. + */ + fr_atexit_global_trigger_all(); - fr_exit_now(EXIT_SUCCESS); + if ((stats.lost > 0) || (stats.failed > 0)) return EXIT_FAILURE; + + return EXIT_SUCCESS; } diff --git a/src/bin/radict.c b/src/bin/radict.c index 2e3a37365d..d3079ab8a8 100644 --- a/src/bin/radict.c +++ b/src/bin/radict.c @@ -248,7 +248,6 @@ int main(int argc, char *argv[]) char const *protocol = NULL; TALLOC_CTX *autofree; - fr_dict_gctx_t const *our_dict_gctx = NULL; /* * Must be called first, so the handler is called last @@ -312,8 +311,7 @@ int main(int argc, char *argv[]) goto finish; } - our_dict_gctx = fr_dict_global_ctx_init(NULL, true, dict_dir); - if (!our_dict_gctx) { + if (!fr_dict_global_ctx_init(NULL, true, dict_dir)) { fr_perror("radict"); ret = 1; goto finish; @@ -399,7 +397,13 @@ finish: fr_dict_free(dict_p, __FILE__); } while (++dict_p < dict_end); } - if (fr_dict_global_ctx_free(our_dict_gctx) < 0) fr_perror("radict"); if (talloc_free(autofree) < 0) fr_perror("radict"); + + /* + * Ensure our atexit handlers run before any other + * atexit handlers registered by third party libraries. + */ + fr_atexit_global_trigger_all(); + return found ? ret : 64; } diff --git a/src/bin/radiusd.c b/src/bin/radiusd.c index c737a0e43f..a9e502a2d5 100644 --- a/src/bin/radiusd.c +++ b/src/bin/radiusd.c @@ -1125,6 +1125,12 @@ cleanup: if (!rad_suid_is_down_permanent() && (fr_get_lsan_state() == 1)) rad_suid_up(); fr_strerror_clear(); /* clear error buffer */ + /* + * Ensure our atexit handlers run before any other + * atexit handlers registered by third party libraries. + */ + fr_atexit_global_trigger_all(); + return ret; } diff --git a/src/bin/radsniff.c b/src/bin/radsniff.c index 66aec78bbe..548ef4573f 100644 --- a/src/bin/radsniff.c +++ b/src/bin/radsniff.c @@ -2246,7 +2246,6 @@ int main(int argc, char *argv[]) char const *raddb_dir = RADDBDIR; char const *dict_dir = DICTDIR; TALLOC_CTX *autofree; - fr_dict_gctx_t const *dict_gctx = NULL; rs_stats_t *stats; @@ -2597,8 +2596,7 @@ int main(int argc, char *argv[]) conf->pcap_filter, conf->pcap_filter); } - dict_gctx = fr_dict_global_ctx_init(NULL, true, dict_dir); - if (!dict_gctx) { + if (!fr_dict_global_ctx_init(NULL, true, dict_dir)) { fr_perror("radsniff"); fr_exit_now(EXIT_FAILURE); } @@ -3038,10 +3036,11 @@ finish: fr_dict_autofree(radsniff_dict); fr_radius_free(); - if (fr_dict_global_ctx_free(dict_gctx) < 0) { - fr_perror("radsniff"); - ret = EXIT_FAILURE; - } + /* + * Ensure our atexit handlers run before any other + * atexit handlers registered by third party libraries. + */ + fr_atexit_global_trigger_all(); return ret; } diff --git a/src/bin/radsnmp.c b/src/bin/radsnmp.c index 7791a30db3..60118b6bd1 100644 --- a/src/bin/radsnmp.c +++ b/src/bin/radsnmp.c @@ -1154,5 +1154,11 @@ finish: */ fr_dict_autofree(radsnmp_dict); + /* + * Ensure our atexit handlers run before any other + * atexit handlers registered by third party libraries. + */ + fr_atexit_global_trigger_all(); + return ret; } diff --git a/src/bin/radwho.c b/src/bin/radwho.c index e1d57d7a16..7d8c6dedd1 100644 --- a/src/bin/radwho.c +++ b/src/bin/radwho.c @@ -198,7 +198,6 @@ int main(int argc, char **argv) int zap = 0; fr_dict_t *dict = NULL; TALLOC_CTX *autofree; - fr_dict_gctx_t const *dict_gctx = NULL; char const *p; main_config_t *config; @@ -307,8 +306,7 @@ int main(int argc, char **argv) return 1; } - dict_gctx = fr_dict_global_ctx_init(NULL, true, config->dict_dir); - if (!dict_gctx) { + if (!fr_dict_global_ctx_init(NULL, true, config->dict_dir)) { fr_perror("%s", main_config->name); fr_exit_now(EXIT_FAILURE); } @@ -563,11 +561,13 @@ int main(int argc, char **argv) } fclose(fp); - if (fr_dict_global_ctx_free(dict_gctx) < 0) { - fr_perror("radwho"); - ret = EXIT_FAILURE; - } main_config_free(&config); + /* + * Ensure our atexit handlers run before any other + * atexit handlers registered by third party libraries. + */ + fr_atexit_global_trigger_all(); + return ret; } diff --git a/src/bin/unit_test_attribute.c b/src/bin/unit_test_attribute.c index 39e810cc0b..8704fb3e8c 100644 --- a/src/bin/unit_test_attribute.c +++ b/src/bin/unit_test_attribute.c @@ -3760,5 +3760,11 @@ cleanup: ret = EXIT_FAILURE; } + /* + * Ensure our atexit handlers run before any other + * atexit handlers registered by third party libraries. + */ + fr_atexit_global_trigger_all(); + return ret; } diff --git a/src/bin/unit_test_map.c b/src/bin/unit_test_map.c index 13300e7ba9..b35569d6c4 100644 --- a/src/bin/unit_test_map.c +++ b/src/bin/unit_test_map.c @@ -89,10 +89,6 @@ static int process_file(char const *filename) }; map_list_init(&list); - /* - * Must be called first, so the handler is called last - */ - fr_atexit_global_setup(); config = main_config_alloc(NULL); if (!config) { @@ -168,7 +164,6 @@ int main(int argc, char *argv[]) char const *receipt_file = NULL; TALLOC_CTX *autofree; - fr_dict_gctx_t const *dict_gctx = NULL; /* * Must be called first, so the handler is called last @@ -237,8 +232,7 @@ int main(int argc, char *argv[]) EXIT_WITH_FAILURE; } - dict_gctx = fr_dict_global_ctx_init(NULL, true, dict_dir); - if (!dict_gctx) { + if (!fr_dict_global_ctx_init(NULL, true, dict_dir)) { fr_perror("unit_test_map"); EXIT_WITH_FAILURE; } @@ -299,15 +293,16 @@ cleanup: ret = EXIT_FAILURE; } - if (fr_dict_global_ctx_free(dict_gctx) < 0) { - fr_perror("unit_test_map"); - ret = EXIT_FAILURE; - } - if (receipt_file && (ret == EXIT_SUCCESS) && (fr_touch(NULL, receipt_file, 0644, true, 0755) <= 0)) { fr_perror("unit_test_map"); ret = EXIT_FAILURE; } + /* + * Ensure our atexit handlers run before any other + * atexit handlers registered by third party libraries. + */ + fr_atexit_global_trigger_all(); + return ret; } diff --git a/src/bin/unit_test_module.c b/src/bin/unit_test_module.c index e0131bf6f0..3d59bcce19 100644 --- a/src/bin/unit_test_module.c +++ b/src/bin/unit_test_module.c @@ -548,7 +548,6 @@ int main(int argc, char *argv[]) char const *receipt_file = NULL; TALLOC_CTX *autofree; - fr_dict_gctx_t const *dict_gctx = NULL; TALLOC_CTX *thread_ctx; char *p; @@ -723,8 +722,7 @@ int main(int argc, char *argv[]) */ modules_init(config->lib_dir); - dict_gctx = fr_dict_global_ctx_init(NULL, true, config->dict_dir); - if (!dict_gctx) { + if (!fr_dict_global_ctx_init(NULL, true, config->dict_dir)) { fr_perror("%s", config->name); EXIT_WITH_FAILURE; } @@ -1079,6 +1077,12 @@ cleanup: ret = EXIT_FAILURE; } + /* + * Ensure our atexit handlers run before any other + * atexit handlers registered by third party libraries. + */ + fr_atexit_global_trigger_all(); + return ret; } diff --git a/src/lib/util/atexit.c b/src/lib/util/atexit.c index 273c4ae447..8e419e82f2 100644 --- a/src/lib/util/atexit.c +++ b/src/lib/util/atexit.c @@ -348,6 +348,45 @@ void fr_atexit_thread_local_disarm_all(void) } } +/** Cause all thread local free triggers to fire + * + * This is necessary when we're running in single threaded mode + * to ensure all "thread-local" memory (which isn't actually thread local) + * is cleaned up. + * + * One example is the OpenSSL log BIOs which must be cleaned up + * before fr_openssl_free is called. + */ +unsigned int fr_atexit_thread_trigger_all(void) +{ + fr_atexit_entry_t *e = NULL, *ee; + fr_atexit_list_t *list; + unsigned int count = 0; + + /* + * Iterate over the list of thread local + * destructor lists running the + * destructors. + */ + while ((e = fr_dlist_next(&fr_atexit_threads->head, e))) { + if (!e->func) continue; /* thread already joined */ + + list = talloc_get_type_abort(e->uctx, fr_atexit_list_t); + ee = NULL; + while ((ee = fr_dlist_next(&list->head, ee))) { + ATEXIT_DEBUG("%s - Thread %u triggering %p/%p func=%p, uctx=%p (alloced %s:%u)", + __FUNCTION__, + (unsigned int)pthread_self(), + list, ee, ee->func, ee->uctx, ee->file, ee->line); + + count++; + ee = fr_dlist_talloc_free_item(&list->head, ee); + } + } + + return count; +} + /** Remove a specific global destructor (without executing it) * * @note This function's primary purpose is to help diagnose issues with destructors @@ -406,6 +445,35 @@ void fr_atexit_global_disarm_all(void) } } +/** Cause all global free triggers to fire + * + * This is necessary when libraries (perl) register their own + * atexit handlers using the normal POSIX mechanism, and we need + * to ensure all our atexit handlers fire before so any global + * deinit is done explicitly by us. + */ +unsigned int fr_atexit_global_trigger_all(void) +{ + fr_atexit_entry_t *e = NULL; + unsigned int count = 0; + + /* + * Iterate over the list of thread local + * destructor lists running the + * destructors. + */ + while ((e = fr_dlist_next(&fr_atexit_global->head, e))) { + ATEXIT_DEBUG("%s - Triggering %p/%p func=%p, uctx=%p (alloced %s:%u)", + __FUNCTION__, + fr_atexit_global, e, e->func, e->uctx, e->file, e->line); + + count++; + e = fr_dlist_talloc_free_item(&fr_atexit_global->head, e); + } + + return count; +} + /** Iterates through all thread local destructor lists, causing destructor to be triggered * * This should only be called by the main process not by threads. @@ -470,44 +538,6 @@ do_threads: return count; } -/** Cause all thread local free triggers to fire - * - * This is necessary when we're running in single threaded mode - * to ensure all "thread-local" memory (which isn't actually thread local) - * is cleaned up. - * - * One example is the OpenSSL log BIOs which must be cleaned up - * before fr_openssl_free is called. - */ -unsigned int fr_atexit_thread_trigger_all(void) -{ - fr_atexit_entry_t *e = NULL, *ee; - fr_atexit_list_t *list; - unsigned int count = 0; - - /* - * Iterate over the list of thread local - * destructor lists running the - * destructors. - */ - while ((e = fr_dlist_next(&fr_atexit_threads->head, e))) { - if (!e->func) continue; /* thread already joined */ - - list = talloc_get_type_abort(e->uctx, fr_atexit_list_t); - ee = NULL; - while ((ee = fr_dlist_next(&list->head, ee))) { - ATEXIT_DEBUG("%s - Thread %u triggering %p/%p func=%p, uctx=%p (alloced %s:%u)", - __FUNCTION__, - (unsigned int)pthread_self(), - list, ee, ee->func, ee->uctx, ee->file, ee->line); - - count++; - ee = fr_dlist_talloc_free_item(&list->head, ee); - } - } - - return count; -} /** Return whether we're currently in the teardown phase * diff --git a/src/lib/util/atexit.h b/src/lib/util/atexit.h index 9d1dc0911b..e8b42a11b7 100644 --- a/src/lib/util/atexit.h +++ b/src/lib/util/atexit.h @@ -117,13 +117,15 @@ unsigned int fr_atexit_thread_local_disarm(bool uctx_scope, fr_atexit_t func, vo void fr_atexit_thread_local_disarm_all(void); +unsigned int fr_atexit_thread_trigger_all(void); + unsigned int fr_atexit_global_disarm(bool uctx_scope, fr_atexit_t func, void const *uctx); void fr_atexit_global_disarm_all(void); -unsigned int fr_atexit_trigger(bool uctx_scope, fr_atexit_t func, void const *uctx); +unsigned int fr_atexit_global_trigger_all(void); -unsigned int fr_atexit_thread_trigger_all(void); +unsigned int fr_atexit_trigger(bool uctx_scope, fr_atexit_t func, void const *uctx); bool fr_atexit_is_exiting(void); diff --git a/src/listen/control/radmin.c b/src/listen/control/radmin.c index aaebe90396..fcb349cc17 100644 --- a/src/listen/control/radmin.c +++ b/src/listen/control/radmin.c @@ -843,7 +843,6 @@ int main(int argc, char **argv) #endif TALLOC_CTX *autofree; - fr_dict_gctx_t const *dict_gctx = NULL; char *commands[MAX_COMMANDS]; int num_commands = -1; @@ -975,8 +974,7 @@ int main(int argc, char **argv) * Need to read in the dictionaries, else we may get * validation errors when we try and parse the config. */ - dict_gctx = fr_dict_global_ctx_init(NULL, true, dict_dir); - if (!dict_gctx) { + if (!fr_dict_global_ctx_init(NULL, true, dict_dir)) { fr_perror("radmin"); fr_exit_now(64); } @@ -1322,8 +1320,6 @@ int main(int argc, char **argv) exit: fr_dict_free(&dict, __FILE__); - if (fr_dict_global_ctx_free(dict_gctx) < 0) fr_perror("radmin"); - if (inputfp != stdin) fclose(inputfp); if (radmin_log.dst == L_DST_FILES) close(radmin_log.fd); @@ -1332,5 +1328,11 @@ exit: if (!quiet) fprintf(stdout, "\n"); + /* + * Ensure our atexit handlers run before any other + * atexit handlers registered by third party libraries. + */ + fr_atexit_global_trigger_all(); + return exit_status; } diff --git a/src/tests/radclient/acct_1.out b/src/tests/radclient/acct_1.out index 4183666c4f..853836238a 100644 --- a/src/tests/radclient/acct_1.out +++ b/src/tests/radclient/acct_1.out @@ -3,3 +3,4 @@ Sent Accounting-Request Id 123 from 0.0.0.0:1234 to 127.0.0.1:12340 length 43 User-Password = "hello" Password.Cleartext = "hello" Received Accounting-Response Id 123 from 127.0.0.1:12340 to 0.0.0.0:1234 via lo length 20 +(0) src/tests/radclient/acct_1.txt response code 5 diff --git a/src/tests/radclient/auth_1.out b/src/tests/radclient/auth_1.out index f59a3e8154..970ed35712 100644 --- a/src/tests/radclient/auth_1.out +++ b/src/tests/radclient/auth_1.out @@ -3,3 +3,4 @@ Sent Access-Request Id 123 from 0.0.0.0:1234 to 127.0.0.1:12340 length 43 User-Password = "hello" Password.Cleartext = "hello" Received Access-Accept Id 123 from 127.0.0.1:12340 to 0.0.0.0:1234 via lo length 20 +(0) src/tests/radclient/auth_1.txt response code 2 diff --git a/src/tests/radclient/auth_4.out b/src/tests/radclient/auth_4.out index 5e80983bba..d271130235 100644 --- a/src/tests/radclient/auth_4.out +++ b/src/tests/radclient/auth_4.out @@ -7,3 +7,4 @@ Received Access-Accept Id 123 from 127.0.0.1:12340 to 0.0.0.0:1234 via lo length Class = 0x483d342c493d34 raw.Vendor-Specific = 0x483d342c493d34 raw.Vendor-Specific = 0x483d342c493d43 +(0) src/tests/radclient/auth_4.txt response code 2