LinuxLists.cc - [PATCH BlueZ 1/3] gdbus: Fix memory leak

2014-02-10 17:14:56

Subject: [PATCH BlueZ 1/3] gdbus: Fix memory leak

data->conn and data->path must be destroyed before freeing "data".
---
gdbus/object.c | 2 ++
1 file changed, 2 insertions(+)

diff --git a/gdbus/object.c b/gdbus/object.c
index b248cbb..13cf9a9 100644
--- a/gdbus/object.c
+++ b/gdbus/object.c
@@ -1253,6 +1253,8 @@ static struct generic_data *object_path_ref(DBusConnection *connection,

if (!dbus_connection_register_object_path(connection, path,
&generic_table, data)) {
+ dbus_connection_unref(data->conn);
+ g_free(data->path);
g_free(data->introspect);
g_free(data);
return NULL;
--
1.7.9.5

2014-02-18 20:25:26

by Luiz Augusto von Dentz

[permalink] [raw]

Subject: Re: [PATCH BlueZ v2] gdbus: Fix incorrect DBusConnection reference counting

Hi Anderson,

On Tue, Feb 18, 2014 at 6:45 PM, Anderson Lizardo
<[email protected]> wrote:
> Commit abfc2b0dd5c3e33abfdf1a815b16d492c1751c06 attempted to fix a crash
> related to improper reference counting, but the main issue was that the
> reference was taken only during the function call (which is usually
> unnecessary for single thread), but still passed a pointer to
> DBusConnection to a function that is called by the mainloop. This left a
> window where the DBusConnection can be destroyed.
>
> Fixes this crash on unit/test-gdbus-client:
>
> ==32642== Invalid read of size 4
> ==32642== at 0x690D0A6: dbus_connection_ref (in
> /lib/i386-linux-gnu/libdbus-1.so.3.7.6)
> ==32642== by 0x804CEDB: message_dispatch (mainloop.c:73)
> ==32642== by 0x684580E: g_timeout_dispatch (gmain.c:4450)
> ==32642== by 0x6844A75: g_main_context_dispatch (gmain.c:3065)
> ==32642== by 0x6844E14: g_main_context_iterate.isra.23 (gmain.c:3712)
> ==32642== by 0x68452FA: g_main_loop_run (gmain.c:3906)
> ==32642== by 0x804C7D3: client_connect_disconnect
> (test-gdbus-client.c:188)
> ==32642== by 0x6868DB2: g_test_run_suite_internal (gtestutils.c:2067)
> ==32642== by 0x6868F8D: g_test_run_suite_internal (gtestutils.c:2138)
> ==32642== by 0x6869320: g_test_run_suite (gtestutils.c:2189)
> ==32642== by 0x686936B: g_test_run (gtestutils.c:1508)
> ==32642== by 0x696D4D2: (below main) (libc-start.c:226)
> ==32642== Address 0x709c6e4 is 140 bytes inside a block of size 144
> free'd
> ==32642== at 0x67E806C: free (in
> /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
> ==32642== by 0x692D62E: dbus_free (in
> /lib/i386-linux-gnu/libdbus-1.so.3.7.6)
> ==32642== by 0x690E1C2: ??? (in
> /lib/i386-linux-gnu/libdbus-1.so.3.7.6)
> ==32642== by 0x804AAEC: destroy_context (test-gdbus-client.c:104)
> ==32642== by 0x6868DB2: g_test_run_suite_internal (gtestutils.c:2067)
> ==32642== by 0x6868F8D: g_test_run_suite_internal (gtestutils.c:2138)
> ==32642== by 0x6869320: g_test_run_suite (gtestutils.c:2189)
> ==32642== by 0x686936B: g_test_run (gtestutils.c:1508)
> ==32642== by 0x696D4D2: (below main) (libc-start.c:226)
> ---
> gdbus/mainloop.c | 14 ++++----------
> 1 file changed, 4 insertions(+), 10 deletions(-)
>
> diff --git a/gdbus/mainloop.c b/gdbus/mainloop.c
> index 099b67f..ec52554 100644
> --- a/gdbus/mainloop.c
> +++ b/gdbus/mainloop.c
> @@ -70,8 +70,6 @@ static gboolean message_dispatch(void *data)
> {
> DBusConnection *conn = data;
>
> - dbus_connection_ref(conn);
> -
> /* Dispatch messages */
> while (dbus_connection_dispatch(conn) == DBUS_DISPATCH_DATA_REMAINS);
>
> @@ -84,7 +82,8 @@ static inline void queue_dispatch(DBusConnection *conn,
> DBusDispatchStatus status)
> {
> if (status == DBUS_DISPATCH_DATA_REMAINS)
> - g_timeout_add(DISPATCH_TIMEOUT, message_dispatch, conn);
> + g_timeout_add(DISPATCH_TIMEOUT, message_dispatch,
> + dbus_connection_ref(conn));
> }
>
> static gboolean watch_func(GIOChannel *chan, GIOCondition cond, gpointer data)
> @@ -92,9 +91,6 @@ static gboolean watch_func(GIOChannel *chan, GIOCondition cond, gpointer data)
> struct watch_info *info = data;
> unsigned int flags = 0;
> DBusDispatchStatus status;
> - DBusConnection *conn;
> -
> - conn = dbus_connection_ref(info->conn);
>
> if (cond & G_IO_IN) flags |= DBUS_WATCH_READABLE;
> if (cond & G_IO_OUT) flags |= DBUS_WATCH_WRITABLE;
> @@ -103,10 +99,8 @@ static gboolean watch_func(GIOChannel *chan, GIOCondition cond, gpointer data)
>
> dbus_watch_handle(info->watch, flags);
>
> - status = dbus_connection_get_dispatch_status(conn);
> - queue_dispatch(conn, status);
> -
> - dbus_connection_unref(conn);
> + status = dbus_connection_get_dispatch_status(info->conn);
> + queue_dispatch(info->conn, status);
>
> return TRUE;
> }
> --
> 1.8.5.3

Applied, thanks.

--
Luiz Augusto von Dentz

2014-02-18 16:45:17

by Anderson Lizardo

[permalink] [raw]

Subject: [PATCH BlueZ v2] gdbus: Fix incorrect DBusConnection reference counting

Commit abfc2b0dd5c3e33abfdf1a815b16d492c1751c06 attempted to fix a crash
related to improper reference counting, but the main issue was that the
reference was taken only during the function call (which is usually
unnecessary for single thread), but still passed a pointer to
DBusConnection to a function that is called by the mainloop. This left a
window where the DBusConnection can be destroyed.

Fixes this crash on unit/test-gdbus-client:

==32642== Invalid read of size 4
==32642== at 0x690D0A6: dbus_connection_ref (in
/lib/i386-linux-gnu/libdbus-1.so.3.7.6)
==32642== by 0x804CEDB: message_dispatch (mainloop.c:73)
==32642== by 0x684580E: g_timeout_dispatch (gmain.c:4450)
==32642== by 0x6844A75: g_main_context_dispatch (gmain.c:3065)
==32642== by 0x6844E14: g_main_context_iterate.isra.23 (gmain.c:3712)
==32642== by 0x68452FA: g_main_loop_run (gmain.c:3906)
==32642== by 0x804C7D3: client_connect_disconnect
(test-gdbus-client.c:188)
==32642== by 0x6868DB2: g_test_run_suite_internal (gtestutils.c:2067)
==32642== by 0x6868F8D: g_test_run_suite_internal (gtestutils.c:2138)
==32642== by 0x6869320: g_test_run_suite (gtestutils.c:2189)
==32642== by 0x686936B: g_test_run (gtestutils.c:1508)
==32642== by 0x696D4D2: (below main) (libc-start.c:226)
==32642== Address 0x709c6e4 is 140 bytes inside a block of size 144
free'd
==32642== at 0x67E806C: free (in
/usr/lib/valgrind/vgpreload_memcheck-x86-linux.so)
==32642== by 0x692D62E: dbus_free (in
/lib/i386-linux-gnu/libdbus-1.so.3.7.6)
==32642== by 0x690E1C2: ??? (in
/lib/i386-linux-gnu/libdbus-1.so.3.7.6)
==32642== by 0x804AAEC: destroy_context (test-gdbus-client.c:104)
==32642== by 0x6868DB2: g_test_run_suite_internal (gtestutils.c:2067)
==32642== by 0x6868F8D: g_test_run_suite_internal (gtestutils.c:2138)
==32642== by 0x6869320: g_test_run_suite (gtestutils.c:2189)
==32642== by 0x686936B: g_test_run (gtestutils.c:1508)
==32642== by 0x696D4D2: (below main) (libc-start.c:226)
---
gdbus/mainloop.c | 14 ++++----------
1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/gdbus/mainloop.c b/gdbus/mainloop.c
index 099b67f..ec52554 100644
--- a/gdbus/mainloop.c
+++ b/gdbus/mainloop.c
@@ -70,8 +70,6 @@ static gboolean message_dispatch(void *data)
{
DBusConnection *conn = data;

- dbus_connection_ref(conn);
-
/* Dispatch messages */
while (dbus_connection_dispatch(conn) == DBUS_DISPATCH_DATA_REMAINS);

@@ -84,7 +82,8 @@ static inline void queue_dispatch(DBusConnection *conn,
DBusDispatchStatus status)
{
if (status == DBUS_DISPATCH_DATA_REMAINS)
- g_timeout_add(DISPATCH_TIMEOUT, message_dispatch, conn);
+ g_timeout_add(DISPATCH_TIMEOUT, message_dispatch,
+ dbus_connection_ref(conn));
}

static gboolean watch_func(GIOChannel *chan, GIOCondition cond, gpointer data)
@@ -92,9 +91,6 @@ static gboolean watch_func(GIOChannel *chan, GIOCondition cond, gpointer data)
struct watch_info *info = data;
unsigned int flags = 0;
DBusDispatchStatus status;
- DBusConnection *conn;
-
- conn = dbus_connection_ref(info->conn);

if (cond & G_IO_IN) flags |= DBUS_WATCH_READABLE;
if (cond & G_IO_OUT) flags |= DBUS_WATCH_WRITABLE;
@@ -103,10 +99,8 @@ static gboolean watch_func(GIOChannel *chan, GIOCondition cond, gpointer data)

dbus_watch_handle(info->watch, flags);

- status = dbus_connection_get_dispatch_status(conn);
- queue_dispatch(conn, status);
-
- dbus_connection_unref(conn);
+ status = dbus_connection_get_dispatch_status(info->conn);
+ queue_dispatch(info->conn, status);

return TRUE;
}
--
1.8.5.3

2014-02-17 11:56:57

by Anderson Lizardo

[permalink] [raw]

Subject: Re: [PATCH BlueZ 1/3] gdbus: Fix memory leak

Hi,

On Mon, Feb 10, 2014 at 1:14 PM, Anderson Lizardo
<[email protected]> wrote:
> data->conn and data->path must be destroyed before freeing "data".
> ---
> gdbus/object.c | 2 ++
> 1 file changed, 2 insertions(+)

ping.

--
Anderson Lizardo
http://www.indt.org/?lang=en
INdT - Manaus - Brazil

2014-02-10 17:14:58

by Anderson Lizardo

[permalink] [raw]

Subject: [PATCH BlueZ 3/3] unit: Fix memory leaks in test-gdbus-client

---
unit/test-gdbus-client.c | 5 +++++
1 file changed, 5 insertions(+)

diff --git a/unit/test-gdbus-client.c b/unit/test-gdbus-client.c
index 685729a..ee8a760 100644
--- a/unit/test-gdbus-client.c
+++ b/unit/test-gdbus-client.c
@@ -101,6 +101,7 @@ static void destroy_context(struct context *context)

dbus_connection_flush(context->dbus_conn);
dbus_connection_close(context->dbus_conn);
+ dbus_connection_unref(context->dbus_conn);

g_main_loop_unref(context->main_loop);

@@ -956,6 +957,10 @@ static void client_force_disconnect(void)

g_main_loop_run(context->main_loop);

+ g_dbus_unregister_interface(conn, SERVICE_PATH, SERVICE_NAME1);
+ g_dbus_detach_object_manager(conn);
+ dbus_connection_unref(conn);
+
destroy_context(context);
}

--
1.7.9.5

2014-02-10 17:14:57

by Anderson Lizardo

[permalink] [raw]

Subject: [PATCH BlueZ 1/3] gdbus: Fix memory leak

Subject: Re: [PATCH BlueZ v2] gdbus: Fix incorrect DBusConnection reference counting

Subject: [PATCH BlueZ v2] gdbus: Fix incorrect DBusConnection reference counting

Subject: Re: [PATCH BlueZ 1/3] gdbus: Fix memory leak

Subject: [PATCH BlueZ 3/3] unit: Fix memory leaks in test-gdbus-client

Subject: [PATCH BlueZ 2/3] gdbus: Fix incorrect DBusConnection reference counting