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 fee0ba98eba938f2e59e5235bda8417c6719a37c..cf279e5cbefc5673d66ca7369084d4682a12ff64 100644 (file)
--- 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 264316c3dc1ba93cb500aeaee58501e579220422..005d1b21d21ea0806ccda6f66ccaf104edc2369d 100644 (file)
--- 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 65adc92513d737854607a406ad15e0a13e0ef111..a3bc32814213a403b184e600409862ebe38148c8 100644 (file)
--- 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 f3501d17ae705dc2c0afa443ae71dca5898a906c..8a95fec05f8f2647a2f910508edd1643ad14b553 100644 (file)
--- 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 f15f845e819a6b55aa81d8064e5a10f72f3bf163..7acbbb0b9ea26457d52a3eb55363886ab8afab38 100644 (file)
--- 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 4fe2489c55a6785f265b9a034429c785a5fd9394..10968e18caa2d059c9aa1751e776db974de0ccaf 100644 (file)
--- 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;
 }