Comment 2 for bug 1750013

Revision history for this message
Guilherme G. Piccoli (gpiccoli) wrote :

It was noticed that during the logind manager logic for creating a session
based on dbus event, the sessions's initialization procedure will allocate
2 structures related to cgroup management of a session: controllers and
reset_controllers.

Both these structs are filled with cgroup controllers (when they exist)
in bus_parse_strv_iter(), which will always allocate at least one of each
to be used as a parent for a "string"-based list of elements abstraction.

Currently, the code does not free reset_controllers, so we have a memory
leak at each session creation, which shows up in a valgrind output like
this:

2,696 bytes in 337 blocks are definitely lost in loss record 99 of 101
at 0x4C2AB80: malloc (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
by 0x434F58: malloc_multiply (util.h:564)
by 0x4368DA: bus_parse_strv_iter (dbus-common.c:905)
by 0x40AE9D: bus_manager_create_session (logind-dbus.c:498)
by 0x40EB39: manager_message_handler (logind-dbus.c:1752)
by 0x5ED8E85: ??? (in /lib/x86_64-linux-gnu/libdbus-1.so.3.7.6)
by 0x5ECBA20: dbus_connection_dispatch (in /lib/x86_64-linux-gnu/libdbus-1.so.3.7.6)
by 0x409E21: manager_run (logind.c:1729)
by 0x40A27C: main (logind.c:1860)

Notice this was after 337 sessions.
--

I wrote a small patch that addresses the issue by just adding the
necessary strv_free(). It diverges from upstream systemd since
reset_controllers entity was removed from the code after the refactor
on cgroup's handling made by commit
fb6becb ("logind: port over to use scopes+slices for all cgroup stuff").

The strv_free() call has no known risk and potential double-frees aren't
expected since strv_free() validates the struct and its "children"
before de-allocate the resources.

The patch:

diff --git a/src/login/logind-session.c b/src/login/logind-session.c
index 662273b..1a7389a 100644
--- a/src/login/logind-session.c
+++ b/src/login/logind-session.c
@@ -93,6 +93,7 @@ void session_free(Session *s) {

         free(s->cgroup_path);
         strv_free(s->controllers);
+ strv_free(s->reset_controllers);

         free(s->tty);
         free(s->display);

After testing with both patches already mentioned and cgmanager, there
is still a (really) small leak to be investigated.