ell/main.c Memory leak

This is from iwd:

[DBUS] > 6c 04 01 01 98 00 00 00 04 00 00 00 68 00 00 00  l...........h...
[DBUS]   01 01 6f 00 01 00 00 00 2f 00 00 00 00 00 00 00  ..o...../.......
[DBUS]   03 01 73 00 0f 00 00 00 49 6e 74 65 72 66 61 63  ..s.....Interfac
[DBUS]   65 73 41 64 64 65 64 00 02 01 73 00 22 00 00 00  esAdded...s."...
[DBUS]   6f 72 67 2e 66 72 65 65 64 65 73 6b 74 6f 70 2e  org.freedesktop.
[DBUS]   44 42 75 73 2e 4f 62 6a 65 63 74 4d 61 6e 61 67  DBus.ObjectManag
[DBUS]   65 72 00 00 00 00 00 00 08 01 67 00 0a 6f 61 7b  er........g..oa{
[DBUS]   73 61 7b 73 76 7d 7d 00                          sa{sv}}.
[DBUS]   02 00 00 00 2f 33 00 00 88 00 00 00 00 00 00 00  ..../3..........
[DBUS]   16 00 00 00 6e 65 74 2e 63 6f 6e 6e 6d 61 6e 2e  ....net.connman.
[DBUS]   69 77 64 2e 44 65 76 69 63 65 00 00 33 00 00 00  iwd.Device..3...
[DBUS]   04 00 00 00 4e 61 6d 65 00 01 73 00 06 00 00 00  ....Name..s.....
[DBUS]   77 6c 70 32 73 30 00 00 07 00 00 00 41 64 64 72  wlp2s0......Addr
[DBUS]   65 73 73 00 01 73 00 00 06 00 00 00 a0 a8 cd 1c  ess..s..........
[DBUS]   7e c9 00 00 00 00 00 00 27 00 00 00 6e 65 74 2e  ~.......'...net.
[DBUS]   63 6f 6e 6e 6d 61 6e 2e 69 77 64 2e 57 69 46 69  connman.iwd.WiFi
[DBUS]   53 69 6d 70 6c 65 43 6f 6e 66 69 67 75 72 61 74  SimpleConfigurat
[DBUS]   69 6f 6e 00 00 00 00 00                          ion.....

src/scan.c:scan_notify() Scan notification 33
[DBUS] disconnect
D-Bus disconnected, quitting... src/eap.c:__eap_method_disable()
src/eap.c:eap_md5_exit()
src/eap-tls.c:eap_tls_exit()
src/eap-ttls.c:eap_ttls_exit()
src/main.c:nl80211_vanished() Lost nl80211 interface src/wsc.c:wsc_exit()
src/scan.c:scan_exit()
src/scan.c:scan_context_free() sc: 0x53f4690 src/netdev.c:netdev_free() Freeing netdev wlp2s0[3] src/netdev.c:netdev_exit() Closing route netlink socket src/wiphy.c:wiphy_free() Freeing wiphy phy0 src/dbus.c:dbus_exit()
==11733==
==11733== HEAP SUMMARY:
==11733== in use at exit: 1,048 bytes in 2 blocks
==11733== total heap usage: 759 allocs, 757 frees, 88,760 bytes allocated
==11733==
==11733== 24 bytes in 1 blocks are still reachable in loss record 1 of 2

==11733==    at 0x4C2C970: malloc (vg_replace_malloc.c:296)
==11733==    by 0x41CA37: l_malloc (util.c:62)
==11733==    by 0x41DE00: l_queue_new (queue.c:63)
==11733==    by 0x422B9D: create_epoll (main.c:102)
==11733==    by 0x422B9D: idle_add (main.c:263)
==11733==    by 0x42361A: l_idle_create (idle.c:108)
==11733==    by 0x4397AF: schedule_emit_signals (dbus-service.c:1081)
==11733==    by 0x43A6EE: _dbus_object_tree_remove_interface
(dbus-service.c:1503)
==11733==    by 0x439F45: check_interface_used (dbus-service.c:1306)
==11733==    by 0x41F342: l_hashmap_foreach (hashmap.c:532)
==11733==    by 0x439FBC: _dbus_object_tree_unregister_interface

(dbus-service.c:1320)
==11733== by 0x42CC6B: l_dbus_unregister_interface (dbus.c:1715)
==11733== by 0x418BE9: wsc_exit (wsc.c:341)
==11733==
==11733== 1,024 bytes in 1 blocks are still reachable in loss record 2 of 2

==11733==    at 0x4C2C970: malloc (vg_replace_malloc.c:296)
==11733==    by 0x422B85: create_epoll (main.c:98)
==11733==    by 0x422B85: idle_add (main.c:263)
==11733==    by 0x42361A: l_idle_create (idle.c:108)
==11733==    by 0x4397AF: schedule_emit_signals (dbus-service.c:1081)
==11733==    by 0x43A6EE: _dbus_object_tree_remove_interface
(dbus-service.c:1503)
==11733==    by 0x439F45: check_interface_used (dbus-service.c:1306)
==11733==    by 0x41F342: l_hashmap_foreach (hashmap.c:532)
==11733==    by 0x439FBC: _dbus_object_tree_unregister_interface
(dbus-service.c:1320)
==11733==    by 0x42CC6B: l_dbus_unregister_interface (dbus.c:1715)
==11733==    by 0x418BE9: wsc_exit (wsc.c:341)
==11733==    by 0x4029D3: nl80211_vanished (main.c:117)
==11733==    by 0x428D49: l_genl_family_unref (genl.c:1057)

