Fix the ref/unref dependency

The current code has the profile ref the device, but the device has no back-ref to
the profile. The following code will free the profile from underneath the
client, despite the caller still having a valid ref:

p = ratbag_device_get_profile_by_index(device, 0);
ratbag_profile_ref(p);
ratbag_device_unref(device);

Fix this by not using refs internally. After probe, the device is static
anyway, so the various refs internally aren't needed. Instead, we also ref/unref
the parent struct whenever we ref/unref a struct. Thus, whenevever the device
refcount reaches zero, we know the caller has no ref active.

Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>

src/libratbag.c

@@ -37,6 +37,11 @@
 #include "libratbag-private.h"
 #include "libratbag-util.h"
 
+static void
+ratbag_profile_destroy(struct ratbag_profile *profile);
+static void
+ratbag_button_destroy(struct ratbag_button *button);
+
 static void
 ratbag_default_log_func(struct ratbag *ratbag,
 			enum ratbag_log_priority priority,
@@ -278,12 +283,14 @@ ratbag_device_destroy(struct ratbag_device *device)
 	if (!device)
 		return;
 
+	/* if we get to the point where the device is destroyed, profiles,
+	 * buttons, etc. are at a refcount of 0, so we can destroy
+	 * everything */
 	if (device->driver && device->driver->remove)
 		device->driver->remove(device);
 
-	/* the profiles are created during probe(), we should unref them */
 	list_for_each_safe(profile, next, &device->profiles, link)
-		ratbag_profile_unref(profile);
+		ratbag_profile_destroy(profile);
 
 	if (device->udev_device)
 		udev_device_unref(device->udev_device);
@@ -635,7 +642,7 @@ ratbag_create_button(struct ratbag_profile *profile, unsigned int index)
 	struct ratbag_button *button;
 
 	button = zalloc(sizeof(*button));
-	button->refcount = 1;
+	button->refcount = 0;
 	button->profile = profile;
 	button->index = index;
 
@@ -669,7 +676,7 @@ ratbag_create_profile(struct ratbag_device *device,
 	unsigned i;
 
 	profile = zalloc(sizeof(*profile));
-	profile->refcount = 1;
+	profile->refcount = 0;
 	profile->device = device;
 	profile->index = index;
 
@@ -707,6 +714,7 @@ ratbag_device_init_profiles(struct ratbag_device *device,
 LIBRATBAG_EXPORT struct ratbag_profile *
 ratbag_profile_ref(struct ratbag_profile *profile)
 {
+	ratbag_device_ref(profile->device);
 	profile->refcount++;
 	return profile;
 }
@@ -716,9 +724,13 @@ ratbag_profile_destroy(struct ratbag_profile *profile)
 {
 	struct ratbag_button *button, *next;
 
-	/* the buttons are created by the profile, so we clean them up */
+	/* if we get to the point where the profile is destroyed, buttons,
+	 * resolutions , etc. are at a refcount of 0, so we can destroy
+	 * everything */
 	list_for_each_safe(button, next, &profile->buttons, link)
-		ratbag_button_unref(button);
+		ratbag_button_destroy(button);
+
+	/* Resolution is a fixed list of structs, no freeing required */
 
 	list_remove(&profile->link);
 	free(profile);
@@ -727,6 +739,7 @@ ratbag_profile_destroy(struct ratbag_profile *profile)
 LIBRATBAG_EXPORT struct ratbag_profile *
 ratbag_profile_unref(struct ratbag_profile *profile)
 {
+	struct ratbag_profile *p = NULL;
 
 	if (profile == NULL)
 		return NULL;
@@ -734,11 +747,11 @@ ratbag_profile_unref(struct ratbag_profile *profile)
 	assert(profile->refcount > 0);
 	profile->refcount--;
 	if (profile->refcount > 0)
-		return profile;
+		p = profile;
 
-	ratbag_profile_destroy(profile);
+	ratbag_device_unref(profile->device);
 
-	return NULL;
+	return p;
 }
 
 LIBRATBAG_EXPORT struct ratbag_profile *
@@ -886,6 +899,7 @@ ratbag_profile_get_resolution(struct ratbag_profile *profile, unsigned int idx)
 LIBRATBAG_EXPORT struct ratbag_resolution *
 ratbag_resolution_ref(struct ratbag_resolution *resolution)
 {
+	ratbag_profile_ref(resolution->profile);
 	resolution->refcount++;
 	return resolution;
 }
@@ -893,17 +907,19 @@ ratbag_resolution_ref(struct ratbag_resolution *resolution)
 LIBRATBAG_EXPORT struct ratbag_resolution *
 ratbag_resolution_unref(struct ratbag_resolution *resolution)
 {
+	struct ratbag_resolution *r = NULL;
+
 	if (resolution == NULL)
 		return NULL;
 
 	assert(resolution->refcount > 0);
 	resolution->refcount--;
 	if (resolution->refcount > 0)
-		return resolution;
+		r = resolution;
 
-	/* Resolution is a fixed list of structs, no freeing required */
+	ratbag_profile_unref(resolution->profile);
 
-	return NULL;
+	return r;
 }
 
 LIBRATBAG_EXPORT int
@@ -1171,6 +1187,7 @@ ratbag_button_disable(struct ratbag_button *button)
 LIBRATBAG_EXPORT struct ratbag_button *
 ratbag_button_ref(struct ratbag_button *button)
 {
+	ratbag_profile_ref(button->profile);
 	button->refcount++;
 	return button;
 }
@@ -1185,17 +1202,19 @@ ratbag_button_destroy(struct ratbag_button *button)
 LIBRATBAG_EXPORT struct ratbag_button *
 ratbag_button_unref(struct ratbag_button *button)
 {
+	struct ratbag_button *b = NULL;
+
 	if (button == NULL)
 		return NULL;
 
 	assert(button->refcount > 0);
 	button->refcount--;
 	if (button->refcount > 0)
-		return button;
+		b = button;
 
-	ratbag_button_destroy(button);
+	ratbag_profile_unref(button->profile);
 
-	return NULL;
+	return b;
 }
 
 LIBRATBAG_EXPORT void

test/test-device.c

@@ -133,6 +133,44 @@ START_TEST(device_init)
 }
 END_TEST
 
+#define ref_unref_test(T, a) \
+ { \
+	 int i; \
+	 struct T *tmp = NULL; \
+ \
+	 for (i = 0; i < 1000; i++) { \
+		 tmp = T##_ref(a); \
+		 ck_assert(tmp == a); \
+	 } \
+	 for (i = 0; i < 1000; i++) { \
+		 tmp = T##_unref(a); \
+		 ck_assert(tmp == a); \
+	 } \
+	 for (i = 0; i < 1000; i++) { \
+		 tmp = T##_ref(a); \
+		 ck_assert(tmp == a); \
+		 tmp = T##_unref(a); \
+		 ck_assert(tmp == a); \
+	 } \
+ }
+
+START_TEST(device_ref_unref)
+{
+	struct ratbag *r;
+	struct ratbag_device *d;
+	struct ratbag_test_device td = sane_device;
+
+	r = ratbag_create_context(&abort_iface, NULL);
+	d = ratbag_device_new_test_device(r, &td);
+	ck_assert(d != NULL);
+
+	ref_unref_test(ratbag_device, d);
+
+	ratbag_device_unref(d);
+	ratbag_unref(r);
+}
+END_TEST
+
 START_TEST(device_profiles)
 {
 	struct ratbag *r;
@@ -169,6 +207,27 @@ START_TEST(device_profiles)
 }
 END_TEST
 
+START_TEST(device_profiles_ref_unref)
+{
+	struct ratbag *r;
+	struct ratbag_device *d;
+	struct ratbag_profile *p;
+
+	struct ratbag_test_device td = sane_device;
+
+	r = ratbag_create_context(&abort_iface, NULL);
+	d = ratbag_device_new_test_device(r, &td);
+	p = ratbag_device_get_profile_by_index(d, 1);
+
+	ref_unref_test(ratbag_profile, p);
+
+	ratbag_device_unref(d);
+
+	ratbag_profile_unref(p);
+	ratbag_unref(r);
+}
+END_TEST
+
 START_TEST(device_profiles_num_0)
 {
 	struct ratbag *r;
@@ -348,10 +407,12 @@ test_context_suite(void)
 	s = suite_create("device");
 	tc = tcase_create("device");
 	tcase_add_test(tc, device_init);
+	tcase_add_test(tc, device_ref_unref);
 	suite_add_tcase(s, tc);
 
 	tc = tcase_create("profiles");
 	tcase_add_test(tc, device_profiles);
+	tcase_add_test(tc, device_profiles_ref_unref);
 	tcase_add_test(tc, device_profiles_num_0);
 	tcase_add_test(tc, device_profiles_multiple_active);
 	tcase_add_test(tc, device_profiles_get_invalid);