search

swaywm / swayidle

Idle management daemon for Wayland
MIT License
550 stars 50 forks source link

Memory leaks in parse_timeout? #92

Closed nikobockerman closed 3 years ago

nikobockerman commented 3 years ago

Hi,

I tried running swayidle with clang address sanitizer and LeakSanitizer reported the following leaks when I stopped swayidle:

Feb 04 21:33:33 laptop swayidle[44588]: =================================================================
Feb 04 21:33:33 laptop swayidle[44588]: ==44588==ERROR: LeakSanitizer: detected memory leaks
Feb 04 21:33:33 laptop swayidle[44588]: Direct leak of 240 byte(s) in 3 object(s) allocated from:
Feb 04 21:33:33 laptop swayidle[44588]:     #0 0x496782 in calloc (/home/niko/ews/sway-related/swayidle/build/swayidle+0x496782)
Feb 04 21:33:33 laptop swayidle[44588]:     #1 0x7f4fefbbd902  (/usr/lib64/libwayland-client.so.0+0x5902)
Feb 04 21:33:33 laptop swayidle[44588]: Direct leak of 128 byte(s) in 2 object(s) allocated from:
Feb 04 21:33:33 laptop swayidle[44588]:     #0 0x49660d in malloc (/home/niko/ews/sway-related/swayidle/build/swayidle+0x49660d)
Feb 04 21:33:33 laptop swayidle[44588]:     #1 0x7f4fefbabfa8 in wl_event_loop_add_signal (/usr/lib64/libwayland-server.so.0+0xafa8)
Feb 04 21:33:33 laptop swayidle[44588]: Direct leak of 50 byte(s) in 2 object(s) allocated from:
Feb 04 21:33:33 laptop swayidle[44588]:     #0 0x482624 in strdup (/home/niko/ews/sway-related/swayidle/build/swayidle+0x482624)
Feb 04 21:33:33 laptop swayidle[44588]:     #1 0x4c84fe in parse_command /home/niko/ews/sway-related/swayidle/build/../main.c:632:9
Feb 04 21:33:33 laptop swayidle[44588]:     #2 0x4c7e7b in parse_timeout /home/niko/ews/sway-related/swayidle/build/../main.c:670:18
Feb 04 21:33:33 laptop swayidle[44588]:     #3 0x4c70da in load_config /home/niko/ews/sway-related/swayidle/build/../main.c:952:4
Feb 04 21:33:33 laptop swayidle[44588]:     #4 0x4c63df in main /home/niko/ews/sway-related/swayidle/build/../main.c:990:20
Feb 04 21:33:33 laptop swayidle[44588]:     #5 0x7f4fef7b0f1b in __libc_start_main /var/tmp/portage/sys-libs/glibc-2.32-r5/work/glibc-2.32/csu/../csu/libc-start.c:314:16
Feb 04 21:33:33 laptop swayidle[44588]: Direct leak of 27 byte(s) in 1 object(s) allocated from:
Feb 04 21:33:33 laptop swayidle[44588]:     #0 0x482624 in strdup (/home/niko/ews/sway-related/swayidle/build/swayidle+0x482624)
Feb 04 21:33:33 laptop swayidle[44588]:     #1 0x4c84fe in parse_command /home/niko/ews/sway-related/swayidle/build/../main.c:632:9
Feb 04 21:33:33 laptop swayidle[44588]:     #2 0x4c7ee8 in parse_timeout /home/niko/ews/sway-related/swayidle/build/../main.c:675:21
Feb 04 21:33:33 laptop swayidle[44588]:     #3 0x4c70da in load_config /home/niko/ews/sway-related/swayidle/build/../main.c:952:4
Feb 04 21:33:33 laptop swayidle[44588]:     #4 0x4c63df in main /home/niko/ews/sway-related/swayidle/build/../main.c:990:20
Feb 04 21:33:33 laptop swayidle[44588]:     #5 0x7f4fef7b0f1b in __libc_start_main /var/tmp/portage/sys-libs/glibc-2.32-r5/work/glibc-2.32/csu/../csu/libc-start.c:314:16
Feb 04 21:33:33 laptop swayidle[44588]: SUMMARY: AddressSanitizer: 445 byte(s) leaked in 8 allocation(s).

This was from a build against latest master (7dafac7) + cherry-picked commit from #83 (8ccea65f1714e4594c1c488d9f3a10bbb68140bd).

I tried to analyze the two leaks from parse_command() when called from parse_timeout(). However, I fail to understand how that code can work at all as it seems to me that the cmd is not stored anywhere, only the cmd->link member is stored in the state->timeout_cmds list.

Do you consider these real problems? And do you want me to provide some extra information?

I could even try to fix those leaks myself and issue a PR, but I don't understand how the code even works today so can't do that without someone explaining that first :)

nikobockerman commented 3 years ago

I found some documentation for wl_list and now I understand how those linked lists work. I hadn't seen such use of offsetof() but it's quite convinient way to implement doubly-linked list.

I'll prepare a merge request to free() those strings allocated in parse_timeout.

Xyene commented 3 years ago

how that code can work at all as it seems to me that the cmd is not stored anywhere, only the cmd->link member is stored in the state->timeout_cmds list.

This is a common pattern in Wayland (and incidentally kernel) code, see https://wayland.freedesktop.org/docs/html/apb.html#Client-structwl__list.

In fact, I suspect that's what's tripping LeakSanitizer up. LeakSanitizer assumes heap blocks not reachable from the stack at shutdown are leaks, but with the wl_list pattern it's not obvious that swayidle_timeout_cmd *cmd is still referred to by the stack.

Xyene commented 3 years ago

Argh, raced by 12 seconds.

nikobockerman commented 3 years ago

Thanks for the link in any case, it had even better information than what I initially read :+1:

I opened #93 as those two members were never freed, and LeakSanitizer detected that.

After that change, the first two leaks triggered by libwayland-client.so and libwayland-server.so are still present, but those are probably not caused by swayidle.

Xyene commented 3 years ago

Fixed in https://github.com/swaywm/swayidle/commit/f8a3997b7e6f5aff5de0b1f0866dbd000a82421f.