ell/main.c Memory leak

ell/main.c Memory leak

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 4930 bytes --]

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.

Thoughts?

Regards,
-Denis

Re: ell/main.c Memory leak

[Andrzej Zaborowski]

[-- Attachment #1: Type: text/plain, Size: 5591 bytes --]

Hi Denis,

On 3 June 2016 at 23:09, Denis Kenzior <denkenz@gmail.com> wrote:
> 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...

Best regards

Re: ell/main.c Memory leak

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 669 bytes --]

Hi Andrew,

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

It will, and this one is rather benign, but I want to run have clean 
valgrind runs...

> 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

Yes, this is another possibility.  Prevent various main event loops 
elements from being created if l_main_quit has been called...

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

Regards,
-Denis

Re: ell/main.c Memory leak

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 1945 bytes --]

On Fri, 3 Jun 2016, Denis Kenzior wrote:

> 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.

--
Mat Martineau
Intel OTC

Re: ell/main.c Memory leak

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 1844 bytes --]

Hi Mat,

 >
> 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.

>
> 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?

Regards,
-Denis

Re: ell/main.c Memory leak

[Mat Martineau]

[-- Attachment #1: Type: text/plain, Size: 2625 bytes --]

On Fri, 3 Jun 2016, Denis Kenzior wrote:

> Hi Mat,
>
>>
>> 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.

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

>
> 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.

True, I didn't consider that.

>
>> 
>> 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().

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

>
> 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.

--
Mat Martineau
Intel OTC

Re: ell/main.c Memory leak

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 2208 bytes --]

Hi Mat,

On 06/03/2016 06:17 PM, Mat Martineau wrote:
> On Fri, 3 Jun 2016, Denis Kenzior wrote:
>
>> Hi Mat,
>>
>>>
>>> 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.
>
> Seems like glib projects would have the same kind of problem, unless
> there are some restrictions on what happens in the cleanup code.
>

I don't recall ever having such issues.  I think glib allows loops to be 
restarted after main_loop_quit.  We could as well.

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

Which no one uses, so we can hi-jack that one.

>
>>
>> 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.
>

Yes, but all of this can be hidden away from the API level.  First main 
event loop that is created becomes the default.  Then subsequent calls 
to l_idle_add, etc will always use the default main event loop.

If someone *actually* wants to use multiple event loops in the future, 
we can add API extensions to handle that.

Regards,
-Denis