Bug#816872: wmbattery: memory leak in wmbattery
Control: tags -1 pending On Mon, 6 Nov 2017 09:58:29 -0500 Doug Torrance wrote: > Wonderful, thank you! I've submitted the patch upstream and hope to > release a new Debian version soon. FYI, a new upstream version containing this patch (2.51) has been released: http://www.dockapps.net/wmbattery The new Debian package is awaiting sponsorship. Doug
Bug#816872: wmbattery: memory leak in wmbattery
On 11/06/2017 09:32 AM, David Johnson wrote: FYI: I've been running with this patch applied for over a week, no issues seen. Please apply unless there is an alternative. David Johnson writes: I've concluded libupower-glib is just super leaky. The below patch moves the upower API calls into a child process that doesn't exist for long to keep the leaks out of the main process. Tested against stretch versions, looks good. Wonderful, thank you! I've submitted the patch upstream and hope to release a new Debian version soon. Doug
Bug#816872: wmbattery: memory leak in wmbattery
FYI: I've been running with this patch applied for over a week, no issues seen. Please apply unless there is an alternative. David Johnson writes: > > I've concluded libupower-glib is just super leaky. The below patch > moves the upower API calls into a child process that doesn't exist for > long to keep the leaks out of the main process. > > Tested against stretch versions, looks good. >
Bug#816872: wmbattery: memory leak in wmbattery
I've concluded libupower-glib is just super leaky. The below patch
moves the upower API calls into a child process that doesn't exist for
long to keep the leaks out of the main process.
Tested against stretch versions, looks good.
--- wmbattery-2.50.orig/upower.c2015-08-30 19:58:13.0 -0400
+++ wmbattery-2.50/upower.c 2017-10-24 15:19:46.474102031 -0400
@@ -6,13 +6,15 @@
#include
#include
#include
+#include
+#include
+#include
+#include
#include
#include "apm.h"
#define MAX_RETRIES 3
-static UpClient * up;
-
struct context {
int current;
int needed;
@@ -54,29 +56,11 @@
}
}
-int upower_supported(void)
-{
- up = up_client_new();
-
- if (!up) {
- return 0;
- } else {
- GPtrArray *devices = up_client_get_devices(up);
-
- if (!devices) {
- return 0;
- } else {
- g_ptr_array_unref(devices);
- return 1;
- }
- }
-}
-
/* Fill the passed apm_info struct. */
-int upower_read(int battery, apm_info *info)
+static int upower_read_child(int battery, apm_info *info)
{
+ UpClient * up;
GPtrArray *devices = NULL;
- static int retries = 0;
up = up_client_new();
@@ -90,15 +74,9 @@
devices = up_client_get_devices(up);
- if (!devices) {
- retries++;
- if (retries < MAX_RETRIES)
- return 0; /* fine immediately after hibernation */
- else
- return -1;
- }
+ if (!devices)
+ return -1;
- retries = 0;
info->battery_flags = 0;
info->using_minutes = 0;
@@ -143,6 +121,92 @@
info->battery_status = BATTERY_STATUS_ABSENT;
}
- g_ptr_array_free(devices, TRUE);
+ g_ptr_array_unref(devices);
+ return 0;
+}
+
+static int upower_read_work(int battery, apm_info *info)
+{
+ int sp[2];
+ int child;
+ int status;
+ ssize_t cv;
+
+ if (socketpair(AF_UNIX, SOCK_DGRAM, 0, sp) < 0) {
+ fprintf(stderr, "socketpair: %s", strerror(errno));
+ goto fail;
+ }
+
+ child = fork();
+ if (child < 0) {
+ fprintf(stderr, "fork: %s", strerror(errno));
+ goto fail_close;
+ }
+
+ if (child == 0) {
+ /* child process does work, writes failure or result back to
parent */
+ close(sp[0]);
+ status = upower_read_child(battery, info);
+ if (status < 0) {
+ cv = send(sp[1], , sizeof(status), 0);
+ }
+ else {
+ cv = send(sp[1], info, sizeof(*info), 0);
+ }
+ exit(cv < 0);
+ }
+ close(sp[1]);
+
+ child = waitpid(child, , 0);
+ if (child < 0) {
+ fprintf(stderr, "waitpid: %s status=%d", strerror(errno),
status);
+ goto fail_close0;
+ }
+
+ cv = recv(sp[0], info, sizeof(*info), 0);
+ if (cv < 0) {
+ fprintf(stderr, "recv: %s", strerror(errno));
+ goto fail_close0;
+ }
+ else if (cv == sizeof(status)) {
+ memcpy(, info, sizeof(status));
+ goto fail_close0;
+ }
+ else if (cv != sizeof(*info)) {
+ fprintf(stderr, "recv: unexpected size %d", cv);
+ goto fail_close0;
+ }
+
+ close(sp[0]);
+ return 0;
+
+ fail_close:
+ close(sp[1]);
+ fail_close0:
+ close(sp[0]);
+ fail:
+ return -1;
+}
+
+int upower_supported(void)
+{
+ apm_info info;
+ return !(upower_read_work(1, ) < 0);
+}
+
+
+int upower_read(int battery, apm_info *info)
+{
+ static int retries = 0;
+
+ if (upower_read_work(battery, info) < 0) {
+ retries++;
+ if (retries < MAX_RETRIES)
+ return 0; /* fine immediately after hibernation */
+ else
+ return -1;
+ }
+
+ retries = 0;
return 0;
}
Bug#816872: wmbattery: memory leak in wmbattery
With the patch provided to remove the extra up_client_new() calls, I still see these 3 leaks from within up_client_get_devices(). Neither g_ptr_array_free() or g_ptr_array_unref() are cleaning them up. I'm assuming a more comprehensive fix would be to remove up_client_get_devices() from upower_read() since it seems to be internally leaking in libupower-glib and save the data from the call in upower_supported() for each polling cycle instead of discarding it each time. ==17098== ==17098== 5,968 bytes in 746 blocks are possibly lost in loss record 1,557 of 1,606 ==17098==at 0x4238A0E: g_type_create_instance (in /usr/lib/i386-linux-gnu/libgobject-2.0.so.0.4200.1) ==17098==by 0x421A9D5: g_object_new_internal (in /usr/lib/i386-linux-gnu/libgobject-2.0.so.0.4200.1) ==17098==by 0x421C4A5: g_object_newv (in /usr/lib/i386-linux-gnu/libgobject-2.0.so.0.4200.1) ==17098==by 0x421CACC: g_object_new (in /usr/lib/i386-linux-gnu/libgobject-2.0.so.0.4200.1) ==17098==by 0x41EDB06: up_device_new (in /usr/lib/i386-linux-gnu/libupower-glib.so.3.0.0) ==17098==by 0x41E8FC6: up_client_get_devices (in /usr/lib/i386-linux-gnu/libupower-glib.so.3.0.0) ==17098==by 0x804BE5A: upower_read (upower.c:89) ==17098==by 0x804A996: alarmhandler (wmbattery.c:711) ==17098==by 0x43BD3F7: ??? (in /lib/i386-linux-gnu/i686/cmov/libc-2.19.so) ==17098== ==17098== 14,920 bytes in 746 blocks are possibly lost in loss record 1,566 of 1,606 ==17098==at 0x42389C2: g_type_create_instance (in /usr/lib/i386-linux-gnu/libgobject-2.0.so.0.4200.1) ==17098==by 0x421A9D5: g_object_new_internal (in /usr/lib/i386-linux-gnu/libgobject-2.0.so.0.4200.1) ==17098==by 0x421C4A5: g_object_newv (in /usr/lib/i386-linux-gnu/libgobject-2.0.so.0.4200.1) ==17098==by 0x421CACC: g_object_new (in /usr/lib/i386-linux-gnu/libgobject-2.0.so.0.4200.1) ==17098==by 0x41EDB06: up_device_new (in /usr/lib/i386-linux-gnu/libupower-glib.so.3.0.0) ==17098==by 0x41E8FC6: up_client_get_devices (in /usr/lib/i386-linux-gnu/libupower-glib.so.3.0.0) ==17098==by 0x804BE5A: upower_read (upower.c:89) ==17098==by 0x804A996: alarmhandler (wmbattery.c:711) ==17098==by 0x43BD3F7: ??? (in /lib/i386-linux-gnu/i686/cmov/libc-2.19.so) ==17098== ==17098== 26,856 bytes in 746 blocks are possibly lost in loss record 1,584 of 1,606 ==17098==at 0x402C0D5: calloc (vg_replace_malloc.c:623) ==17098==by 0x42B3745: g_malloc0 (in /lib/i386-linux-gnu/libglib-2.0.so.0.4200.1) ==17098==by 0x42148C4: g_closure_new_simple (in /usr/lib/i386-linux-gnu/libgobject-2.0.so.0.4200.1) ==17098==by 0x4215C28: g_cclosure_new (in /usr/lib/i386-linux-gnu/libgobject-2.0.so.0.4200.1) ==17098==by 0x422DF1C: g_signal_connect_data (in /usr/lib/i386-linux-gnu/libgobject-2.0.so.0.4200.1) ==17098==by 0x41ECAB2: up_device_set_object_path_sync (in /usr/lib/i386-linux-gnu/libupower-glib.so.3.0.0) ==17098==by 0x41E8FD3: up_client_get_devices (in /usr/lib/i386-linux-gnu/libupower-glib.so.3.0.0) ==17098==by 0x804BE5A: upower_read (upower.c:89) ==17098==by 0x804A996: alarmhandler (wmbattery.c:711) ==17098==by 0x43BD3F7: ??? (in /lib/i386-linux-gnu/i686/cmov/libc-2.19.so) ==17098== -- David Johnson Principal Engineer
Bug#816872: wmbattery: memory leak in wmbattery
Doug, When I first filed this bug I did some digging with valgrind trying to find the leaks. I found several issues with the upower module (the older methonds were less leaky). valgrind found lots of leaks, but they were hard to trace, except that most/all were in the upower area. I suspect that some of the leaks are in libupower not wmbattery, however it wasn't clear to me if it was wmbattery's usage of libupower or libupower itself. I did find the following improved the leaks, however it was still leaking, just not as bad. Also the rate of leaks is directly linked to the "-w" option, the faster the refresh the faster the leaking. Hope this helps: diff -Nuar wmbattery-2.50.orig/upower.c wmbattery-2.50/upower.c --- wmbattery-2.50.orig/upower.c2015-08-30 19:58:13.0 -0400 +++ wmbattery-2.50/upower.c 2016-03-20 16:08:57.177709309 -0400 @@ -78,8 +78,6 @@ GPtrArray *devices = NULL; static int retries = 0; - up = up_client_new(); - if (!up) return -1; @@ -143,6 +141,6 @@ info->battery_status = BATTERY_STATUS_ABSENT; } - g_ptr_array_free(devices, TRUE); + g_ptr_array_unref(devices); return 0; } -- David Johnson Principal Engineer
Bug#816872: wmbattery: memory leak in wmbattery
On 10/19/2017 12:43 PM, bw wrote: On Thu, 19 Oct 2017, Tim Connors wrote: Package: wmbattery Version: 2.50-1+b1 Followup-For: Bug #816872 Dear Maintainer, Memory bug is still present, nice app though overall, thanks. Hi, this just killed my system (13GB RSS, 20GB VMM). Oct 19 22:46:35 dirac kernel: [163443.300793] Killed process 12027 (wmbattery) total-vm:20361464kB, anon-rss:12105144kB, file-rss:728kB, shmem-rss:0kB Since there's no activity on this bug since March 2016, could the maintainers please arrange for this to be removed from debian? -- Tim Connors ___ Pkg-wmaker-devel mailing list pkg-wmaker-de...@lists.alioth.debian.org https://lists.alioth.debian.org/mailman/listinfo/pkg-wmaker-devel My C is very rusty so I didn't look into this much. I found package wmacpi that does a good job at monitoring the battery and doesn't have the same issue. Looking at the source code for wmbattery, In the file acpi.c there is a function called find_items() that allocates memory for a list of devices like this: char **devices = malloc(ACPI_MAXITEM * sizeof(char *)); This works out to 8 * 8 on amd64 or 64 pointers to contain the list. During the while loop later, it looks like all the acpi device filenames are read into this array of pointers and assigned memory by the strdup() call. I'm not sure what dir it's looking at, but on my system, there are more than 64 acpi devices... so this would seem problematic if the way I understand it is correct. b17@themini:/sys/bus/acpi/devices$ ls | wc -l 70 It's a nice looking app, but yeah if this bug can't be found and fixed I agree it should be assigned grave importance. Thanks for the reminder about this bug and the investigative work! I'm both the Debian maintainer and primary upstream maintainer. I'm pretty busy at work for the next week or so, but I'll try and take a look at it before too long. Doug
Bug#816872: wmbattery: memory leak in wmbattery
> Package: wmbattery > Version: 2.50-1+b1 > Followup-For: Bug #816872 > > Dear Maintainer, > > Memory bug is still present, nice app though overall, thanks. Hi, this just killed my system (13GB RSS, 20GB VMM). Oct 19 22:46:35 dirac kernel: [163443.300793] Killed process 12027 (wmbattery) total-vm:20361464kB, anon-rss:12105144kB, file-rss:728kB, shmem-rss:0kB Since there's no activity on this bug since March 2016, could the maintainers please arrange for this to be removed from debian? -- Tim Connors
Bug#816872: wmbattery: memory leak in wmbattery
Package: wmbattery Version: 2.50-1+b1 Followup-For: Bug #816872 Dear Maintainer, Memory bug is still present, nice app though overall, thanks. -- System Information: Debian Release: 9.1 APT prefers stable APT policy: (500, 'stable') Architecture: amd64 (x86_64) Kernel: Linux 4.9.0-3-amd64 (SMP w/2 CPU cores) Locale: LANG=en_US.UTF-8, LC_CTYPE=en_US.UTF-8 (charmap=UTF-8), LANGUAGE=en_US.UTF-8 (charmap=UTF-8) Shell: /bin/sh linked to /bin/dash Init: systemd (via /run/systemd/system) Versions of packages wmbattery depends on: ii libapm1 3.2.2-15+b2 ii libc62.24-11+deb9u1 ii libglib2.0-0 2.50.3-2 ii libupower-glib3 0.99.4-4+b1 ii libx11-6 2:1.6.4-3 ii libxext6 2:1.3.3-1+b2 ii libxpm4 1:3.5.12-1 ii upower 0.99.4-4+b1 wmbattery recommends no packages. Versions of packages wmbattery suggests: pn wmaker -- no debconf information
Bug#816872: wmbattery: memory leak in wmbattery
Control: forwarded -1 wmaker-...@lists.windowmaker.org On 03/05/2016 10:24 PM, Dave Johnson wrote: Package: wmbattery Version: 2.45-2 Severity: important Dear Maintainer, wmbattery appears to have a memory leak. When invoked with "wmbattery -w 2" it is leaking approx 350KB/minute on my system. Eventually it runs out of memory and is killed by kernel. $ while sleep 300; do ps u $(pidof wmbattery); done USER PID %CPU %MEMVSZ RSS TTY STAT START TIME COMMAND davijoh3 29894 0.2 0.0 18524 6792 ?Sl 21:24 0:00 wmbattery -w 2 USER PID %CPU %MEMVSZ RSS TTY STAT START TIME COMMAND davijoh3 29894 0.2 0.1 20068 7976 ?Sl 21:24 0:01 wmbattery -w 2 USER PID %CPU %MEMVSZ RSS TTY STAT START TIME COMMAND davijoh3 29894 0.2 0.1 21604 9404 ?Sl 21:24 0:01 wmbattery -w 2 USER PID %CPU %MEMVSZ RSS TTY STAT START TIME COMMAND davijoh3 29894 0.2 0.1 23160 10588 ?Sl 21:24 0:02 wmbattery -w 2 USER PID %CPU %MEMVSZ RSS TTY STAT START TIME COMMAND davijoh3 29894 0.2 0.1 24696 12208 ?Sl 21:24 0:03 wmbattery -w 2 USER PID %CPU %MEMVSZ RSS TTY STAT START TIME COMMAND davijoh3 29894 0.2 0.1 26164 14032 ?Sl 21:24 0:03 wmbattery -w 2 USER PID %CPU %MEMVSZ RSS TTY STAT START TIME COMMAND davijoh3 29894 0.2 0.1 27700 15464 ?Sl 21:24 0:04 wmbattery -w 2 USER PID %CPU %MEMVSZ RSS TTY STAT START TIME COMMAND davijoh3 29894 0.2 0.2 29644 17240 ?Sl 21:24 0:05 wmbattery -w 2 USER PID %CPU %MEMVSZ RSS TTY STAT START TIME COMMAND davijoh3 29894 0.2 0.2 31180 18964 ?Sl 21:24 0:05 wmbattery -w 2 Thanks for your report! I'll look into it. (Also forwarding upstream.)
Bug#816872: wmbattery: memory leak in wmbattery
Package: wmbattery Version: 2.45-2 Severity: important Dear Maintainer, wmbattery appears to have a memory leak. When invoked with "wmbattery -w 2" it is leaking approx 350KB/minute on my system. Eventually it runs out of memory and is killed by kernel. $ while sleep 300; do ps u $(pidof wmbattery); done USER PID %CPU %MEMVSZ RSS TTY STAT START TIME COMMAND davijoh3 29894 0.2 0.0 18524 6792 ?Sl 21:24 0:00 wmbattery -w 2 USER PID %CPU %MEMVSZ RSS TTY STAT START TIME COMMAND davijoh3 29894 0.2 0.1 20068 7976 ?Sl 21:24 0:01 wmbattery -w 2 USER PID %CPU %MEMVSZ RSS TTY STAT START TIME COMMAND davijoh3 29894 0.2 0.1 21604 9404 ?Sl 21:24 0:01 wmbattery -w 2 USER PID %CPU %MEMVSZ RSS TTY STAT START TIME COMMAND davijoh3 29894 0.2 0.1 23160 10588 ?Sl 21:24 0:02 wmbattery -w 2 USER PID %CPU %MEMVSZ RSS TTY STAT START TIME COMMAND davijoh3 29894 0.2 0.1 24696 12208 ?Sl 21:24 0:03 wmbattery -w 2 USER PID %CPU %MEMVSZ RSS TTY STAT START TIME COMMAND davijoh3 29894 0.2 0.1 26164 14032 ?Sl 21:24 0:03 wmbattery -w 2 USER PID %CPU %MEMVSZ RSS TTY STAT START TIME COMMAND davijoh3 29894 0.2 0.1 27700 15464 ?Sl 21:24 0:04 wmbattery -w 2 USER PID %CPU %MEMVSZ RSS TTY STAT START TIME COMMAND davijoh3 29894 0.2 0.2 29644 17240 ?Sl 21:24 0:05 wmbattery -w 2 USER PID %CPU %MEMVSZ RSS TTY STAT START TIME COMMAND davijoh3 29894 0.2 0.2 31180 18964 ?Sl 21:24 0:05 wmbattery -w 2 -- System Information: Debian Release: 8.3 APT prefers stable APT policy: (500, 'stable') Architecture: i386 (x86_64) Kernel: Linux 3.16.0-4-amd64 (SMP w/4 CPU cores) Locale: LANG=en_US.UTF-8, LC_CTYPE=en_US.UTF-8 (charmap=UTF-8) Shell: /bin/sh linked to /bin/dash Init: systemd (via /run/systemd/system) Versions of packages wmbattery depends on: ii libapm1 3.2.2-15 ii libc62.19-18+deb8u3 ii libglib2.0-0 2.42.1-1 ii libupower-glib3 0.99.1-3.2 ii libx11-6 2:1.6.2-3 ii libxext6 2:1.3.3-1 ii libxpm4 1:3.5.11-1+b1 ii upower 0.99.1-3.2 wmbattery recommends no packages. Versions of packages wmbattery suggests: ii wmaker 0.95.5-2+b2 -- no debconf information