==11733==
==11733== LEAK SUMMARY:
==11733== definitely lost: 0 bytes in 0 blocks
==11733== indirectly lost: 0 bytes in 0 blocks
==11733== possibly lost: 0 bytes in 0 blocks
==11733== still reachable: 1,048 bytes in 2 blocks
==11733== suppressed: 0 bytes in 0 blocks

This leak happens in create_epoll(). The sequence of events is a bit bizarre:

1. We send an invalid signal
2. DBus detects that and kills our connection
3. l_main_quit is called, which frees all main event loop resources
4. We start tearing down the rest of the daemon
a) At some point we unregister some interfaces
b) ObjectManager schedules the transmission of a signal
c) idle_create gets called, which calls create_epoll()
5. DBus is destroyed, which removes the idle source in 4c

Unfortunately at this point, nothing actually frees the resources created by create_epoll().

I'm thinking l_main_run needs to be split up into l_main_new, l_main_run and l_main_destroy.

I think I saw this but concluded it will go away when the Address

property format gets changed to be a proper string. One thing that could work to avoid this in the future is to exit from create_epoll if epoll_terminate is true, IIRC there was something else that already

prevented l_main_run from being used multiple times. Also it's a very small leak that only happens on exit...

This leak happens in create_epoll(). The sequence of events is a bit bizarre:

1. We send an invalid signal
2. DBus detects that and kills our connection
3. l_main_quit is called, which frees all main event loop resources
4. We start tearing down the rest of the daemon
a) At some point we unregister some interfaces
b) ObjectManager schedules the transmission of a signal
c) idle_create gets called, which calls create_epoll()
5. DBus is destroyed, which removes the idle source in 4c

Unfortunately at this point, nothing actually frees the resources created by create_epoll().

I'm thinking l_main_run needs to be split up into l_main_new, l_main_run and l_main_destroy.

Maybe this is more of an iwd architecture problem than an ELL problem? The cleanup code runs after l_main_run() returns, and that code should not do anything that creates an idle or watch because those handlers will never get called. The event loop is already gone.

The cleanup should be done before the main loop exits. Instead of calling l_main_quit to kill the daemon, create an iwd_quit function that will run all the teardown code first. Once teardown is complete, then call l_main_quit.

The associated problem is how to give better feedback to all users of ELL, so it's obvious when they're doing something they shouldn't. Changing where create_epoll is called could be a big help here. Rather than doing it automatically in watch_add, idle_add, and l_main_run, we could add an explicit l_main_init() function that must be called before l_main_run and any ELL functions that expect epoll_fd to exist. watch_add and idle_add would return errors if epoll_fd was 0. Yes, this is a lot like l_main_new/run/destroy, but calling it "init" doesn't create the impression that you can have multiple main loops.

Hi Mat,

Quoted 10 lines

Maybe this is more of an iwd architecture problem than an ELL problem? The cleanup code runs after l_main_run() returns, and that code should not do anything that creates an idle or watch because those handlers will never get called. The event loop is already gone.

The cleanup should be done before the main loop exits. Instead of calling l_main_quit to kill the daemon, create an iwd_quit function that will run all the teardown code first. Once teardown is complete, then call l_main_quit.

I've thought of this, but I wasn't really happy doing it that way. The sequence of events would also be quite different from our GLib based projects. If we want to make those projects easy to port to ell, this is not what we want.

However, there's another problem.

Various epoll sources can be created before the main event loop is started. So if we init things, but fail prior to calling l_main_run for whatever reason, then again, epoll resources are not cleaned up.

Quoted 11 lines

The associated problem is how to give better feedback to all users of ELL, so it's obvious when they're doing something they shouldn't. Changing where create_epoll is called could be a big help here. Rather than doing it automatically in watch_add, idle_add, and l_main_run, we could add an explicit l_main_init() function that must be called before l_main_run and any ELL functions that expect epoll_fd to exist. watch_add and idle_add would return errors if epoll_fd was 0. Yes, this is a lot like l_main_new/run/destroy, but calling it "init" doesn't create the impression that you can have multiple main loops.

Which is fine, but then we might as well add l_main_exit().

But if we add those two, how far away are we from supporting multiple event loops?

Quoted 13 lines · Denis Kenzior

Hi Mat,

Quoted 10 lines

Maybe this is more of an iwd architecture problem than an ELL problem? The cleanup code runs after l_main_run() returns, and that code should not do anything that creates an idle or watch because those handlers will never get called. The event loop is already gone.

The cleanup should be done before the main loop exits. Instead of calling l_main_quit to kill the daemon, create an iwd_quit function that will run all the teardown code first. Once teardown is complete, then call l_main_quit.

Quoted 4 lines

I've thought of this, but I wasn't really happy doing it that way. The sequence of events would also be quite different from our GLib based projects. If we want to make those projects easy to port to ell, this is not what we want.

Seems like glib projects would have the same kind of problem, unless there are some restrictions on what happens in the cleanup code.

Sure, I'm sold on needing something to clean up. Note that l_main_exit is already taken.

Quoted 4 lines

But if we add those two, how far away are we from supporting multiple event loops?

This keeps coming up. :) Even if it's a rhetortical question, it's fun to ponder.

Bundling up a bunch of globals in to an l_main struct is simple enough, but every idle and watch-based abstraction (timeouts, signals, ios, ...) would need to be told which event loop to use. I'm currently in the keep-it-simple camp on this one, but the right use case could always justify the feature.