[PATCH BlueZ] obexd: Reference count the phonebook back-end setup and teardown

[PATCH BlueZ] obexd: Reference count the phonebook back-end setup and teardown

[Paul Menzel]

Stopping obexd on Debian sid/unstable with *bluez* 5.87-1 logs a GObject
critical:

    obexd[10687]: Terminating
    systemd[1804]: Stopping obex.service - Bluetooth OBEX service...
    obexd[10687]: g_object_unref: assertion 'G_IS_OBJECT (object)' failed
    systemd[1804]: Stopped obex.service - Bluetooth OBEX service.

Two builtin plugins use the phonebook back-end: pbap (pbap_init() at
obexd/plugins/pbap.c:962, pbap_exit() at :1002) and irmc (irmc_init() at
obexd/plugins/irmc.c:446, irmc_exit() at :473). Each calls phonebook_init()
when it is loaded and phonebook_exit() when it is unloaded, but neither the
callers nor the back-end track ownership of the singleton they share.
plugin_init() therefore sets the back-end up twice and plugin_cleanup()
tears it down twice. A gdb trace of the shutdown path confirms both pairs
of calls.

The dummy back-end tolerates this by accident: phonebook_init() bails out
early when root_folder is already set, and the second phonebook_exit() only
repeats a g_free()/NULL assignment. The ebook back-end, which Debian builds
(*bluez-obexd* depends on *libebook-1.2* and *libedataserver*), does not.
Its phonebook_init() stores three GObject references in static variables
and phonebook_exit() unconditionally drops all three, so the second
teardown unrefs objects that were already finalized. The registry and the
address book are effectively singletons in evolution-data-server and merely
gain a second reference, but e_book_client_connect_sync() hands back a
fresh client on every call, so the first client leaks and the second one is
unreffed twice – hence a single critical rather than three.

Put the ownership tracking in one place instead of duplicating it in every
back-end: phonebook_init() and phonebook_exit() now live in a new shared
obexd/plugins/phonebook.c and reference count the back-end, so only the
first init and the last exit reach it. The back-end entry points are
renamed to phonebook_driver_init()/phonebook_driver_exit() so that they
cannot be called directly by mistake.

Assisted-by: Claude Code:claude-opus-5
---
 Makefile.obexd                    |  1 +
 obexd/plugins/phonebook-dummy.c   |  4 +--
 obexd/plugins/phonebook-ebook.c   |  4 +--
 obexd/plugins/phonebook-tracker.c |  4 +--
 obexd/plugins/phonebook.c         | 47 +++++++++++++++++++++++++++++++
 obexd/plugins/phonebook.h         | 10 +++++++
 6 files changed, 64 insertions(+), 6 deletions(-)
 create mode 100644 obexd/plugins/phonebook.c

diff --git a/Makefile.obexd b/Makefile.obexd
index 7ad74e1..9daa5aa 100644
--- a/Makefile.obexd
+++ b/Makefile.obexd
@@ -48,6 +48,7 @@ obexd_builtin_modules += pbap
 obexd_builtin_sources += obexd/plugins/pbap.c \
 				obexd/plugins/vcard.h obexd/plugins/vcard.c \
 				obexd/plugins/phonebook.h \
+				obexd/plugins/phonebook.c \
 				obexd/plugins/phonebook-@PLUGIN_PHONEBOOK@.c
 EXTRA_DIST += obexd/plugins/phonebook-dummy.c obexd/plugins/phonebook-ebook.c \
 				obexd/plugins/phonebook-tracker.c
diff --git a/obexd/plugins/phonebook-dummy.c b/obexd/plugins/phonebook-dummy.c
index 0dce13f..f308ea7 100644
--- a/obexd/plugins/phonebook-dummy.c
+++ b/obexd/plugins/phonebook-dummy.c
@@ -73,7 +73,7 @@ static void query_free(void *user_data)
 	g_free(query);
 }
 
-int phonebook_init(void)
+int phonebook_driver_init(void)
 {
 	if (root_folder)
 		return 0;
@@ -84,7 +84,7 @@ int phonebook_init(void)
 	return 0;
 }
 
-void phonebook_exit(void)
+void phonebook_driver_exit(void)
 {
 	g_free(root_folder);
 	root_folder = NULL;
diff --git a/obexd/plugins/phonebook-ebook.c b/obexd/plugins/phonebook-ebook.c
index 5fc0498..beb72b2 100644
--- a/obexd/plugins/phonebook-ebook.c
+++ b/obexd/plugins/phonebook-ebook.c
@@ -625,7 +625,7 @@ next:
 	return data;
 }
 
-int phonebook_init(void)
+int phonebook_driver_init(void)
 {
 	EClient *client;
 	GError *gerr = NULL;
@@ -662,7 +662,7 @@ int phonebook_init(void)
 	return 0;
 }
 
-void phonebook_exit(void)
+void phonebook_driver_exit(void)
 {
 	g_object_unref(book_client);
 	g_object_unref(address_book);
diff --git a/obexd/plugins/phonebook-tracker.c b/obexd/plugins/phonebook-tracker.c
index eb7a84f..36e73db 100644
--- a/obexd/plugins/phonebook-tracker.c
+++ b/obexd/plugins/phonebook-tracker.c
@@ -1420,14 +1420,14 @@ done:
 	 */
 }
 
