systemctl: avoid "leaking" some strings in UnitStatusInfo

% valgrind --leak-check=full systemctl status multipathd.service --no-pager -n0
...
==431== 16 bytes in 2 blocks are definitely lost in loss record 1 of 2
==431==    at 0x4C2BBAD: malloc (vg_replace_malloc.c:299)
==431==    by 0x534AF19: strdup (in /usr/lib64/libc-2.23.so)
==431==    by 0x4E81AEE: free_and_strdup (string-util.c:794)
==431==    by 0x4EF66C1: map_basic (bus-util.c:1030)
==431==    by 0x4EF6A8E: bus_message_map_all_properties (bus-util.c:1153)
==431==    by 0x120487: show_one (systemctl.c:4672)
==431==    by 0x1218F3: show (systemctl.c:4990)
==431==    by 0x4EC359E: dispatch_verb (verbs.c:92)
==431==    by 0x12A3AE: systemctl_main (systemctl.c:7742)
==431==    by 0x12B1A8: main (systemctl.c:8011)
==431==
==431== LEAK SUMMARY:
==431==    definitely lost: 16 bytes in 2 blocks

This happens because map_basic() strdups the strings. Other code in systemctl
assigns strings to UnitStatusInfo without copying them, relying on the fact
that the message is longer lived than UnitStatusInfo. Add a helper function
that is similar to map_basic, but only accepts strings and does not copy them.
The alternative of continuing to use map_basic() but adding proper cleanup
to free fields in UnitStatusInfo seems less attractive because it'd require
changing a lot of code and doing a lot of more allocations for little gain.

(I put "leaking" in quotes, because systemctl is short lived anyway.)

diff --git a/src/systemctl/systemctl.c b/src/systemctl/systemctl.c
index e1bcef2d955e8639a8740826f1147fb3472c5563..d4ec1da2902b4f09ac1c974db6994efb10c32043 100644 (file)
--- a/src/systemctl/systemctl.c
+++ b/src/systemctl/systemctl.c
@@ -224,6 +224,21 @@ static void release_busses(void) {
                 busses[w] = sd_bus_flush_close_unref(busses[w]);
 }
 
+static int map_string_no_copy(sd_bus *bus, const char *member, sd_bus_message *m, sd_bus_error *error, void *userdata) {
+        char *s;
+        const char **p = userdata;
+        int r;
+
+        r = sd_bus_message_read_basic(m, SD_BUS_TYPE_STRING, &s);
+        if (r < 0)
+                return r;
+
+        if (!isempty(s))
+                *p = s;
+
+        return 0;
+}
+
 static void ask_password_agent_open_if_enabled(void) {
 
         /* Open the password agent as a child process if necessary */
@@ -4632,8 +4647,8 @@ static int show_one(
                 bool *ellipsized) {
 
         static const struct bus_properties_map property_map[] = {
-                { "LoadState",   "s", NULL, offsetof(UnitStatusInfo, load_state)   },
-                { "ActiveState", "s", NULL, offsetof(UnitStatusInfo, active_state) },
+                { "LoadState",   "s", map_string_no_copy, offsetof(UnitStatusInfo, load_state)   },
+                { "ActiveState", "s", map_string_no_copy, offsetof(UnitStatusInfo, active_state) },
                 {}
         };
 
@@ -5664,8 +5679,8 @@ static int unit_exists(const char *unit) {
         _cleanup_(sd_bus_error_free) sd_bus_error error = SD_BUS_ERROR_NULL;
         _cleanup_free_ char *path = NULL;
         static const struct bus_properties_map property_map[] = {
-                { "LoadState",   "s", NULL, offsetof(UnitStatusInfo, load_state)  },
-                { "ActiveState", "s", NULL, offsetof(UnitStatusInfo, active_state)},
+                { "LoadState",   "s", map_string_no_copy, offsetof(UnitStatusInfo, load_state)  },
+                { "ActiveState", "s", map_string_no_copy, offsetof(UnitStatusInfo, active_state)},
                 {},
         };
         UnitStatusInfo info = {};