From: Zbigniew Jędrzejewski-Szmek Date: Thu, 28 May 2020 12:58:35 +0000 (+0200) Subject: shared/unit-file: make sure the old hashmaps and sets are freed upon replacement X-Git-Tag: v246-rc1~248^2~2 X-Git-Url: http://git.ipfire.org/gitweb.cgi?a=commitdiff_plain;h=3fb2326f3ed87aa0b26078d307ebfb299e36286d;p=thirdparty%2Fsystemd.git shared/unit-file: make sure the old hashmaps and sets are freed upon replacement Possibly fixes #15220. (There might be another leak. I'm still investigating.) The leak would occur when the path cache was rebuilt. So in normal circumstances it wouldn't be too bad, since usually the path cache is not rebuilt too often. But the case in #15220, where new unit files are created in a loop and started, the leak occurs once for each unit file: $ for i in {1..300}; do cp ~/.config/systemd/user/test0001.service ~/.config/systemd/user/test$(printf %04d $i).service; systemctl --user start test$(printf %04d $i).service;done --- diff --git a/src/basic/hash-funcs.c b/src/basic/hash-funcs.c index fee0ba98eba..cf279e5cbef 100644 --- a/src/basic/hash-funcs.c +++ b/src/basic/hash-funcs.c @@ -55,6 +55,8 @@ void path_hash_func(const char *q, struct siphash *state) { } DEFINE_HASH_OPS(path_hash_ops, char, path_hash_func, path_compare); +DEFINE_HASH_OPS_WITH_KEY_DESTRUCTOR(path_hash_ops_free, + char, path_hash_func, path_compare, free); void trivial_hash_func(const void *p, struct siphash *state) { siphash24_compress(&p, sizeof(p), state); diff --git a/src/basic/hash-funcs.h b/src/basic/hash-funcs.h index 264316c3dc1..005d1b21d21 100644 --- a/src/basic/hash-funcs.h +++ b/src/basic/hash-funcs.h @@ -81,6 +81,7 @@ extern const struct hash_ops string_hash_ops_free_free; void path_hash_func(const char *p, struct siphash *state); extern const struct hash_ops path_hash_ops; +extern const struct hash_ops path_hash_ops_free; /* This will compare the passed pointers directly, and will not dereference them. This is hence not useful for strings * or suchlike. */ diff --git a/src/basic/hashmap.h b/src/basic/hashmap.h index 65adc92513d..a3bc3281421 100644 --- a/src/basic/hashmap.h +++ b/src/basic/hashmap.h @@ -89,6 +89,14 @@ OrderedHashmap *internal_ordered_hashmap_new(const struct hash_ops *hash_ops HA #define hashmap_new(ops) internal_hashmap_new(ops HASHMAP_DEBUG_SRC_ARGS) #define ordered_hashmap_new(ops) internal_ordered_hashmap_new(ops HASHMAP_DEBUG_SRC_ARGS) +#define hashmap_free_and_replace(a, b) \ + ({ \ + hashmap_free(a); \ + (a) = (b); \ + (b) = NULL; \ + 0; \ + }) + HashmapBase *internal_hashmap_free(HashmapBase *h, free_func_t default_free_key, free_func_t default_free_value); static inline Hashmap *hashmap_free(Hashmap *h) { return (void*) internal_hashmap_free(HASHMAP_BASE(h), NULL, NULL); diff --git a/src/basic/set.h b/src/basic/set.h index f3501d17ae7..8a95fec05f8 100644 --- a/src/basic/set.h +++ b/src/basic/set.h @@ -5,6 +5,14 @@ #include "hashmap.h" #include "macro.h" +#define set_free_and_replace(a, b) \ + ({ \ + set_free(a); \ + (a) = (b); \ + (b) = NULL; \ + 0; \ + }) + Set *internal_set_new(const struct hash_ops *hash_ops HASHMAP_DEBUG_PARAMS); #define set_new(ops) internal_set_new(ops HASHMAP_DEBUG_SRC_ARGS) diff --git a/src/core/manager.c b/src/core/manager.c index f15f845e819..7acbbb0b9ea 100644 --- a/src/core/manager.c +++ b/src/core/manager.c @@ -696,7 +696,7 @@ static int manager_setup_prefix(Manager *m) { static void manager_free_unit_name_maps(Manager *m) { m->unit_id_map = hashmap_free(m->unit_id_map); m->unit_name_map = hashmap_free(m->unit_name_map); - m->unit_path_cache = set_free_free(m->unit_path_cache); + m->unit_path_cache = set_free(m->unit_path_cache); m->unit_cache_mtime = 0; } diff --git a/src/shared/unit-file.c b/src/shared/unit-file.c index 4fe2489c55a..10968e18caa 100644 --- a/src/shared/unit-file.c +++ b/src/shared/unit-file.c @@ -229,9 +229,9 @@ static bool lookup_paths_mtime_good(const LookupPaths *lp, usec_t mtime) { int unit_file_build_name_map( const LookupPaths *lp, usec_t *cache_mtime, - Hashmap **ret_unit_ids_map, - Hashmap **ret_unit_names_map, - Set **ret_path_cache) { + Hashmap **unit_ids_map, + Hashmap **unit_names_map, + Set **path_cache) { /* Build two mappings: any name → main unit (i.e. the end result of symlink resolution), unit name → * all aliases (i.e. the entry for a given key is a a list of all names which point to this key). The @@ -239,7 +239,8 @@ int unit_file_build_name_map( * have a key, but it is not present in the value for itself, there was an alias pointing to it, but * the unit itself is not loadable. * - * At the same, build a cache of paths where to find units. + * At the same, build a cache of paths where to find units. The non-const parameters are for input + * and output. Existing contents will be freed before the new contents are stored. */ _cleanup_hashmap_free_ Hashmap *ids = NULL, *names = NULL; @@ -253,8 +254,8 @@ int unit_file_build_name_map( if (cache_mtime && *cache_mtime > 0 && lookup_paths_mtime_good(lp, *cache_mtime)) return 0; - if (ret_path_cache) { - paths = set_new(&path_hash_ops); + if (path_cache) { + paths = set_new(&path_hash_ops_free); if (!paths) return log_oom(); } @@ -296,7 +297,7 @@ int unit_file_build_name_map( if (!filename) return log_oom(); - if (ret_path_cache) { + if (paths) { r = set_consume(paths, filename); if (r < 0) return log_oom(); @@ -418,10 +419,11 @@ int unit_file_build_name_map( if (cache_mtime) *cache_mtime = mtime; - *ret_unit_ids_map = TAKE_PTR(ids); - *ret_unit_names_map = TAKE_PTR(names); - if (ret_path_cache) - *ret_path_cache = TAKE_PTR(paths); + + hashmap_free_and_replace(*unit_ids_map, ids); + hashmap_free_and_replace(*unit_names_map, names); + if (path_cache) + set_free_and_replace(*path_cache, paths); return 1; }