From 3aae792b1021a3e46490bd52e8b3300c3aa71e82 Mon Sep 17 00:00:00 2001 From: Kevin Svetlitski Date: Mon, 17 Jul 2023 15:22:26 -0700 Subject: [PATCH] Fix infinite purging loop in HPA As reported in #2449, under certain circumstances it's possible to get stuck in an infinite loop attempting to purge from the HPA. We now handle this by validating the HPA settings at the end of configuration parsing and either normalizing them or aborting depending on if `abort_conf` is set. --- Makefile.in | 1 + .../internal/jemalloc_internal_externs.h | 1 + src/jemalloc.c | 44 ++++++++++++++- test/unit/hpa_background_thread.sh | 2 +- test/unit/hpa_validate_conf.c | 56 +++++++++++++++++++ test/unit/hpa_validate_conf.sh | 3 + 6 files changed, 105 insertions(+), 2 deletions(-) create mode 100644 test/unit/hpa_validate_conf.c create mode 100644 test/unit/hpa_validate_conf.sh diff --git a/Makefile.in b/Makefile.in index a0131558..3a02b3fd 100644 --- a/Makefile.in +++ b/Makefile.in @@ -225,6 +225,7 @@ TESTS_UNIT := \ $(srcroot)test/unit/hook.c \ $(srcroot)test/unit/hpa.c \ $(srcroot)test/unit/hpa_background_thread.c \ + $(srcroot)test/unit/hpa_validate_conf.c \ $(srcroot)test/unit/hpdata.c \ $(srcroot)test/unit/huge.c \ $(srcroot)test/unit/inspect.c \ diff --git a/include/jemalloc/internal/jemalloc_internal_externs.h b/include/jemalloc/internal/jemalloc_internal_externs.h index ae03c644..64d9aa20 100644 --- a/include/jemalloc/internal/jemalloc_internal_externs.h +++ b/include/jemalloc/internal/jemalloc_internal_externs.h @@ -25,6 +25,7 @@ extern bool opt_junk_alloc; extern bool opt_junk_free; extern void (*JET_MUTABLE junk_free_callback)(void *ptr, size_t size); extern void (*JET_MUTABLE junk_alloc_callback)(void *ptr, size_t size); +extern void (*JET_MUTABLE invalid_conf_abort)(void); extern bool opt_utrace; extern bool opt_xmalloc; extern bool opt_experimental_infallible_new; diff --git a/src/jemalloc.c b/src/jemalloc.c index ccb20c81..c5a06f6e 100644 --- a/src/jemalloc.c +++ b/src/jemalloc.c @@ -144,6 +144,7 @@ static void default_junk_free(void *ptr, size_t usize) { void (*JET_MUTABLE junk_alloc_callback)(void *ptr, size_t size) = &default_junk_alloc; void (*JET_MUTABLE junk_free_callback)(void *ptr, size_t size) = &default_junk_free; +void (*JET_MUTABLE invalid_conf_abort)(void) = &abort; bool opt_utrace = false; bool opt_xmalloc = false; @@ -959,7 +960,7 @@ malloc_abort_invalid_conf(void) { assert(opt_abort_conf); malloc_printf(": Abort (abort_conf:true) on invalid conf " "value (see above).\n"); - abort(); + invalid_conf_abort(); } static void @@ -1081,6 +1082,46 @@ obtain_malloc_conf(unsigned which_source, char buf[PATH_MAX + 1]) { return ret; } +static void +validate_hpa_settings(void) { + if (!hpa_supported() || !opt_hpa || opt_hpa_opts.dirty_mult == (fxp_t)-1) { + return; + } + size_t hpa_threshold = fxp_mul_frac(HUGEPAGE, opt_hpa_opts.dirty_mult) + + opt_hpa_opts.hugification_threshold; + if (hpa_threshold > HUGEPAGE) { + return; + } + + had_conf_error = true; + char hpa_dirty_mult[FXP_BUF_SIZE]; + char hugification_threshold[FXP_BUF_SIZE]; + char normalization_message[256] = {0}; + fxp_print(opt_hpa_opts.dirty_mult, hpa_dirty_mult); + fxp_print(fxp_div(FXP_INIT_INT((unsigned) + (opt_hpa_opts.hugification_threshold >> LG_PAGE)), + FXP_INIT_INT(HUGEPAGE_PAGES)), hugification_threshold); + if (!opt_abort_conf) { + char normalized_hugification_threshold[FXP_BUF_SIZE]; + opt_hpa_opts.hugification_threshold += + HUGEPAGE - hpa_threshold; + fxp_print(fxp_div(FXP_INIT_INT((unsigned) + (opt_hpa_opts.hugification_threshold >> LG_PAGE)), + FXP_INIT_INT(HUGEPAGE_PAGES)), + normalized_hugification_threshold); + malloc_snprintf(normalization_message, + sizeof(normalization_message), ": Normalizing " + "HPA settings to avoid pathological behavior, setting " + "hpa_hugification_threshold_ratio: to %s.\n", + normalized_hugification_threshold); + } + malloc_printf( + ": Invalid combination of options " + "hpa_hugification_threshold_ratio: %s and hpa_dirty_mult: %s. " + "These values should sum to > 1.0.\n%s", hugification_threshold, + hpa_dirty_mult, normalization_message); +} + static void malloc_conf_init_helper(sc_data_t *sc_data, unsigned bin_shard_sizes[SC_NBINS], bool initial_call, const char *opts_cache[MALLOC_CONF_NSOURCES], @@ -1749,6 +1790,7 @@ malloc_conf_init_helper(sc_data_t *sc_data, unsigned bin_shard_sizes[SC_NBINS], /* Re-enable diagnostic "-Wtype-limits" */ JEMALLOC_DIAGNOSTIC_POP } + validate_hpa_settings(); if (opt_abort_conf && had_conf_error) { malloc_abort_invalid_conf(); } diff --git a/test/unit/hpa_background_thread.sh b/test/unit/hpa_background_thread.sh index 65a56a08..33b70e19 100644 --- a/test/unit/hpa_background_thread.sh +++ b/test/unit/hpa_background_thread.sh @@ -1,4 +1,4 @@ #!/bin/sh -export MALLOC_CONF="hpa_dirty_mult:0,hpa_min_purge_interval_ms:50,hpa_sec_nshards:0" +export MALLOC_CONF="hpa_dirty_mult:0.001,hpa_hugification_threshold_ratio:1.0,hpa_min_purge_interval_ms:50,hpa_sec_nshards:0" diff --git a/test/unit/hpa_validate_conf.c b/test/unit/hpa_validate_conf.c new file mode 100644 index 00000000..8c1847ba --- /dev/null +++ b/test/unit/hpa_validate_conf.c @@ -0,0 +1,56 @@ +#include "test/jemalloc_test.h" + +static bool abort_called = false; +static void (*default_malloc_message)(void *, const char *); + +static void +mock_invalid_conf_abort(void) { + abort_called = true; +} + +static void +null_malloc_message(void *_1, const char* _2) { +} + +TEST_BEGIN(test_hpa_validate_conf) { + test_skip_if(!hpa_supported()); + void *ptr = malloc(4096); + /* Need to restore this here to see any possible assert messages */ + malloc_message = default_malloc_message; + assert_true(abort_called, + "Should have aborted due to invalid values for hpa_dirty_mult and " + "hpa_hugification_threshold_ratio"); + free(ptr); +} +TEST_END + +/* + * We have to set `abort_conf:true` here and not via the `MALLOC_CONF` + * environment variable in the associated shell script for this test. This is + * because when testing on FreeBSD (where Jemalloc is the system allocator) in + * CI configs where HPA is not supported, setting `abort_conf:true` there would + * result in the system Jemalloc picking this up and aborting before we could + * ever even launch the test. + */ +const char *malloc_conf = "abort_conf:true"; + +int +main(void) { + /* + * OK, this is a sort of nasty hack. We don't want to add *another* + * config option for HPA (the intent is that it becomes available on + * more platforms over time, and we're trying to prune back config + * options generally. But we'll get initialization errors on other + * platforms if we set hpa:true in the MALLOC_CONF (even if we set + * abort_conf:false as well). So we reach into the internals and set + * them directly, but only if we know that we're actually going to do + * something nontrivial in the tests. + */ + if (hpa_supported()) { + default_malloc_message = malloc_message; + malloc_message = null_malloc_message; + opt_hpa = true; + invalid_conf_abort = mock_invalid_conf_abort; + } + return test_no_reentrancy(test_hpa_validate_conf); +} diff --git a/test/unit/hpa_validate_conf.sh b/test/unit/hpa_validate_conf.sh new file mode 100644 index 00000000..692c3da9 --- /dev/null +++ b/test/unit/hpa_validate_conf.sh @@ -0,0 +1,3 @@ +#!/bin/sh + +export MALLOC_CONF='tcache:false,hpa_dirty_mult:0.25,hpa_hugification_threshold_ratio:0.6'