Merge pull request #15954 from keszybz/unit-file-leak

Fix leak in unit path cache and another small optimization

NEWS

@@ -338,6 +338,17 @@ CHANGES WITH 246 in spe:
           unit when the unit is stopped. It will now warn about services using
           KillMode=none, as this is generally an unsafe thing to make use of.
 
+        * .socket units gained a new boolean setting PassPacketInfo=. If
+          enabled, the kernel will attach additional per-packet metadata to all
+          packets read from the socket, as ancillary message. This controls the
+          IP_PKTINFO, IPV6_RECVPKTINFO, NETLINK_PKTINFO socket options,
+          depending on socket type.
+
+        * A new boolean option AssignAcquiredDelegatedPrefixAddress= has been
+          added to the [DHCPv6] section of .network files. If enabled (which is
+          the default) an address from any acquire delegated prefix is
+          automatically chosen and assigned to the interface.
+
 CHANGES WITH 245:
 
         * A new tool "systemd-repart" has been added, that operates as an

README.md

@@ -14,6 +14,7 @@ System and Service Manager
 [![Language Grade: C/C++](https://img.shields.io/lgtm/grade/cpp/g/systemd/systemd.svg?logo=lgtm&logoWidth=18)](https://lgtm.com/projects/g/systemd/systemd/context:cpp)<br/>
 [![CentOS CI Build Status](https://ci.centos.org/buildStatus/icon?job=systemd-pr-build)](https://ci.centos.org/job/systemd-pr-build/)<br/>
 [![Build Status](https://dev.azure.com/evvers/systemd-systemd/_apis/build/status/systemd.systemd?branchName=master)](https://dev.azure.com/evvers/systemd-systemd/_build/latest?definitionId=1&branchName=master)<br/>
+[![Fossies codespell report](https://fossies.org/linux/test/systemd-master.tar.gz/codespell.svg)](https://fossies.org/linux/test/systemd-master.tar.gz/codespell.html)</br>
 [![Packaging status](https://repology.org/badge/tiny-repos/systemd.svg)](https://repology.org/project/systemd/versions)
 
 ## Details

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/main.c

@@ -517,7 +517,7 @@ static int parse_proc_cmdline_item(const char *key, const char *value, void *dat
         } else if (!value) {
                 const char *target;
 
-                /* Compatible with SysV, but supported independently even if SysV compatiblity is disabled. */
+                /* Compatible with SysV, but supported independently even if SysV compatibility is disabled. */
                 target = runlevel_to_target(key);
                 if (target)
                         return free_and_strdup_warn(&arg_default_unit, target);

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;
+        HASHMAP_FOREACH_KEY(v, back, other->dependencies[d], i)
 
-                /* 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 (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,8 +5491,6 @@ 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'. */
@@ -5511,7 +5498,7 @@ void unit_remove_dependencies(Unit *u, UnitDependencyMask 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);

src/network/netdev/vxlan.c

@@ -45,7 +45,7 @@ static int netdev_vxlan_fill_message_create(NetDev *netdev, Link *link, sd_netli
                         r = sd_netlink_message_append_in6_addr(m, IFLA_VXLAN_GROUP6, &v->group.in6);
                 if (r < 0)
                         return log_netdev_error_errno(netdev, r, "Could not append IFLA_VXLAN_GROUP attribute: %m");
-        } else  if (in_addr_is_null(v->remote_family, &v->remote) == 0) {
+        } else if (in_addr_is_null(v->remote_family, &v->remote) == 0) {
                 if (v->remote_family == AF_INET)
                         r = sd_netlink_message_append_in_addr(m, IFLA_VXLAN_GROUP, &v->remote.in);
                 else

src/network/networkd-dhcp6.c

@@ -424,7 +424,7 @@ static int dhcp6_lease_pd_prefix_acquired(sd_dhcp6_client *client, Link *link) {
                  *    order. Prefixes that were previously already allocated to another
                  *    link will be skipped.
 
-                 * If a subnet id request couldn't be fullfilled the failure will be logged (as error)
+                 * If a subnet id request couldn't be fulfilled the failure will be logged (as error)
                  * and no further attempts at obtaining a prefix will be made.
 
                  * The assignment has to be split in two phases since subnet id
@@ -442,7 +442,7 @@ static int dhcp6_lease_pd_prefix_acquired(sd_dhcp6_client *client, Link *link) {
 
                 /* if r == -EAGAIN then the allocation failed because we ran
                  * out of addresses for the preferred subnet id's. This doesn't
-                 * mean we can't fullfill other prefix requests.
+                 * mean we can't fulfill other prefix requests.
                  *
                  * Since we do not have dedicated lists of links that request
                  * specific subnet id's and those that accept any prefix we
@@ -458,7 +458,7 @@ static int dhcp6_lease_pd_prefix_acquired(sd_dhcp6_client *client, Link *link) {
                         return r;
 
                 /* If the prefix distribution did return -EAGAIN we will try to
-                 * fullfill those with the next available pd delegated prefix. */
+                 * fulfill those with the next available pd delegated prefix. */
         }
 
         return 0;

src/network/networkd-network.c

@@ -739,6 +739,9 @@ static Network *network_free(Network *network) {
         free(network->dhcp_server_dns);
         free(network->dhcp_server_ntp);
         free(network->dhcp_server_sip);
+        free(network->dhcp_server_pop3);
+        free(network->dhcp_server_smtp);
+        free(network->dhcp_server_lpr);
 
         set_free_free(network->dnssec_negative_trust_anchors);
 

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

test/fuzz/fuzz-network-parser/github-15951

@@ -0,0 +1,2 @@
+[DHCPServer]
+POP3Servers=1.8.5.0