battstat: call up_client_get_devices once to avoid mem leak #443

alwilson · 2020-02-26T15:53:00Z

This attempts to avoid a memory leak when calling
up_client_get_devices() from libupower-glib by only calling it once
in battstat_upower_initialise() and reusing that GPtrArray later in
battstat_upower_get_battery_info().

This leak was discovered by running strace for mmap and brk syscalls.
No calls to mmap were made, but the brk could caught and backtraced
over and over again to battstat_upower_get_battery_info().

There may be some issues with UP_CHECK_VERSION (0, 99, 0) checks that
need to be addressed here that are made in battstat_upower_initialise(),
but not in battstat_upower_get_battery_info().

See 'Memory leak of battstat-applet #345' for more details:
#345

This attempts to avoid a memory leak when calling up_client_get_devices() from libupower-glib by only calling it once in battstat_upower_initialise() and reusing that GPtrArray later in battstat_upower_get_battery_info(). This leak was discovered by running strace for mmap and brk syscalls. No calls to mmap were made, but the brk could caught and backtraced over and over again to battstat_upower_get_battery_info(). There may be some issues with UP_CHECK_VERSION (0, 99, 0) checks that need to be addressed here that are made in battstat_upower_initialise(), but not in battstat_upower_get_battery_info(). See 'Memory leak of battstat-applet mate-desktop#345' for more details: mate-desktop#345

battstat/battstat-upower.c

AlexTalker · 2020-02-26T16:00:22Z

battstat/battstat-upower.c

@@ -83,9 +85,6 @@ battstat_upower_initialise (void (*callback) (void))
  int i, num;

  status_updated_callback = callback;
-#if UP_CHECK_VERSION (0, 99, 0)


If anyone wonders, removing #if is fine as the actual application for this happens below.
Sharing the variable between functions would lead to eventual utilization of up_client_get_devices unconditionally anyway.

battstat/battstat-upower.c

muktupavels · 2020-02-26T16:02:03Z

https://gitlab.freedesktop.org/upower/upower/-/blob/master/libupower-glib/up-client.c#L78

rbuj · 2020-02-26T16:33:18Z

upower 0.99.8 was released on 2018-06-19. I wonder if we can bump to upower 0.99.8 and use up_client_get_devices2/g_ptr_array_unref instead of using dreprecated up_client_get_devices.

AlexTalker · 2020-02-26T16:34:55Z

@rbuj More #if?

rbuj · 2020-02-26T16:38:29Z

set UPOWER_REQUIRED=0.99.8 on configure.ac.
https://github.com/mate-desktop/mate-applets/blob/master/configure.ac#L21

remove if UP_CHECK_VERSION (0, 99, 0)

$ git grep -H UP_CHECK_VERSION
battstat/battstat-upower.c:#if UP_CHECK_VERSION (0, 99, 0)
battstat/battstat-upower.c:#if UP_CHECK_VERSION (0, 99, 0)
battstat/battstat-upower.c:#if UP_CHECK_VERSION(0, 99, 0)
battstat/battstat-upower.c:#if UP_CHECK_VERSION(0, 99, 0)

AlexTalker · 2020-02-26T16:38:27Z

battstat/battstat-upower.c

@@ -133,6 +131,7 @@ battstat_upower_cleanup( void )
  if( upc == NULL )
    return;

+  g_object_unref( devices );


I'd re-do the code here to put unref calls into if (ptr != NULL) ... unref()
Besides, call g_ptr_array_unref for safety. Hell knows how GTK works.

This is my first time touching GTK, so I'll take your word on that. 😋

This is my first time reviewing GTK, so best of luck.
Hell, all the code seems the same after the years.

You must be glad that I at least have learned some C

AlexTalker · 2020-02-26T16:39:12Z

@rbuj No backward compatibility then?

rbuj · 2020-02-26T16:40:30Z

upower 0.9.4 was released 10 years ago. https://cgit.freedesktop.org/upower/refs/tags

AlexTalker · 2020-02-26T16:43:14Z

