Bug 99793 - dbus-hash: Fix memory leaks in internal hash table tests
Summary: dbus-hash: Fix memory leaks in internal hash table tests
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: unspecified
Hardware: All All
: medium normal
Assignee: D-Bus Maintainers
QA Contact: D-Bus Maintainers
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-02-13 13:03 UTC by Philip Withnall
Modified: 2017-02-16 16:22 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
dbus-hash: Fix memory leaks in internal hash table tests (12.10 KB, patch)
2017-02-13 13:04 UTC, Philip Withnall
Details | Splinter Review
dbus-hash: Fix memory leaks in internal hash table tests (12.13 KB, patch)
2017-02-16 12:44 UTC, Philip Withnall
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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.