Bug 36793

Summary:	re-registering an object at the same path destroys state
Product:	dbus	Reporter:	Simon McVittie <smcv>
Component:	GLib	Assignee:	Simon McVittie <smcv>
Status:	RESOLVED FIXED	QA Contact:	John (J5) Palmieri <johnp>
Severity:	normal
Priority:	medium	CC:	rob.taylor
Version:	unspecified	Keywords:	patch
Hardware:	Other
OS:	All
URL:	http://cgit.collabora.co.uk/git/user/smcv/dbus-glib-smcv.git/log/?h=registrations-36793
Whiteboard:
i915 platform:		i915 features:
Bug Depends on:
Bug Blocks:	32087, 36811
Attachments:	Rename test/core/unregister to registrations (no source changes) [2/6] Convert registrations test to use GTest Subsume the test for #5688 into the more general registrations test [4/6] dbus_g_connection_unregister_g_object: fix out-of-bounds reading [5/6] dbus_g_connection_register_g_object: don't destroy state on early return [6/6] Add a regression test for re-registering an object at the same path [1/6 v2] Rename test/core/unregister to registrations (no source changes) [3/6 v2] Subsume the test for #5688 into the more general registrations test

Description Simon McVittie 2011-05-03 03:27:05 UTC

If you do this:

* create a GObject
* export it on D-Bus (at n >= 1 object paths)
* unref the object enough times that it is freed

there should be no leak.

What actually happens is:

* each object registration "o" has a weak ref which results in a
  call to object_registration_object_died
* object_registration_object_died sets o->object to NULL
* object_registration_object_died calls dbus_connection_unregister_object_path
* this results in a call to object_registration_unregistered
* which calls object_registration_free
* which does not remove the dbus_glib_object_registrations qdata
  from the object, because the object has already been freed at this point
  so it's too late (and also because o->object is NULL)

Because the object registrations are already qdata, we shouldn't really need a weak ref at all...

Comment 1 Simon McVittie 2011-05-03 07:00:53 UTC

Working on this. The longer I look at this code, the more bugs I spot...

Comment 2 Simon McVittie 2011-05-03 08:08:12 UTC

Not fixed yet, but here are some patches to improve test coverage and fix related bugs...

Comment 3 Simon McVittie 2011-05-03 08:08:57 UTC

Created attachment 46286 [details] [review]
Rename test/core/unregister to registrations (no source changes)

Comment 4 Simon McVittie 2011-05-03 08:09:17 UTC

Created attachment 46287 [details] [review]
[2/6] Convert registrations test to use GTest

Also use a private bus connection, for potentially better leak-detection.

Comment 5 Simon McVittie 2011-05-03 08:09:37 UTC

Created attachment 46288 [details] [review]
Subsume the test for #5688 into the more general  registrations test

Also test that the object is unregistered by the last unref or by forced
disposal, without crashing.

Comment 6 Simon McVittie 2011-05-03 08:09:57 UTC

Created attachment 46289 [details] [review]
[4/6] dbus_g_connection_unregister_g_object: fix out-of-bounds reading

The list of registrations is singly linked; we only avoid a crash here
by luck.

Comment 7 Simon McVittie 2011-05-03 08:10:28 UTC

Created attachment 46290 [details] [review]
[5/6] dbus_g_connection_register_g_object: don't destroy state on early return

At the beginning of the function we steal the object's state so we can add
to the list. After doing that, we must make sure we give it back!

Comment 8 Simon McVittie 2011-05-03 08:10:44 UTC

Created attachment 46291 [details] [review]
[6/6] Add a regression test for re-registering an object at the same path

Comment 9 Simon McVittie 2011-05-03 08:11:25 UTC

(all for now, back to fixing the main bug)

Comment 10 Simon McVittie 2011-05-03 10:53:03 UTC

Actually, I'm going to repurpose this bug for the incidental stuff I fixed along the way, and use the cloned Bug #36811 for the leak I initially reported. So, this is ready for review.

Comment 11 Simon McVittie 2011-05-04 05:02:24 UTC

Created attachment 46314 [details] [review]
[1/6 v2] Rename test/core/unregister to registrations (no source changes)

This amends Attachment #46286 [details], which was incomplete (it didn't change run-test.sh).

Comment 12 Simon McVittie 2011-05-04 05:03:05 UTC

Created attachment 46315 [details] [review]
[3/6 v2] Subsume the test for #5688 into the more general registrations test

This amends Attachment #46288 [details], which was incomplete (it didn't change run-test.sh).

Comment 13 Cosimo Alfarano 2011-05-06 09:12:39 UTC

Also this seems to be fine.

Comment 14 Simon McVittie 2011-05-17 07:01:19 UTC

Fixed in git for 0.94 based on review from Cosimo and Sjoerd.

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.