Fix leak in unit path cache and another small optimization

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);

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. */

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);

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)
 

src/core/job.c

@@ -263,6 +263,10 @@ int job_install_deserialized(Job *j) {
                 return log_unit_debug_errno(j->unit, SYNTHETIC_ERRNO(EEXIST),
                                             "Unit already has a job installed. Not installing deserialized job.");
 
+        r = hashmap_ensure_allocated(&j->manager->jobs, NULL);
+        if (r < 0)
+                return r;
+
         r = hashmap_put(j->manager->jobs, UINT32_TO_PTR(j->id), j);
         if (r == -EEXIST)
                 return log_unit_debug_errno(j->unit, r, "Job ID %" PRIu32 " already used, cannot deserialize job.", j->id);

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;
 }
 
@@ -833,10 +833,6 @@ int manager_new(UnitFileScope scope, ManagerTestRunFlags test_run_flags, Manager
         if (r < 0)
                 return r;
 
-        r = hashmap_ensure_allocated(&m->jobs, NULL);
-        if (r < 0)
-                return r;
-
         r = hashmap_ensure_allocated(&m->cgroup_unit, &path_hash_ops);
         if (r < 0)
                 return r;
@@ -3963,6 +3959,11 @@ void manager_check_finished(Manager *m) {
                 return;
         }
 
+        /* The jobs hashmap tends to grow a lot during boot, and then it's not reused until shutdown. Let's
+           kill the hashmap if it is relatively large. */
+        if (hashmap_buckets(m->jobs) > hashmap_size(m->units) / 10)
+                m->jobs = hashmap_free(m->jobs);
+
         manager_flip_auto_status(m, false, "boot finished");
 
         /* Notify Type=idle units that we are done now */

src/core/transaction.c

@@ -656,6 +656,10 @@ static int transaction_apply(
                 assert(!j->transaction_prev);
                 assert(!j->transaction_next);
 
+                r = hashmap_ensure_allocated(&m->jobs, NULL);
+                if (r < 0)
+                        return r;
+
                 r = hashmap_put(m->jobs, UINT32_TO_PTR(j->id), j);
                 if (r < 0)
                         goto rollback;

src/core/unit.c

@@ -501,9 +501,7 @@ static void bidi_set_free(Unit *u, Hashmap *h) {
         /* Frees the hashmap and makes sure we are dropped from the inverse pointers */
 
         HASHMAP_FOREACH_KEY(v, other, h, i) {
-                UnitDependency d;
-
-                for (d = 0; d < _UNIT_DEPENDENCY_MAX; d++)
+                for (UnitDependency d = 0; d < _UNIT_DEPENDENCY_MAX; d++)
                         hashmap_remove(other->dependencies[d], u);
 
                 unit_add_to_gc_queue(other);
@@ -599,7 +597,6 @@ static void unit_done(Unit *u) {
 }
 
 void unit_free(Unit *u) {
-        UnitDependency d;
         Iterator i;
         char *t;
 
@@ -650,7 +647,7 @@ void unit_free(Unit *u) {
                 job_free(j);
         }
 
-        for (d = 0; d < _UNIT_DEPENDENCY_MAX; d++)
+        for (UnitDependency d = 0; d < _UNIT_DEPENDENCY_MAX; d++)
                 bidi_set_free(u, u->dependencies[d]);
 
         if (u->on_console)
@@ -875,19 +872,17 @@ static void merge_dependencies(Unit *u, Unit *other, const char *other_id, UnitD
         assert(d < _UNIT_DEPENDENCY_MAX);
 
         /* Fix backwards pointers. Let's iterate through all dependent units of the other unit. */
-        HASHMAP_FOREACH_KEY(v, back, other->dependencies[d], i) {
-                UnitDependency k;
-
-                /* Let's now iterate through the dependencies of that dependencies of the other units, looking for
-                 * pointers back, and let's fix them up, to instead point to 'u'. */
+        HASHMAP_FOREACH_KEY(v, back, other->dependencies[d], i)
 
-                for (k = 0; k < _UNIT_DEPENDENCY_MAX; k++) {
+                /* Let's now iterate through the dependencies of that dependencies of the other units,
+                 * looking for pointers back, and let's fix them up, to instead point to 'u'. */
+                for (UnitDependency k = 0; k < _UNIT_DEPENDENCY_MAX; k++)
                         if (back == u) {
                                 /* Do not add dependencies between u and itself. */
                                 if (hashmap_remove(back->dependencies[k], other))
                                         maybe_warn_about_dependency(u, other_id, k);
                         } else {
-                                UnitDependencyInfo di_u, di_other, di_merged;
+                                UnitDependencyInfo di_u, di_other;
 
                                 /* Let's drop this dependency between "back" and "other", and let's create it between
                                  * "back" and "u" instead. Let's merge the bit masks of the dependency we are moving,
@@ -899,7 +894,7 @@ static void merge_dependencies(Unit *u, Unit *other, const char *other_id, UnitD
 
                                 di_u.data = hashmap_get(back->dependencies[k], u);
 
-                                di_merged = (UnitDependencyInfo) {
+                                UnitDependencyInfo di_merged = {
                                         .origin_mask = di_u.origin_mask | di_other.origin_mask,
                                         .destination_mask = di_u.destination_mask | di_other.destination_mask,
                                 };
@@ -911,9 +906,6 @@ static void merge_dependencies(Unit *u, Unit *other, const char *other_id, UnitD
 
                                 /* assert_se(hashmap_remove_and_replace(back->dependencies[k], other, u, di_merged.data) >= 0); */
                         }
-                }
-
-        }
 
         /* Also do not move dependencies on u to itself */
         back = hashmap_remove(other->dependencies[d], u);
@@ -927,7 +919,6 @@ static void merge_dependencies(Unit *u, Unit *other, const char *other_id, UnitD
 }
 
 int unit_merge(Unit *u, Unit *other) {
-        UnitDependency d;
         const char *other_id = NULL;
         int r;
 
@@ -966,7 +957,7 @@ int unit_merge(Unit *u, Unit *other) {
                 other_id = strdupa(other->id);
 
         /* Make reservations to ensure merge_dependencies() won't fail */
-        for (d = 0; d < _UNIT_DEPENDENCY_MAX; d++) {
+        for (UnitDependency d = 0; d < _UNIT_DEPENDENCY_MAX; d++) {
                 r = reserve_dependencies(u, other, d);
                 /*
                  * We don't rollback reservations if we fail. We don't have
@@ -986,7 +977,7 @@ int unit_merge(Unit *u, Unit *other) {
                 unit_ref_set(other->refs_by_target, other->refs_by_target->source, u);
 
         /* Merge dependencies */
-        for (d = 0; d < _UNIT_DEPENDENCY_MAX; d++)
+        for (UnitDependency d = 0; d < _UNIT_DEPENDENCY_MAX; d++)
                 merge_dependencies(u, other, other_id, d);
 
         other->load_state = UNIT_MERGED;
@@ -1221,7 +1212,6 @@ static void print_unit_dependency_mask(FILE *f, const char *kind, UnitDependency
 
 void unit_dump(Unit *u, FILE *f, const char *prefix) {
         char *t, **j;
-        UnitDependency d;
         Iterator i;
         const char *prefix2;
         char timestamp[5][FORMAT_TIMESTAMP_MAX], timespan[FORMAT_TIMESPAN_MAX];
@@ -1377,7 +1367,7 @@ void unit_dump(Unit *u, FILE *f, const char *prefix) {
                         prefix, strna(format_timestamp(timestamp[0], sizeof(timestamp[0]), u->assert_timestamp.realtime)),
                         prefix, yes_no(u->assert_result));
 
-        for (d = 0; d < _UNIT_DEPENDENCY_MAX; d++) {
+        for (UnitDependency d = 0; d < _UNIT_DEPENDENCY_MAX; d++) {
                 UnitDependencyInfo di;
                 Unit *other;
 
@@ -1509,7 +1499,6 @@ int unit_add_default_target_dependency(Unit *u, Unit *target) {
 }
 
 static int unit_add_slice_dependencies(Unit *u) {
-        UnitDependencyMask mask;
         assert(u);
 
         if (!UNIT_HAS_CGROUP_CONTEXT(u))
@@ -1518,7 +1507,7 @@ static int unit_add_slice_dependencies(Unit *u) {
         /* Slice units are implicitly ordered against their parent slices (as this relationship is encoded in the
            name), while all other units are ordered based on configuration (as in their case Slice= configures the
            relationship). */
-        mask = u->type == UNIT_SLICE ? UNIT_DEPENDENCY_IMPLICIT : UNIT_DEPENDENCY_FILE;
+        UnitDependencyMask mask = u->type == UNIT_SLICE ? UNIT_DEPENDENCY_IMPLICIT : UNIT_DEPENDENCY_FILE;
 
         if (UNIT_ISSET(u->slice))
                 return unit_add_two_dependencies(u, UNIT_AFTER, UNIT_REQUIRES, UNIT_DEREF(u->slice), true, mask);
@@ -3205,6 +3194,43 @@ char *unit_dbus_path_invocation_id(Unit *u) {
         return unit_dbus_path_from_name(u->invocation_id_string);
 }
 
+static int unit_set_invocation_id(Unit *u, sd_id128_t id) {
+        int r;
+
+        assert(u);
+
+        /* Set the invocation ID for this unit. If we cannot, this will not roll back, but reset the whole thing. */
+
+        if (sd_id128_equal(u->invocation_id, id))
+                return 0;
+
+        if (!sd_id128_is_null(u->invocation_id))
+                (void) hashmap_remove_value(u->manager->units_by_invocation_id, &u->invocation_id, u);
+
+        if (sd_id128_is_null(id)) {
+                r = 0;
+                goto reset;
+        }
+
+        r = hashmap_ensure_allocated(&u->manager->units_by_invocation_id, &id128_hash_ops);
+        if (r < 0)
+                goto reset;
+
+        u->invocation_id = id;
+        sd_id128_to_string(id, u->invocation_id_string);
+
+        r = hashmap_put(u->manager->units_by_invocation_id, &u->invocation_id, u);
+        if (r < 0)
+                goto reset;
+
+        return 0;
+
+reset:
+        u->invocation_id = SD_ID128_NULL;
+        u->invocation_id_string[0] = 0;
+        return r;
+}
+
 int unit_set_slice(Unit *u, Unit *slice) {
         assert(u);
         assert(slice);
@@ -5344,43 +5370,6 @@ void unit_notify_user_lookup(Unit *u, uid_t uid, gid_t gid) {
                 unit_add_to_dbus_queue(u);
 }
 
-int unit_set_invocation_id(Unit *u, sd_id128_t id) {
-        int r;
-
-        assert(u);
-
-        /* Set the invocation ID for this unit. If we cannot, this will not roll back, but reset the whole thing. */
-
-        if (sd_id128_equal(u->invocation_id, id))
-                return 0;
-
-        if (!sd_id128_is_null(u->invocation_id))
-                (void) hashmap_remove_value(u->manager->units_by_invocation_id, &u->invocation_id, u);
-
-        if (sd_id128_is_null(id)) {
-                r = 0;
-                goto reset;
-        }
-
-        r = hashmap_ensure_allocated(&u->manager->units_by_invocation_id, &id128_hash_ops);
-        if (r < 0)
-                goto reset;
-
-        u->invocation_id = id;
-        sd_id128_to_string(id, u->invocation_id_string);
-
-        r = hashmap_put(u->manager->units_by_invocation_id, &u->invocation_id, u);
-        if (r < 0)
-                goto reset;
-
-        return 0;
-
-reset:
-        u->invocation_id = SD_ID128_NULL;
-        u->invocation_id_string[0] = 0;
-        return r;
-}
-
 int unit_acquire_invocation_id(Unit *u) {
         sd_id128_t id;
         int r;
@@ -5502,16 +5491,14 @@ static void unit_update_dependency_mask(Unit *u, UnitDependency d, Unit *other,
 }
 
 void unit_remove_dependencies(Unit *u, UnitDependencyMask mask) {
-        UnitDependency d;
-
         assert(u);
 
         /* Removes all dependencies u has on other units marked for ownership by 'mask'. */
 
         if (mask == 0)
                 return;
 
-        for (d = 0; d < _UNIT_DEPENDENCY_MAX; d++) {
+        for (UnitDependency d = 0; d < _UNIT_DEPENDENCY_MAX; d++) {
                 bool done;
 
                 do {
@@ -5522,8 +5509,6 @@ void unit_remove_dependencies(Unit *u, UnitDependencyMask mask) {
                         done = true;
 
                         HASHMAP_FOREACH_KEY(di.data, other, u->dependencies[d], i) {
-                                UnitDependency q;
-
                                 if ((di.origin_mask & ~mask) == di.origin_mask)
                                         continue;
                                 di.origin_mask &= ~mask;
@@ -5534,7 +5519,7 @@ void unit_remove_dependencies(Unit *u, UnitDependencyMask mask) {
                                  * all dependency types on the other unit and delete all those which point to us and
                                  * have the right mask set. */
 
-                                for (q = 0; q < _UNIT_DEPENDENCY_MAX; q++) {
+                                for (UnitDependency q = 0; q < _UNIT_DEPENDENCY_MAX; q++) {
                                         UnitDependencyInfo dj;
 
                                         dj.data = hashmap_get(other->dependencies[q], u);

src/core/unit.h

@@ -829,7 +829,6 @@ void unit_unref_uid_gid(Unit *u, bool destroy_now);
 
 void unit_notify_user_lookup(Unit *u, uid_t uid, gid_t gid);
 
-int unit_set_invocation_id(Unit *u, sd_id128_t id);
 int unit_acquire_invocation_id(Unit *u);
 
 bool unit_shall_confirm_spawn(Unit *u);