@rbuj and CentOS(read: RedHat) DOWNGRADED nvme-cli package ( see https://bugs.centos.org/view.php?id=16516 for yourself ),
you never can be sure which way these sick folks go.

muktupavels · 2020-02-26T16:45:32Z

As documentation says to call ’g_object_unref’ simplest fix might be to just set free function on array.

…

On Wed, Feb 26, 2020, 18:40 Robert Antoni Buj Gelonch < ***@***.***> wrote: upower 0.9.4 was released 10 years ago. https://cgit.freedesktop.org/upower/refs/tags — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#443?email_source=notifications&email_token=AAJ3BAIACSRCWIAVU6HTPG3RE2LP7A5CNFSM4K4IRWFKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENA62CA#issuecomment-591523080>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAJ3BAKW62B5Z5C3BYLZONLRE2LP7ANCNFSM4K4IRWFA> .

AlexTalker · 2020-02-26T16:47:53Z

@muktupavels Which is set to NULL by the library(for now), so I think better to keep things that worked before.

alwilson · 2020-02-26T17:11:22Z

@rbuj I assume you have the final say on what gets committed then, right? I'm not entirely sure who the maintainers are here. You would like me to up the upower version and use the non-deprecated calls and still keep a single call to up_client_get_devices2() in the initialization.

…wer version checks

rbuj · 2020-02-26T17:22:39Z

Let's see what the other members think about using up_client_get_devices2 (bumping to upower 0.99.8) instead of up_client_get_devices, which does not have a free_func.
https://gitlab.freedesktop.org/upower/upower/-/blob/master/libupower-glib/up-client.c#L94

alwilson · 2020-02-26T17:26:19Z

I wrote up a quick test of both up_client_get_devices{,2} functions in an endless loop independent of mate-applets and they still leak memory. Although that's not as much of a concern now given the change to only call it once.

alwilson · 2020-02-26T18:05:34Z

Arch linux build didn't pass b/c the docker image was missing the autoconf-archive package.

muktupavels · 2020-02-26T18:36:43Z

First, this seems wrong to me. UPower has signals - devices-added and devices-removed... Don't now when/how this affects real usage, but to me looks like this will cause regression...

Look at this merge request and/or related commit:
https://gitlab.freedesktop.org/upower/upower/-/merge_requests/1
https://gitlab.freedesktop.org/upower/upower/-/commit/cb1071b9ab6bf743d7c51545841dfadcbfe7ec9c

It looks like you have always needed to do something like this:

for (i = 0; i < devices->len; i++)
  {
    UpDevice *upd = g_ptr_array_index (devices, i);
    g_object_unref (upd);
  }
g_ptr_array_unref (devices);

Since devices are unrefed in multiple places it is easier to do g_ptr_array_set_free_func (devices, (GDestroyNotify) g_object_unref) after up_client_get_devices. Other option is to create function with above code and use it in place of g_ptr_array_unref. Or just use up_client_get_devices2 where g_ptr_array_unref will do needed work.

alwilson · 2020-02-26T19:34:17Z

@muktupavels Hmm, I tried cycling my laptop with the power cord and the charging indicator from the applet no longer works. I think you're right that calling up_client_get_devices just once means we don't pick up those changes. I don't think this patch set is the answer since it breaks battery charging detecting and my laptop has 2 batteries, one removable, and it's failing to detect those changes too. So are we stuck with this memory leak then? This patch set is a regression as is.

My little toy upower program still sees memory leaks with your proposed changes. I'll keep playing around with it since I'm not sure I'm completely understand how glib memory management works here. I'm beginning to suspect libupower_glib of being the culprit though b/c it should be possible to call either of these functions without memory leaks. Maybe the dbus functions in the brk backtrace are to blame? Still need to figure out if this is an arch linux or config specific leak.

// gcc $(pkg-config --cflags upower-glib) upower_test.c -o upower_test $(pkg-config --libs upower-glib)

#include <upower.h>

int main(int argc, char **argv)
{
	int i;
	UpClient *upc;
	GPtrArray *devices;

	upc = up_client_new();

	while (1) {
		//devices = up_client_get_devices( upc );
		devices = up_client_get_devices2( upc );
		for (i = 0; i < devices->len; i++) {
			UpDevice *upd = g_ptr_array_index (devices, i);
			// g_object_unref (upd);
		}
		//g_object_unref( devices );
		g_ptr_array_unref( devices );
	}

	return 0;
}

muktupavels · 2020-02-26T20:06:01Z

If that leaks then memory leak is in upower.

alwilson · 2020-02-26T20:20:41Z

Sorry for the noise, but it turns out that battstat has additional bugs in general around unpluging batteries. I thought this changeset broke when cycling the power plug, but that was because I removed a battery. Removing a battery while battstat is running will screw up battstat with or without these changes. Without screwing around with the batteries this pachset will still update the charging icon. I guess I'll take a look at mate-power-manager b/c it seems to do all the right things. Is it worth fixing this memory leak if there's already a mate applet that does the same thing?

alwilson · 2020-02-26T20:46:54Z

Confirmed, mate-power-manager is doing essentially what this applet wants to be doing. It calls up_client_get_devices2() once then it has callbacks for devices being removed, added, or changed. I think fixing battstat would mean duplicating code from an existing app.

Here is the core of what battstat should probably be doing (initialization with proper callbacks):
https://github.com/mate-desktop/mate-power-manager/blob/master/src/gpm-engine.c#L539

muktupavels · 2020-02-26T22:54:28Z

Your testcase might be wrong... Seems there is something that requires GMainLoop running to properly free resources. At least adding main loop I no longer see huge leaks. So up_client_get_devices2() or g_ptr_array_set_free_func() should work.

alwilson · 2020-02-27T02:35:03Z

@muktupavels I tried out adding GMainLoop to my testcase and it appears to stops the "memory leak". Well, that means adding this somewhere higher up in battstat_applet.c would be sufficient. I can make another pull request adding just the GMainLoop calls since that appears to be the better solution here. I think addressing updates to upower is useful at some point, but after seeing mate-power-manager's source I would think deprecating this applet and migrating missing features would make better sense long term.

GMainLoop *main_loop = g_main_loop_new(NULL, FALSE);
g_main_loop_run(main_loop);

muktupavels · 2020-02-27T12:35:36Z

You don't need to add main loop in applet... It was only about your test case, applet / panel already has main loop!

You still have to fix leak here by adding free function or switching to get_devices2.

raveit65 · 2020-07-14T16:52:38Z

I just fixed the deprecation and the leak in upower.
Also, required version of glib-upower is bumped and legacy code is droped.
See 06feef7
f7a5d24

lukefromdc · 2020-12-25T00:36:01Z

Any progress on this? Doesn't look like something I can properly test on the desktop except for build and not crashing on adding to a panel

AlexTalker suggested changes Feb 26, 2020

View reviewed changes

battstat: remove whitespace noise; free devices ptr in cleanup func

a73baf2

AlexTalker suggested changes Feb 26, 2020

View reviewed changes

battstat: use more specific unref for devices ptr; rearrange cleanup

591ebcc

battstat: bump upower version; update depricated funcs and remove upo…

135c7c4

…wer version checks

rbuj requested a review from a team February 26, 2020 17:22

add autoconf-archive pkg to arch build config

be43872

rbuj mentioned this pull request Jul 14, 2020

battstat: fix deprecated up_client_get_devices #487

Merged

rbuj force-pushed the master branch from 2a85880 to 6f974ba Compare December 22, 2020 10:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

battstat: call up_client_get_devices once to avoid mem leak #443

battstat: call up_client_get_devices once to avoid mem leak #443

alwilson commented Feb 26, 2020

AlexTalker Feb 26, 2020

muktupavels commented Feb 26, 2020

rbuj commented Feb 26, 2020

AlexTalker commented Feb 26, 2020

rbuj commented Feb 26, 2020

AlexTalker Feb 26, 2020

alwilson Feb 26, 2020

AlexTalker Feb 26, 2020

AlexTalker Feb 26, 2020

AlexTalker commented Feb 26, 2020

rbuj commented Feb 26, 2020

AlexTalker commented Feb 26, 2020

muktupavels commented Feb 26, 2020 via email

AlexTalker commented Feb 26, 2020

alwilson commented Feb 26, 2020

rbuj commented Feb 26, 2020

alwilson commented Feb 26, 2020

alwilson commented Feb 26, 2020

muktupavels commented Feb 26, 2020

alwilson commented Feb 26, 2020

muktupavels commented Feb 26, 2020

alwilson commented Feb 26, 2020

alwilson commented Feb 26, 2020

muktupavels commented Feb 26, 2020

alwilson commented Feb 27, 2020

muktupavels commented Feb 27, 2020

raveit65 commented Jul 14, 2020 •

edited

Loading

lukefromdc commented Dec 25, 2020

battstat: call up_client_get_devices once to avoid mem leak #443

Are you sure you want to change the base?

battstat: call up_client_get_devices once to avoid mem leak #443

Conversation

alwilson commented Feb 26, 2020

AlexTalker Feb 26, 2020

Choose a reason for hiding this comment

muktupavels commented Feb 26, 2020

rbuj commented Feb 26, 2020

AlexTalker commented Feb 26, 2020

rbuj commented Feb 26, 2020

AlexTalker Feb 26, 2020

Choose a reason for hiding this comment

alwilson Feb 26, 2020

Choose a reason for hiding this comment

AlexTalker Feb 26, 2020

Choose a reason for hiding this comment

AlexTalker Feb 26, 2020

Choose a reason for hiding this comment

AlexTalker commented Feb 26, 2020

rbuj commented Feb 26, 2020

AlexTalker commented Feb 26, 2020

muktupavels commented Feb 26, 2020 via email

AlexTalker commented Feb 26, 2020

alwilson commented Feb 26, 2020

rbuj commented Feb 26, 2020

alwilson commented Feb 26, 2020

alwilson commented Feb 26, 2020

muktupavels commented Feb 26, 2020

alwilson commented Feb 26, 2020

muktupavels commented Feb 26, 2020

alwilson commented Feb 26, 2020

alwilson commented Feb 26, 2020

muktupavels commented Feb 26, 2020

alwilson commented Feb 27, 2020

muktupavels commented Feb 27, 2020

raveit65 commented Jul 14, 2020 • edited Loading

lukefromdc commented Dec 25, 2020

raveit65 commented Jul 14, 2020 •

edited

Loading