picnoir/Systemd
1
0
Fork 0
Code Issues Pull requests Projects Releases Wiki Activity

systemctl: avoid "leaking" some strings in UnitStatusInfo

Browse source 
% 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.)
This commit is contained in:
Zbigniew Jędrzejewski-Szmek 2016-07-25 12:15:57 -04:00
parent a733551846
commit 662bea6729
1 changed files with 19 additions and 4 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

23
src/systemctl/systemctl.c
View file

@ -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 = {};