Bug 99793

Summary: dbus-hash: Fix memory leaks in internal hash table tests
Product: dbus Reporter: Philip Withnall <bugzilla>
Component: coreAssignee: D-Bus Maintainers <dbus>
Status: RESOLVED FIXED QA Contact: D-Bus Maintainers <dbus>
Severity: normal    
Priority: medium CC: bugzilla
Version: unspecified   
Hardware: All   
OS: All   
Whiteboard:
i915 platform: i915 features:
Attachments: dbus-hash: Fix memory leaks in internal hash table tests
dbus-hash: Fix memory leaks in internal hash table tests

Description Philip Withnall 2017-02-13 13:03:57 UTC
This one’s a bit more involved than the other fixes. Introduce a steal() function to make it clear when hash table functions steal their arguments. For all other cases (mostly for handling the `goto out` paths), use a top-level str_key and str_value which are cleared at the end of the function.
Comment 1 Philip Withnall 2017-02-13 13:04:00 UTC
Created attachment 129553 [details] [review]
dbus-hash: Fix memory leaks in internal hash table tests

This includes fixing a memory leak in _dbus_hash_iter_lookup(), which is
not one of the unit tests; but it is only ever called from the unit
tests, so this is not a user-facing leak.

Coverity IDs: 54730, 54740

Signed-off-by: Philip Withnall <withnall@endlessm.com>
Comment 2 Simon McVittie 2017-02-13 13:13:40 UTC
Comment on attachment 129553 [details] [review]
dbus-hash: Fix memory leaks in internal hash table tests

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

Makes sense.
Comment 3 Simon McVittie 2017-02-13 16:42:19 UTC
Fixed in git for 1.11.10, thanks
Comment 4 Simon McVittie 2017-02-14 17:43:17 UTC
Bisection says this makes tests fail:

dbus-daemon[26876]: Activating service name='org.freedesktop.DBus.TestSuiteEchoService' requested by ':1.2415' (uid=1000 pid=26876 comm=".../bus/.libs/test-bus ")
dbus-daemon[26876]: Failed to activate service 'org.freedesktop.DBus.TestSuiteEchoService': timed out (service_start_timeout=25000ms)
dbus-daemon[26876]: Did not expect error org.freedesktop.DBus.Error.TimedOut

I reverted it.
Comment 5 Simon McVittie 2017-02-14 17:44:16 UTC
It's possible that this is exposing some other wrongness, maybe an issue in an out-of-memory code path: we're doing this from test-bus and not really dbus-daemon.
Comment 6 Philip Withnall 2017-02-16 12:44:20 UTC
Created attachment 129665 [details] [review]
dbus-hash: Fix memory leaks in internal hash table tests

This includes fixing a memory leak in _dbus_hash_iter_lookup(), which is
not one of the unit tests; but it is only ever called from the unit
tests, so this is not a user-facing leak.

Coverity IDs: 54730, 54740

Signed-off-by: Philip Withnall <withnall@endlessm.com>
Comment 7 Philip Withnall 2017-02-16 13:13:31 UTC
The previous version of the patch introduced a double-free in the case where find_generic_function() was called from the more traditional insertion functions (like _dbus_hash_table_insert_string_preallocated()).

This version is a bit less invasive, and only modifies _dbus_hash_iter_lookup(), which is only ever called from the tests. The tests pass with this patch applied.
Comment 8 Simon McVittie 2017-02-16 14:01:49 UTC
Comment on attachment 129665 [details] [review]
dbus-hash: Fix memory leaks in internal hash table tests

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

Looks good.
Comment 9 Simon McVittie 2017-02-16 16:22:54 UTC
Fixed in git for 1.11.10, coming soon.

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.