Bug 107320 - Valgrind reports a couple of memory leaks
Summary: Valgrind reports a couple of memory leaks
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: 1.12
Hardware: All Linux (All)
: low minor
Assignee: Simon McVittie
QA Contact: D-Bus Maintainers
URL:
Whiteboard: review+
Keywords: patch
Depends on:
Blocks:
 
Reported: 2018-07-21 07:22 UTC by Evgeny Vereshchagin
Modified: 2018-08-31 15:29 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
bus: Free address (from --address) when we have finished using it (848 bytes, patch)
2018-08-17 17:01 UTC, Simon McVittie
Details | Splinter Review
server-unix: Don't leak address of systemd server on success (793 bytes, patch)
2018-08-17 17:03 UTC, Simon McVittie
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Evgeny Vereshchagin 2018-07-21 07:22:51 UTC
These memory leaks were originally reported in https://github.com/systemd/systemd/issues/5004#issuecomment-405113892 and then I was asked to create an issue here. Below is the output of Valgrind which show two minor memory leaks:

==27751== Command: /usr/bin/dbus-daemon --system --address=systemd: --nofork --nopidfile --systemd-activation --syslog-only
==27751==
==27751==
==27751== HEAP SUMMARY:
==27751==     in use at exit: 81,327 bytes in 223 blocks
==27751==   total heap usage: 7,411 allocs, 7,188 frees, 967,700 bytes allocated
==27751==
==27751== 16 bytes in 1 blocks are definitely lost in loss record 21 of 116
==27751==    at 0x4C2EC15: realloc (vg_replace_malloc.c:785)
==27751==    by 0x4E6B4CC: ??? (in /usr/lib64/libdbus-1.so.3.19.7)
==27751==    by 0x4E6BB56: _dbus_string_append (in /usr/lib64/libdbus-1.so.3.19.7)
==27751==    by 0x1121D1: main (main.c:546)
==27751==
==27751== 72 bytes in 1 blocks are definitely lost in loss record 55 of 116
==27751==    at 0x4C2EC15: realloc (vg_replace_malloc.c:785)
==27751==    by 0x4E6B4CC: ??? (in /usr/lib64/libdbus-1.so.3.19.7)
==27751==    by 0x4E6BEA5: _dbus_string_append_byte (in /usr/lib64/libdbus-1.so.3.19.7)
==27751==    by 0x4E47EFD: ??? (in /usr/lib64/libdbus-1.so.3.19.7)
==27751==    by 0x4E7143E: ??? (in /usr/lib64/libdbus-1.so.3.19.7)
==27751==    by 0x4E62416: ??? (in /usr/lib64/libdbus-1.so.3.19.7)
==27751==    by 0x4E609FE: dbus_server_listen (in /usr/lib64/libdbus-1.so.3.19.7)
==27751==    by 0x116518: process_config_first_time_only (bus.c:471)
==27751==    by 0x116518: bus_context_new (bus.c:809)
==27751==    by 0x111D40: main (main.c:690)
==27751==

The first one seems to be relatively easy to fix by calling _dbus_string_free(&address) when address is no longer needed, but, unfortunately, I'm not sure how to fix the second one.
Comment 1 Simon McVittie 2018-07-21 10:12:16 UTC
These both look like they're O(1) per run of the dbus-daemon, so are not urgent to fix immediately.
Comment 2 Simon McVittie 2018-07-23 19:51:55 UTC
If you can reproduce these with dbus debug symbols available (on Debian that'd be the dbus-dbgsym and libdbus-1-3-dbgsym packages, but I don't know how this works in other distributions) then that would give us better backtraces, but I think I can guess what these are already.

> ==27751==    by 0x4E6BB56: _dbus_string_append (in /usr/lib64/libdbus-1.so.3.19.7)
> ==27751==    by 0x1121D1: main (main.c:546)

To avoid this and leaks like it, main() would need to free/clear/otherwise destroy all its local variables before returning 0. At the moment it doesn't. It isn't worth putting a lot of effort into this, because the amount of memory allocated by main() is similar to the length of the command line; and by the time we'd free it, the dbus-daemon is about to exit anyway.

> ==27751==    by 0x4E6BEA5: _dbus_string_append_byte (in /usr/lib64/libdbus-1.so.3.19.7)
> ==27751==    by 0x4E47EFD: ??? (in /usr/lib64/libdbus-1.so.3.19.7)
> ==27751==    by 0x4E7143E: ??? (in /usr/lib64/libdbus-1.so.3.19.7)
> ==27751==    by 0x4E62416: ??? (in /usr/lib64/libdbus-1.so.3.19.7)
> ==27751==    by 0x4E609FE: dbus_server_listen (in /usr/lib64/libdbus-1.so.3.19.7)

It looks as though the "systemd:" code path in _dbus_server_listen_platform_specific() leaks its local variable "DBusString address". This is a real leak, but very minor, and any non-pathological process is only going to want to listen on "systemd:" once, so it will leak O(1) bytes.

I'm trying to get the automated tests to run more things under valgrind, but running the actual dbus-daemon under valgrind is some way down the line and might require upstream changes in AX_VALGRIND_CHECK (some juggling environment variables needs to happen to avoid running libtool's wrapper shell script under valgrind, which fails, because bash has some small (intentional?) leaks).
Comment 3 Evgeny Vereshchagin 2018-07-24 04:51:04 UTC
@smcv thank you for looking into this. I totally agree that the leaks are harmless and I should have set the importance to low (or even lower) when I opened the issue.

Regarding valgring, if I recall correctly, I had debug symbols installed, but it seems valgrind had trouble unwinding the stack. I'll see what I can do.

And, yes, it would be great to run more things under valgrind, though since a while ago, because it slows down everything considerably and doesn't keep up with the addition of new syscalls and options to the kernel (which is hard, to be honest), I've been using it only to check fd leaks in systemd. I prefer ASan theese days, but I'm pretty sure it'll complain about memory leaks in bash as well.
Comment 4 Simon McVittie 2018-08-17 17:01:21 UTC
Created attachment 141170 [details] [review]
bus: Free address (from --address) when we have finished  using it
Comment 5 Simon McVittie 2018-08-17 17:03:07 UTC
Created attachment 141171 [details] [review]
server-unix: Don't leak address of systemd server on  success

---

Unlike the previous attachment, running tests under AddressSanitizer doesn't reproduce this one, but the bug and solution are fairly obvious.
Comment 6 Philip Withnall 2018-08-17 18:08:57 UTC
Comment on attachment 141170 [details] [review]
bus: Free address (from --address) when we have finished  using it

Review of attachment 141170 [details] [review]:
-----------------------------------------------------------------

r+
Comment 7 Philip Withnall 2018-08-17 18:09:46 UTC
Comment on attachment 141171 [details] [review]
server-unix: Don't leak address of systemd server on  success

Review of attachment 141171 [details] [review]:
-----------------------------------------------------------------

r+
Comment 8 Simon McVittie 2018-08-29 18:03:59 UTC
Fixed in git for 1.13.8, but leaving this open for now until I apply the patches to dbus-1.12
Comment 9 Simon McVittie 2018-08-31 15:18:46 UTC
Fixed in git for 1.13.8, 1.12.12
Comment 10 Philip Withnall 2018-08-31 15:27:23 UTC
Closing the issue now, or is there a reason to leave it open further?


Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.