[PATCH v2] gatt-client: prevent use-after-free when clients disconnect

[Lasse Dalegaard <dalegaard@gmail.com>]

Clients with an acquired characteristic subscription would free the
client during disconnect. When notify_io_destroy then tries to remove
the client, it'll use-after-free.

Add another reference when allocating the notify_io and unref it again
in notify_io_destroy.

This was found with AddressSanitizer:

==766875==ERROR: AddressSanitizer: heap-use-after-free on address 0x7b7782a31df0 at pc 0x55d19ae87cf0 bp 0x7ffcc28ea0d0 sp 0x7ffcc28ea0c0
READ of size 8 at 0x7b7782a31df0 thread T0
    #0 0x55d19ae87cef in notify_io_destroy src/gatt-client.c:1567
    #1 0x55d19ae83462 in sock_io_destroy src/gatt-client.c:1171
    #2 0x55d19ae83a4b in destroy_sock src/gatt-client.c:1192
    #3 0x55d19ae83c52 in sock_hup src/gatt-client.c:1207
    ...

0x7b7782a31df0 is located 0 bytes inside of 32-byte region [0x7b7782a31df0,0x7b7782a31e10)
freed by thread T0 here:
    #0 0x7f4784d1f79d in free /usr/src/debug/gcc/gcc/libsanitizer/asan/asan_malloc_linux.cpp:51
    #1 0x55d19ae85c57 in notify_client_free src/gatt-client.c:1360
    #2 0x55d19ae85dd5 in notify_client_unref src/gatt-client.c:1372
    #3 0x55d19ae86517 in notify_client_disconnect src/gatt-client.c:1418
    ...

previously allocated by thread T0 here:
    #0 0x7f4784d20cb5 in malloc /usr/src/debug/gcc/gcc/libsanitizer/asan/asan_malloc_linux.cpp:67
    #1 0x55d19af6154d in util_malloc src/shared/util.c:46
    #2 0x55d19ae86550 in notify_client_create src/gatt-client.c:1426
    #3 0x55d19ae880cd in characteristic_acquire_notify src/gatt-client.c:1593
    ...

Other avenues could also result in this use-after-free. The root issue
is that the client struct is put in to both the notify list, and the
notify_io struct, but without an extra reference increment.
---
 src/gatt-client.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/gatt-client.c b/src/gatt-client.c
index 44ec95db0..374e67c34 100644
--- a/src/gatt-client.c
+++ b/src/gatt-client.c
@@ -1566,6 +1566,7 @@ static void notify_io_destroy(void *data)
 
 	if (queue_remove(client->chrc->notify_clients, client))
 		notify_client_unref(client);
+	notify_client_unref(client);
 }
 
 static DBusMessage *characteristic_acquire_notify(DBusConnection *conn,
@@ -1607,7 +1608,7 @@ static DBusMessage *characteristic_acquire_notify(DBusConnection *conn,
 	queue_push_tail(chrc->notify_clients, client);
 
 	chrc->notify_io = new0(struct sock_io, 1);
-	chrc->notify_io->data = client;
+	chrc->notify_io->data = notify_client_ref(client);
 	chrc->notify_io->msg = dbus_message_ref(msg);
 	chrc->notify_io->destroy = notify_io_destroy;
 
-- 
2.52.0