-int phonebook_init(void)
+int phonebook_driver_init(void)
 {
 	g_type_init();
 
 	return 0;
 }
 
-void phonebook_exit(void)
+void phonebook_driver_exit(void)
 {
 }
 
diff --git a/obexd/plugins/phonebook.c b/obexd/plugins/phonebook.c
new file mode 100644
index 0000000..6afdb7a
--- /dev/null
+++ b/obexd/plugins/phonebook.c
@@ -0,0 +1,47 @@
+// SPDX-License-Identifier: GPL-2.0-or-later
+/*
+ *
+ *  OBEX Server
+ *
+ *  Copyright (C) 2007-2010  Marcel Holtmann <marcel@holtmann.org>
+ *
+ *
+ */
+
+#ifdef HAVE_CONFIG_H
+#include <config.h>
+#endif
+
+#include <stdint.h>
+
+#include <glib.h>
+
+#include "phonebook.h"
+
+static unsigned int refcount = 0;
+
+int phonebook_init(void)
+{
+	int err;
+
+	if (refcount > 0) {
+		refcount++;
+		return 0;
+	}
+
+	err = phonebook_driver_init();
+	if (err < 0)
+		return err;
+
+	refcount = 1;
+
+	return 0;
+}
+
+void phonebook_exit(void)
+{
+	if (refcount == 0 || --refcount > 0)
+		return;
+
+	phonebook_driver_exit();
+}
diff --git a/obexd/plugins/phonebook.h b/obexd/plugins/phonebook.h
index c73ae73..b878b14 100644
--- a/obexd/plugins/phonebook.h
+++ b/obexd/plugins/phonebook.h
@@ -79,9 +79,19 @@ typedef void (*phonebook_entry_cb) (const char *id, uint32_t handle,
 typedef void (*phonebook_cache_ready_cb) (void *user_data);
 
 
+/*
+ * Set up and tear down the phonebook back-end. The back-end is shared by
+ * the pbap and the irmc plugin, which are loaded and unloaded independently
+ * of each other, so the calls are reference counted: only the first
+ * phonebook_init() and the last phonebook_exit() reach the back-end.
+ */
 int phonebook_init(void);
 void phonebook_exit(void);
 
+/* Implemented by the back-end, only called through the pair above. */
+int phonebook_driver_init(void);
+void phonebook_driver_exit(void);
+
 /*
  * Changes the current folder in the phonebook back-end. The PBAP core
  * doesn't validate or restrict the possible values for the folders,
-- 
2.54.0

RE: [BlueZ] obexd: Reference count the phonebook back-end setup and teardown

[bluez.test.bot]

[-- Attachment #1: Type: text/plain, Size: 2134 bytes --]

This is automated email and please do not reply to this email!

Dear submitter,

Thank you for submitting the patches to the linux bluetooth mailing list.
This is a CI test results with your patch series:
PW Link:https://patchwork.kernel.org/project/bluetooth/list/?series=1147292

---Test result---

Test Summary:
CheckPatch                    FAIL      0.45 seconds
GitLint                       PASS      0.24 seconds
BuildEll                      PASS      20.23 seconds
BluezMake                     PASS      598.08 seconds
MakeCheck                     PASS      19.61 seconds
MakeDistcheck                 PASS      158.20 seconds
CheckValgrind                 PASS      229.46 seconds
CheckSmatch                   PASS      308.91 seconds
bluezmakeextell               PASS      104.97 seconds
IncrementalBuild              PASS      608.01 seconds
ScanBuild                     PASS      970.58 seconds

Details
##############################
Test: CheckPatch - FAIL
Desc: Run checkpatch.pl script
Output:
[BlueZ] obexd: Reference count the phonebook back-end setup and teardown
WARNING:BAD_SIGN_OFF: Non-standard signature: Assisted-by:
#88: 
Assisted-by: Claude Code:claude-opus-5

ERROR:BAD_SIGN_OFF: Unrecognized email address: 'Claude Code:claude-opus-5'
#88: 
Assisted-by: Claude Code:claude-opus-5

ERROR:INITIALISED_STATIC: do not initialise statics to 0
#202: FILE: obexd/plugins/phonebook.c:21:
+static unsigned int refcount = 0;

/github/workspace/src/patch/14753861.patch total: 2 errors, 1 warnings, 121 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

/github/workspace/src/patch/14753861.patch has style problems, please review.

NOTE: Ignored message types: COMMIT_MESSAGE COMPLEX_MACRO CONST_STRUCT FILE_PATH_CHANGES MISSING_SIGN_OFF PREFER_PACKED SPDX_LICENSE_TAG SPLIT_STRING SSCANF_TO_KSTRTO

NOTE: If any of the errors are false positives, please report
      them to the maintainer, see CHECKPATCH in MAINTAINERS.




https://github.com/bluez/bluez/pull/2409

---
Regards,
Linux Bluetooth