2018-11-19 15:43:18

by Luiz Augusto von Dentz

Subject: [PATCH BlueZ 1/5] gatt: Fix invalid read when disconnecting

From: Luiz Augusto von Dentz <[email protected]>

In case there is a client of AcquireNotify and a disconnect happens the
code not only have to free the client object but also destroy the io
associated with it, for this reason the client object cannot be freed
until the io is destroyed otherwise it may lead to the following error:

Invalid read of size 4
at 0x63920: notify_io_destroy (gatt-client.c:1461)
by 0x63EDB: pipe_io_destroy (gatt-client.c:1082)
by 0x6405B: characteristic_free (gatt-client.c:1663)
by 0x81F33: remove_interface (object.c:667)
by 0x826CB: g_dbus_unregister_interface (object.c:1391)
by 0x85D2B: queue_remove_all (queue.c:354)
by 0x635F7: unregister_service (gatt-client.c:1893)
by 0x85CF7: queue_remove_all (queue.c:339)
by 0x661DF: btd_gatt_client_service_removed (gatt-client.c:2199)
by 0x695CB: gatt_service_removed (device.c:3747)
by 0x85B17: queue_foreach (queue.c:220)
by 0x91283: notify_service_changed (gatt-db.c:280)
by 0x91283: gatt_db_service_destroy (gatt-db.c:291)
Address 0x515ed48 is 0 bytes inside a block of size 20 free'd
at 0x483EAD0: free (vg_replace_malloc.c:530)
by 0x85D2B: queue_remove_all (queue.c:354)
by 0x636D3: unregister_characteristic (gatt-client.c:1741)
by 0x85D2B: queue_remove_all (queue.c:354)
by 0x635F7: unregister_service (gatt-client.c:1893)
by 0x85CF7: queue_remove_all (queue.c:339)
by 0x661DF: btd_gatt_client_service_removed (gatt-client.c:2199)
by 0x695CB: gatt_service_removed (device.c:3747)
by 0x85B17: queue_foreach (queue.c:220)
by 0x91283: notify_service_changed (gatt-db.c:280)
by 0x91283: gatt_db_service_destroy (gatt-db.c:291)
by 0x85D2B: queue_remove_all (queue.c:354)
by 0x91387: gatt_db_clear_range (gatt-db.c:475)
---
src/gatt-client.c | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/src/gatt-client.c b/src/gatt-client.c
index 234f46ed7..55aa5e423 100644
--- a/src/gatt-client.c
+++ b/src/gatt-client.c
@@ -1645,13 +1645,22 @@ static const GDBusMethodTable characteristic_methods[] = {
{ }
};

+static void remove_client(void *data)
+{
+ struct notify_client *ntfy_client = data;
+ struct btd_gatt_client *client = ntfy_client->chrc->service->client;
+
+ queue_remove(client->all_notify_clients, ntfy_client);
+
+ notify_client_unref(ntfy_client);
+}
+
static void characteristic_free(void *data)
{
struct characteristic *chrc = data;

/* List should be empty here */
queue_destroy(chrc->descs, NULL);
- queue_destroy(chrc->notify_clients, NULL);

if (chrc->write_io) {
queue_remove(chrc->service->client->ios, chrc->write_io->io);
@@ -1663,6 +1672,8 @@ static void characteristic_free(void *data)
pipe_io_destroy(chrc->notify_io);
}

+ queue_destroy(chrc->notify_clients, remove_client);
+
g_free(chrc->path);
free(chrc);
}
@@ -1715,16 +1726,6 @@ static struct characteristic *characteristic_create(
return chrc;
}

-static void remove_client(void *data)
-{
- struct notify_client *ntfy_client = data;
- struct btd_gatt_client *client = ntfy_client->chrc->service->client;
-
- queue_remove(client->all_notify_clients, ntfy_client);
-
- notify_client_unref(ntfy_client);
-}
-
static void unregister_characteristic(void *data)
{
struct characteristic *chrc = data;
@@ -1738,7 +1739,6 @@ static void unregister_characteristic(void *data)
if (chrc->write_op)
bt_gatt_client_cancel(gatt, chrc->write_op->id);

- queue_remove_all(chrc->notify_clients, NULL, NULL, remove_client);
queue_remove_all(chrc->descs, NULL, NULL, unregister_descriptor);

g_dbus_unregister_interface(btd_get_dbus_connection(), chrc->path,
--
2.17.2

2018-11-19 15:43:19

by Luiz Augusto von Dentz

Subject: [PATCH BlueZ 2/5] doc/gatt-api: Restrict supported file descriptors

From: Luiz Augusto von Dentz <[email protected]>

Only support sockets with AcquireWrite/AcquireNotify since pipe don't
work with sendmsg therefore MSG_NOSIGNAL cannot be used.
---
doc/gatt-api.txt | 12 ++++++------
1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/doc/gatt-api.txt b/doc/gatt-api.txt
index be4c60c6f..8dda60b8a 100644
--- a/doc/gatt-api.txt
+++ b/doc/gatt-api.txt
@@ -100,9 +100,9 @@ Methods array{byte} ReadValue(dict options)

fd, uint16 AcquireWrite(dict options) [optional]

- Acquire file descriptor and MTU for writing. Usage of
- WriteValue will be locked causing it to return
- NotPermitted error.
+ Acquire file descriptor and MTU for writing. Only
+ sockets are supported. Usage of WriteValue will be
+ locked causing it to return NotPermitted error.

For server the MTU returned shall be equal or smaller
than the negotiated MTU.
@@ -130,9 +130,9 @@ Methods array{byte} ReadValue(dict options)

fd, uint16 AcquireNotify(dict options) [optional]

- Acquire file descriptor and MTU for notify. Usage of
- StartNotify will be locked causing it to return
- NotPermitted error.
+ Acquire file descriptor and MTU for notify. Only
+ sockets are support. Usage of StartNotify will be locked
+ causing it to return NotPermitted error.

For server the MTU returned shall be equal or smaller
than the negotiated MTU.
--
2.17.2

2018-11-19 15:43:21

by Luiz Augusto von Dentz

Subject: [PATCH BlueZ 3/5] gatt: Switch from pipe2 to sockepair for Acquire*

From: Luiz Augusto von Dentz <[email protected]>

This enables to use sendmsg with MSG_NOSIGNAL.
---
src/gatt-client.c | 99 ++++++++++++++++++++++++++-------------------
src/gatt-database.c | 26 +++++++-----
2 files changed, 72 insertions(+), 53 deletions(-)

diff --git a/src/gatt-client.c b/src/gatt-client.c
index 55aa5e423..45ed3b170 100644
--- a/src/gatt-client.c
+++ b/src/gatt-client.c
@@ -90,7 +90,7 @@ struct async_dbus_op {
async_dbus_op_complete_t complete;
};

-struct pipe_io {
+struct sock_io {
DBusMessage *msg;
struct io *io;
void (*destroy)(void *data);
@@ -109,8 +109,8 @@ struct characteristic {
char *path;

unsigned int ready_id;
- struct pipe_io *write_io;
- struct pipe_io *notify_io;
+ struct sock_io *write_io;
+ struct sock_io *notify_io;

struct async_dbus_op *read_op;
struct async_dbus_op *write_op;
@@ -1054,19 +1054,30 @@ fail:
return btd_error_not_supported(msg);
}

-static bool chrc_pipe_read(struct io *io, void *user_data)
+static bool sock_read(struct io *io, void *user_data)
{
struct characteristic *chrc = user_data;
struct bt_gatt_client *gatt = chrc->service->client->gatt;
+ struct msghdr msg;
uint8_t buf[512];
+ struct iovec iov;
int fd = io_get_fd(io);
ssize_t bytes_read;

- bytes_read = read(fd, buf, sizeof(buf));
- if (bytes_read < 0)
+ iov.iov_base = buf;
+ iov.iov_len = sizeof(buf);
+
+ memset(&msg, 0, sizeof(msg));
+ msg.msg_iov = &iov;
+ msg.msg_iovlen = 1;
+
+ bytes_read = recvmsg(fd, &msg, MSG_DONTWAIT);
+ if (bytes_read < 0) {
+ error("recvmsg: %s", strerror(errno));
return false;
+ }

- if (!gatt)
+ if (!gatt || bytes_read == 0)
return false;

bt_gatt_client_write_without_response(gatt, chrc->value_handle,
@@ -1076,7 +1087,7 @@ static bool chrc_pipe_read(struct io *io, void *user_data)
return true;
}

-static void pipe_io_destroy(struct pipe_io *io)
+static void sock_io_destroy(struct sock_io *io)
{
if (io->destroy)
io->destroy(io->data);
@@ -1088,20 +1099,19 @@ static void pipe_io_destroy(struct pipe_io *io)
free(io);
}

-static void characteristic_destroy_pipe(struct characteristic *chrc,
- struct io *io)
+static void destroy_sock(struct characteristic *chrc, struct io *io)
{
queue_remove(chrc->service->client->ios, io);

if (chrc->write_io && io == chrc->write_io->io) {
- pipe_io_destroy(chrc->write_io);
+ sock_io_destroy(chrc->write_io);
chrc->write_io = NULL;
g_dbus_emit_property_changed(btd_get_dbus_connection(),
chrc->path,
GATT_CHARACTERISTIC_IFACE,
"WriteAcquired");
} else if (chrc->notify_io) {
- pipe_io_destroy(chrc->notify_io);
+ sock_io_destroy(chrc->notify_io);
chrc->notify_io = NULL;
g_dbus_emit_property_changed(btd_get_dbus_connection(),
chrc->path,
@@ -1110,22 +1120,21 @@ static void characteristic_destroy_pipe(struct characteristic *chrc,
}
}

-static bool characteristic_pipe_hup(struct io *io, void *user_data)
+static bool sock_hup(struct io *io, void *user_data)
{
struct characteristic *chrc = user_data;

DBG("%s: io %p", chrc->path, io);

- characteristic_destroy_pipe(chrc, io);
+ destroy_sock(chrc, io);

return false;
}

-static DBusMessage *characteristic_create_pipe(struct characteristic *chrc,
- DBusMessage *msg)
+static DBusMessage *create_sock(struct characteristic *chrc, DBusMessage *msg)
{
struct bt_gatt_client *gatt = chrc->service->client->gatt;
- int pipefd[2];
+ int fds[2];
struct io *io;
bool dir;
uint16_t mtu;
@@ -1134,33 +1143,34 @@ static DBusMessage *characteristic_create_pipe(struct characteristic *chrc,
if (!gatt || !bt_gatt_client_is_ready(gatt))
return btd_error_failed(msg, "Not connected");

- if (pipe2(pipefd, O_DIRECT | O_NONBLOCK | O_CLOEXEC) < 0)
+ if (socketpair(AF_LOCAL, SOCK_SEQPACKET | SOCK_NONBLOCK | SOCK_CLOEXEC,
+ 0, fds) < 0)
return btd_error_failed(msg, strerror(errno));

dir = dbus_message_has_member(msg, "AcquireWrite");

- io = io_new(pipefd[!dir]);
+ io = io_new(fds[!dir]);
if (!io) {
- close(pipefd[0]);
- close(pipefd[1]);
+ close(fds[0]);
+ close(fds[1]);
return btd_error_failed(msg, strerror(EIO));
}

io_set_close_on_destroy(io, true);

- if (!io_set_read_handler(io, chrc_pipe_read, chrc, NULL))
+ if (!io_set_read_handler(io, sock_read, chrc, NULL))
goto fail;

- if (!io_set_disconnect_handler(io, characteristic_pipe_hup, chrc, NULL))
+ if (!io_set_disconnect_handler(io, sock_hup, chrc, NULL))
goto fail;

mtu = bt_gatt_client_get_mtu(gatt);

- reply = g_dbus_create_reply(msg, DBUS_TYPE_UNIX_FD, &pipefd[dir],
+ reply = g_dbus_create_reply(msg, DBUS_TYPE_UNIX_FD, &fds[dir],
DBUS_TYPE_UINT16, &mtu,
DBUS_TYPE_INVALID);

- close(pipefd[dir]);
+ close(fds[dir]);

if (dir) {
chrc->write_io->io = io;
@@ -1185,7 +1195,7 @@ static DBusMessage *characteristic_create_pipe(struct characteristic *chrc,

fail:
io_destroy(io);
- close(pipefd[dir]);
+ close(fds[dir]);
return btd_error_failed(msg, strerror(EIO));
}

@@ -1197,7 +1207,7 @@ static void characteristic_ready(bool success, uint8_t ecode, void *user_data)
chrc->ready_id = 0;

if (chrc->write_io && chrc->write_io->msg) {
- reply = characteristic_create_pipe(chrc, chrc->write_io->msg);
+ reply = create_sock(chrc, chrc->write_io->msg);

g_dbus_send_message(btd_get_dbus_connection(), reply);

@@ -1206,7 +1216,7 @@ static void characteristic_ready(bool success, uint8_t ecode, void *user_data)
}

if (chrc->notify_io && chrc->notify_io->msg) {
- reply = characteristic_create_pipe(chrc, chrc->notify_io->msg);
+ reply = create_sock(chrc, chrc->notify_io->msg);

g_dbus_send_message(btd_get_dbus_connection(), reply);

@@ -1230,7 +1240,7 @@ static DBusMessage *characteristic_acquire_write(DBusConnection *conn,
if (!(chrc->props & BT_GATT_CHRC_PROP_WRITE_WITHOUT_RESP))
return btd_error_not_supported(msg);

- chrc->write_io = new0(struct pipe_io, 1);
+ chrc->write_io = new0(struct sock_io, 1);

if (!bt_gatt_client_is_ready(gatt)) {
/* GATT not ready, wait until it becomes ready */
@@ -1242,7 +1252,7 @@ static DBusMessage *characteristic_acquire_write(DBusConnection *conn,
return NULL;
}

- return characteristic_create_pipe(chrc, msg);
+ return create_sock(chrc, msg);
}

struct notify_client {
@@ -1414,21 +1424,26 @@ static void register_notify_cb(uint16_t att_ecode, void *user_data)
static void notify_io_cb(uint16_t value_handle, const uint8_t *value,
uint16_t length, void *user_data)
{
+ struct msghdr msg;
struct iovec iov;
struct notify_client *client = user_data;
struct characteristic *chrc = client->chrc;
int err;

- /* Drop notification if the pipe is not ready */
- if (!chrc->notify_io->io)
+ /* Drop notification if the sock is not ready */
+ if (!chrc->notify_io || !chrc->notify_io->io)
return;

iov.iov_base = (void *) value;
iov.iov_len = length;

- err = io_send(chrc->notify_io->io, &iov, 1);
+ memset(&msg, 0, sizeof(msg));
+ msg.msg_iov = &iov;
+ msg.msg_iovlen = 1;
+
+ err = sendmsg(io_get_fd(chrc->notify_io->io), &msg, MSG_NOSIGNAL);
if (err < 0)
- error("io_send: %s", strerror(-err));
+ error("sendmsg: %s", strerror(errno));
}

static void register_notify_io_cb(uint16_t att_ecode, void *user_data)
@@ -1499,7 +1514,7 @@ static DBusMessage *characteristic_acquire_notify(DBusConnection *conn,

queue_push_tail(chrc->notify_clients, client);

- chrc->notify_io = new0(struct pipe_io, 1);
+ chrc->notify_io = new0(struct sock_io, 1);
chrc->notify_io->data = client;
chrc->notify_io->msg = dbus_message_ref(msg);
chrc->notify_io->destroy = notify_io_destroy;
@@ -1586,16 +1601,16 @@ static DBusMessage *characteristic_stop_notify(DBusConnection *conn,
const char *sender = dbus_message_get_sender(msg);
struct notify_client *client;

+ if (chrc->notify_io) {
+ destroy_sock(chrc, chrc->notify_io->io);
+ return dbus_message_new_method_return(msg);
+ }
+
client = queue_remove_if(chrc->notify_clients, match_notify_sender,
(void *) sender);
if (!client)
return btd_error_failed(msg, "No notify session started");

- if (chrc->notify_io) {
- characteristic_destroy_pipe(chrc, chrc->notify_io->io);
- return dbus_message_new_method_return(msg);
- }
-
queue_remove(chrc->service->client->all_notify_clients, client);
bt_gatt_client_unregister_notify(gatt, client->notify_id);
update_notifying(chrc);
@@ -1664,12 +1679,12 @@ static void characteristic_free(void *data)

if (chrc->write_io) {
queue_remove(chrc->service->client->ios, chrc->write_io->io);
- pipe_io_destroy(chrc->write_io);
+ sock_io_destroy(chrc->write_io);
}

if (chrc->notify_io) {
queue_remove(chrc->service->client->ios, chrc->notify_io->io);
- pipe_io_destroy(chrc->notify_io);
+ sock_io_destroy(chrc->notify_io);
}

queue_destroy(chrc->notify_clients, remove_client);
diff --git a/src/gatt-database.c b/src/gatt-database.c
index ec584fc3c..c31afa0e9 100644
--- a/src/gatt-database.c
+++ b/src/gatt-database.c
@@ -2139,7 +2139,7 @@ static struct pending_op *send_write(struct btd_device *device,
return NULL;
}

-static bool pipe_hup(struct io *io, void *user_data)
+static bool sock_hup(struct io *io, void *user_data)
{
struct external_chrc *chrc = user_data;

@@ -2155,7 +2155,7 @@ static bool pipe_hup(struct io *io, void *user_data)
return false;
}

-static bool pipe_io_read(struct io *io, void *user_data)
+static bool sock_io_read(struct io *io, void *user_data)
{
struct external_chrc *chrc = user_data;
uint8_t buf[512];
@@ -2176,7 +2176,7 @@ static bool pipe_io_read(struct io *io, void *user_data)
return true;
}

-static struct io *pipe_io_new(int fd, void *user_data)
+static struct io *sock_io_new(int fd, void *user_data)
{
struct io *io;

@@ -2184,21 +2184,25 @@ static struct io *pipe_io_new(int fd, void *user_data)

io_set_close_on_destroy(io, true);

- io_set_read_handler(io, pipe_io_read, user_data, NULL);
+ io_set_read_handler(io, sock_io_read, user_data, NULL);

- io_set_disconnect_handler(io, pipe_hup, user_data, NULL);
+ io_set_disconnect_handler(io, sock_hup, user_data, NULL);

return io;
}

-static int pipe_io_send(struct io *io, const void *data, size_t len)
+static int sock_io_send(struct io *io, const void *data, size_t len)
{
+ struct msghdr msg;
struct iovec iov;

iov.iov_base = (void *) data;
iov.iov_len = len;

- return io_send(io, &iov, 1);
+ memset(&msg, 0, sizeof(msg));
+ msg.msg_iov = &iov;
+
+ return sendmsg(io_get_fd(io), &msg, MSG_NOSIGNAL);
}

static void acquire_write_reply(DBusMessage *message, void *user_data)
@@ -2227,9 +2231,9 @@ static void acquire_write_reply(DBusMessage *message, void *user_data)

DBG("AcquireWrite success: fd %d MTU %u\n", fd, mtu);

- chrc->write_io = pipe_io_new(fd, chrc);
+ chrc->write_io = sock_io_new(fd, chrc);

- if (pipe_io_send(chrc->write_io, op->data.iov_base,
+ if (sock_io_send(chrc->write_io, op->data.iov_base,
op->data.iov_len) < 0)
goto retry;

@@ -2308,7 +2312,7 @@ static void acquire_notify_reply(DBusMessage *message, void *user_data)

DBG("AcquireNotify success: fd %d MTU %u\n", fd, mtu);

- chrc->notify_io = pipe_io_new(fd, chrc);
+ chrc->notify_io = sock_io_new(fd, chrc);

__sync_fetch_and_add(&chrc->ntfy_cnt, 1);

@@ -2685,7 +2689,7 @@ static void chrc_write_cb(struct gatt_db_attribute *attrib,
chrc->prep_authorized = false;

if (chrc->write_io) {
- if (pipe_io_send(chrc->write_io, value, len) < 0) {
+ if (sock_io_send(chrc->write_io, value, len) < 0) {
error("Unable to write: %s", strerror(errno));
goto fail;
}
--
2.17.2

2018-11-19 15:43:22

by Luiz Augusto von Dentz

Subject: [PATCH BlueZ 4/5] client: Switch from write to sendmsg for Acquire*

From: Luiz Augusto von Dentz <[email protected]>

Use sendmsg with MSG_NOSIGNAL to prevent crashes involving SIGPIPE.
---
client/gatt.c | 85 ++++++++++++++++++++++++++++++++++++---------------
1 file changed, 60 insertions(+), 25 deletions(-)

diff --git a/client/gatt.c b/client/gatt.c
index c7dfe42d7..9877c4b47 100644
--- a/client/gatt.c
+++ b/client/gatt.c
@@ -33,6 +33,8 @@
#include <sys/uio.h>
#include <fcntl.h>
#include <string.h>
+#include <sys/types.h>
+#include <sys/socket.h>

#include <glib.h>

@@ -99,14 +101,14 @@ static GList *managers;
static GList *uuids;
static DBusMessage *pending_message = NULL;

-struct pipe_io {
+struct sock_io {
GDBusProxy *proxy;
struct io *io;
uint16_t mtu;
};

-static struct pipe_io write_io;
-static struct pipe_io notify_io;
+static struct sock_io write_io;
+static struct sock_io notify_io;

static void print_service(struct service *service, const char *description)
{
@@ -635,6 +637,24 @@ static void write_setup(DBusMessageIter *iter, void *user_data)
dbus_message_iter_close_container(iter, &dict);
}

+static int sock_send(struct io *io, struct iovec *iov, size_t iovlen)
+{
+ struct msghdr msg;
+ int ret;
+
+ memset(&msg, 0, sizeof(msg));
+ msg.msg_iov = iov;
+ msg.msg_iovlen = iovlen;
+
+ ret = sendmsg(io_get_fd(io), &msg, MSG_NOSIGNAL);
+ if (ret < 0) {
+ ret = -errno;
+ bt_shell_printf("sendmsg: %s", strerror(-ret));
+ }
+
+ return ret;
+}
+
static void write_attribute(GDBusProxy *proxy, char *val_str, uint16_t offset)
{
struct iovec iov;
@@ -671,7 +691,7 @@ static void write_attribute(GDBusProxy *proxy, char *val_str, uint16_t offset)
if (proxy == write_io.proxy && (write_io.io && write_io.mtu >= i)) {
bt_shell_printf("Attempting to write fd %d\n",
io_get_fd(write_io.io));
- if (io_send(write_io.io, &iov, 1) < 0) {
+ if (sock_send(write_io.io, &iov, 1) < 0) {
bt_shell_printf("Failed to write: %s", strerror(errno));
return bt_shell_noninteractive_quit(EXIT_FAILURE);
}
@@ -713,9 +733,11 @@ void gatt_write_attribute(GDBusProxy *proxy, int argc, char *argv[])
return bt_shell_noninteractive_quit(EXIT_FAILURE);
}

-static bool pipe_read(struct io *io, void *user_data)
+static bool sock_read(struct io *io, void *user_data)
{
struct chrc *chrc = user_data;
+ struct msghdr msg;
+ struct iovec iov;
uint8_t buf[MAX_ATTR_VAL_LEN];
int fd = io_get_fd(io);
ssize_t bytes_read;
@@ -723,8 +745,20 @@ static bool pipe_read(struct io *io, void *user_data)
if (io != notify_io.io && !chrc)
return true;

- bytes_read = read(fd, buf, sizeof(buf));
- if (bytes_read < 0)
+ iov.iov_base = buf;
+ iov.iov_len = sizeof(buf);
+
+ memset(&msg, 0, sizeof(msg));
+ msg.msg_iov = &iov;
+ msg.msg_iovlen = 1;
+
+ bytes_read = recvmsg(fd, &msg, MSG_DONTWAIT);
+ if (bytes_read < 0) {
+ bt_shell_printf("recvmsg: %s", strerror(errno));
+ return false;
+ }
+
+ if (!bytes_read)
return false;

if (chrc)
@@ -739,12 +773,12 @@ static bool pipe_read(struct io *io, void *user_data)
return true;
}

-static bool pipe_hup(struct io *io, void *user_data)
+static bool sock_hup(struct io *io, void *user_data)
{
struct chrc *chrc = user_data;

if (chrc) {
- bt_shell_printf("Attribute %s %s pipe closed\n", chrc->path,
+ bt_shell_printf("Attribute %s %s sock closed\n", chrc->path,
io == chrc->write_io ? "Write" : "Notify");

if (io == chrc->write_io) {
@@ -768,7 +802,7 @@ static bool pipe_hup(struct io *io, void *user_data)
return false;
}

-static struct io *pipe_io_new(int fd, void *user_data)
+static struct io *sock_io_new(int fd, void *user_data)
{
struct io *io;

@@ -776,9 +810,9 @@ static struct io *pipe_io_new(int fd, void *user_data)

io_set_close_on_destroy(io, true);

- io_set_read_handler(io, pipe_read, user_data, NULL);
+ io_set_read_handler(io, sock_read, user_data, NULL);

- io_set_disconnect_handler(io, pipe_hup, user_data, NULL);
+ io_set_disconnect_handler(io, sock_hup, user_data, NULL);

return io;
}
@@ -810,7 +844,7 @@ static void acquire_write_reply(DBusMessage *message, void *user_data)
bt_shell_printf("AcquireWrite success: fd %d MTU %u\n", fd,
write_io.mtu);

- write_io.io = pipe_io_new(fd, NULL);
+ write_io.io = sock_io_new(fd, NULL);
return bt_shell_noninteractive_quit(EXIT_SUCCESS);
}

@@ -892,7 +926,7 @@ static void acquire_notify_reply(DBusMessage *message, void *user_data)
bt_shell_printf("AcquireNotify success: fd %d MTU %u\n", fd,
notify_io.mtu);

- notify_io.io = pipe_io_new(fd, NULL);
+ notify_io.io = sock_io_new(fd, NULL);

return bt_shell_noninteractive_quit(EXIT_SUCCESS);
}
@@ -1932,39 +1966,40 @@ static DBusMessage *chrc_write_value(DBusConnection *conn, DBusMessage *msg,
return g_dbus_create_reply(msg, DBUS_TYPE_INVALID);
}

-static DBusMessage *chrc_create_pipe(struct chrc *chrc, DBusMessage *msg)
+static DBusMessage *create_sock(struct chrc *chrc, DBusMessage *msg)
{
- int pipefd[2];
+ int fds[2];
struct io *io;
bool dir;
DBusMessage *reply;

- if (pipe2(pipefd, O_DIRECT | O_NONBLOCK | O_CLOEXEC) < 0)
+ if (socketpair(AF_LOCAL, SOCK_SEQPACKET | SOCK_NONBLOCK | SOCK_CLOEXEC,
+ 0, fds) < 0)
return g_dbus_create_error(msg, "org.bluez.Error.Failed", "%s",
strerror(errno));

dir = dbus_message_has_member(msg, "AcquireWrite");

- io = pipe_io_new(pipefd[!dir], chrc);
+ io = sock_io_new(fds[!dir], chrc);
if (!io) {
- close(pipefd[0]);
- close(pipefd[1]);
+ close(fds[0]);
+ close(fds[1]);
return g_dbus_create_error(msg, "org.bluez.Error.Failed", "%s",
strerror(errno));
}

- reply = g_dbus_create_reply(msg, DBUS_TYPE_UNIX_FD, &pipefd[dir],
+ reply = g_dbus_create_reply(msg, DBUS_TYPE_UNIX_FD, &fds[dir],
DBUS_TYPE_UINT16, &chrc->mtu,
DBUS_TYPE_INVALID);

- close(pipefd[dir]);
+ close(fds[dir]);

if (dir)
chrc->write_io = io;
else
chrc->notify_io = io;

- bt_shell_printf("[" COLORED_CHG "] Attribute %s %s pipe acquired\n",
+ bt_shell_printf("[" COLORED_CHG "] Attribute %s %s sock acquired\n",
chrc->path, dir ? "Write" : "Notify");

return reply;
@@ -1993,7 +2028,7 @@ static DBusMessage *chrc_acquire_write(DBusConnection *conn, DBusMessage *msg,
bt_shell_printf("AcquireWrite: %s link %s\n", path_to_address(device),
link);

- reply = chrc_create_pipe(chrc, msg);
+ reply = create_sock(chrc, msg);

if (chrc->write_io)
g_dbus_emit_property_changed(conn, chrc->path, CHRC_INTERFACE,
@@ -2025,7 +2060,7 @@ static DBusMessage *chrc_acquire_notify(DBusConnection *conn, DBusMessage *msg,
bt_shell_printf("AcquireNotify: %s link %s\n", path_to_address(device),
link);

- reply = chrc_create_pipe(chrc, msg);
+ reply = create_sock(chrc, msg);

if (chrc->notify_io)
g_dbus_emit_property_changed(conn, chrc->path, CHRC_INTERFACE,
--
2.17.2

2018-11-19 15:43:24

by Luiz Augusto von Dentz

Subject: [PATCH BlueZ 5/5] meshctl: Switch from write to sendmsg for Acquire*

From: Luiz Augusto von Dentz <[email protected]>

Use sendmsg with MSG_NOSIGNAL to prevent crashes involving SIGPIPE.
---
tools/mesh/gatt.c | 64 +++++++++++++++++++++++++++++++++--------------
1 file changed, 45 insertions(+), 19 deletions(-)

diff --git a/tools/mesh/gatt.c b/tools/mesh/gatt.c
index 2c29c09b5..2269a20cf 100644
--- a/tools/mesh/gatt.c
+++ b/tools/mesh/gatt.c
@@ -166,7 +166,25 @@ uint16_t mesh_gatt_sar(uint8_t **pkt, uint16_t size)
}
}

-static bool pipe_write(struct io *io, void *user_data)
+static int sock_send(struct io *io, struct iovec *iov, size_t iovlen)
+{
+ struct msghdr msg;
+ int ret;
+
+ memset(&msg, 0, sizeof(msg));
+ msg.msg_iov = iov;
+ msg.msg_iovlen = iovlen;
+
+ ret = sendmsg(io_get_fd(io), &msg, MSG_NOSIGNAL);
+ if (ret < 0) {
+ ret = -errno;
+ bt_shell_printf("sendmsg: %s", strerror(-ret));
+ }
+
+ return ret;
+}
+
+static bool sock_write(struct io *io, void *user_data)
{
struct write_data *data = user_data;
struct iovec iov[2];
@@ -200,9 +218,8 @@ static bool pipe_write(struct io *io, void *user_data)

iov[1] = data->iov;

- err = io_send(io, iov, 2);
+ err = sock_send(io, iov, 2);
if (err < 0) {
- bt_shell_printf("Failed to write: %s\n", strerror(-err));
write_data_free(data);
return false;
}
@@ -247,7 +264,7 @@ static void notify_io_destroy(void)
notify_mtu = 0;
}

-static bool pipe_hup(struct io *io, void *user_data)
+static bool sock_hup(struct io *io, void *user_data)
{
bt_shell_printf("%s closed\n", io == notify_io ? "Notify" : "Write");

@@ -259,7 +276,7 @@ static bool pipe_hup(struct io *io, void *user_data)
return false;
}

-static struct io *pipe_io_new(int fd)
+static struct io *sock_io_new(int fd)
{
struct io *io;

@@ -267,7 +284,7 @@ static struct io *pipe_io_new(int fd)

io_set_close_on_destroy(io, true);

- io_set_disconnect_handler(io, pipe_hup, NULL, NULL);
+ io_set_disconnect_handler(io, sock_hup, NULL, NULL);

return io;
}
@@ -296,9 +313,9 @@ static void acquire_write_reply(DBusMessage *message, void *user_data)

bt_shell_printf("AcquireWrite success: fd %d MTU %u\n", fd, write_mtu);

- write_io = pipe_io_new(fd);
+ write_io = sock_io_new(fd);

- pipe_write(write_io, data);
+ sock_write(write_io, data);
}

static void acquire_setup(DBusMessageIter *iter, void *user_data)
@@ -342,7 +359,7 @@ bool mesh_gatt_write(GDBusProxy *proxy, uint8_t *buf, uint16_t len,
data->cb = cb;

if (write_io)
- return pipe_write(write_io, data);
+ return sock_write(write_io, data);

if (g_dbus_proxy_method_call(proxy, "AcquireWrite",
acquire_setup, acquire_write_reply,
@@ -377,9 +394,11 @@ done:
g_free(data);
}

-static bool pipe_read(struct io *io, bool prov, void *user_data)
+static bool sock_read(struct io *io, bool prov, void *user_data)
{
struct mesh_node *node = user_data;
+ struct msghdr msg;
+ struct iovec iov;
uint8_t buf[512];
uint8_t *res;
int fd = io_get_fd(io);
@@ -388,9 +407,16 @@ static bool pipe_read(struct io *io, bool prov, void *user_data)
if (io != notify_io)
return true;

- while ((len = read(fd, buf, sizeof(buf)))) {
+ iov.iov_base = buf;
+ iov.iov_len = sizeof(buf);
+
+ memset(&msg, 0, sizeof(msg));
+ msg.msg_iov = &iov;
+ msg.msg_iovlen = 1;
+
+ while ((len = recvmsg(fd, &msg, MSG_DONTWAIT))) {
if (len <= 0)
- break;
+ return false;

res = buf;
len_sar = mesh_gatt_sar(&res, len);
@@ -404,14 +430,14 @@ static bool pipe_read(struct io *io, bool prov, void *user_data)
return true;
}

-static bool pipe_read_prov(struct io *io, void *user_data)
+static bool sock_read_prov(struct io *io, void *user_data)
{
- return pipe_read(io, true, user_data);
+ return sock_read(io, true, user_data);
}

-static bool pipe_read_proxy(struct io *io, void *user_data)
+static bool sock_read_proxy(struct io *io, void *user_data)
{
- return pipe_read(io, false, user_data);
+ return sock_read(io, false, user_data);
}

static void acquire_notify_reply(DBusMessage *message, void *user_data)
@@ -457,15 +483,15 @@ static void acquire_notify_reply(DBusMessage *message, void *user_data)
if (g_dbus_proxy_get_property(data->proxy, "UUID", &iter) == FALSE)
goto done;

- notify_io = pipe_io_new(fd);
+ notify_io = sock_io_new(fd);

dbus_message_iter_get_basic(&iter, &uuid);

if (!bt_uuid_strcmp(uuid, MESH_PROV_DATA_OUT_UUID_STR))
- io_set_read_handler(notify_io, pipe_read_prov, data->user_data,
+ io_set_read_handler(notify_io, sock_read_prov, data->user_data,
NULL);
else if (!bt_uuid_strcmp(uuid, MESH_PROXY_DATA_OUT_UUID_STR))
- io_set_read_handler(notify_io, pipe_read_proxy, data->user_data,
+ io_set_read_handler(notify_io, sock_read_proxy, data->user_data,
NULL);

done:
--
2.17.2

2018-11-20 08:51:54

by Luiz Augusto von Dentz

Subject: Re: [PATCH BlueZ 1/5] gatt: Fix invalid read when disconnecting

On Mon, Nov 19, 2018 at 5:43 PM Luiz Augusto von Dentz
<[email protected]> wrote:
>
> From: Luiz Augusto von Dentz <[email protected]>
>
> In case there is a client of AcquireNotify and a disconnect happens the
> code not only have to free the client object but also destroy the io
> associated with it, for this reason the client object cannot be freed
> until the io is destroyed otherwise it may lead to the following error:
>
> Invalid read of size 4
> at 0x63920: notify_io_destroy (gatt-client.c:1461)
> by 0x63EDB: pipe_io_destroy (gatt-client.c:1082)
> by 0x6405B: characteristic_free (gatt-client.c:1663)
> by 0x81F33: remove_interface (object.c:667)
> by 0x826CB: g_dbus_unregister_interface (object.c:1391)
> by 0x85D2B: queue_remove_all (queue.c:354)
> by 0x635F7: unregister_service (gatt-client.c:1893)
> by 0x85CF7: queue_remove_all (queue.c:339)
> by 0x661DF: btd_gatt_client_service_removed (gatt-client.c:2199)
> by 0x695CB: gatt_service_removed (device.c:3747)
> by 0x85B17: queue_foreach (queue.c:220)
> by 0x91283: notify_service_changed (gatt-db.c:280)
> by 0x91283: gatt_db_service_destroy (gatt-db.c:291)
> Address 0x515ed48 is 0 bytes inside a block of size 20 free'd
> at 0x483EAD0: free (vg_replace_malloc.c:530)
> by 0x85D2B: queue_remove_all (queue.c:354)
> by 0x636D3: unregister_characteristic (gatt-client.c:1741)
> by 0x85D2B: queue_remove_all (queue.c:354)
> by 0x635F7: unregister_service (gatt-client.c:1893)
> by 0x85CF7: queue_remove_all (queue.c:339)
> by 0x661DF: btd_gatt_client_service_removed (gatt-client.c:2199)
> by 0x695CB: gatt_service_removed (device.c:3747)
> by 0x85B17: queue_foreach (queue.c:220)
> by 0x91283: notify_service_changed (gatt-db.c:280)
> by 0x91283: gatt_db_service_destroy (gatt-db.c:291)
> by 0x85D2B: queue_remove_all (queue.c:354)
> by 0x91387: gatt_db_clear_range (gatt-db.c:475)
> ---
> src/gatt-client.c | 24 ++++++++++++------------
> 1 file changed, 12 insertions(+), 12 deletions(-)
>
> diff --git a/src/gatt-client.c b/src/gatt-client.c
> index 234f46ed7..55aa5e423 100644
> --- a/src/gatt-client.c
> +++ b/src/gatt-client.c
> @@ -1645,13 +1645,22 @@ static const GDBusMethodTable characteristic_methods[] = {
> { }
> };
>
> +static void remove_client(void *data)
> +{
> + struct notify_client *ntfy_client = data;
> + struct btd_gatt_client *client = ntfy_client->chrc->service->client;
> +
> + queue_remove(client->all_notify_clients, ntfy_client);
> +
> + notify_client_unref(ntfy_client);
> +}
> +
> static void characteristic_free(void *data)
> {
> struct characteristic *chrc = data;
>
> /* List should be empty here */
> queue_destroy(chrc->descs, NULL);
> - queue_destroy(chrc->notify_clients, NULL);
>
> if (chrc->write_io) {
> queue_remove(chrc->service->client->ios, chrc->write_io->io);
> @@ -1663,6 +1672,8 @@ static void characteristic_free(void *data)
> pipe_io_destroy(chrc->notify_io);
> }
>
> + queue_destroy(chrc->notify_clients, remove_client);
> +
> g_free(chrc->path);
> free(chrc);
> }
> @@ -1715,16 +1726,6 @@ static struct characteristic *characteristic_create(
> return chrc;
> }
>
> -static void remove_client(void *data)
> -{
> - struct notify_client *ntfy_client = data;
> - struct btd_gatt_client *client = ntfy_client->chrc->service->client;
> -
> - queue_remove(client->all_notify_clients, ntfy_client);
> -
> - notify_client_unref(ntfy_client);
> -}
> -
> static void unregister_characteristic(void *data)
> {
> struct characteristic *chrc = data;
> @@ -1738,7 +1739,6 @@ static void unregister_characteristic(void *data)
> if (chrc->write_op)
> bt_gatt_client_cancel(gatt, chrc->write_op->id);
>
> - queue_remove_all(chrc->notify_clients, NULL, NULL, remove_client);
> queue_remove_all(chrc->descs, NULL, NULL, unregister_descriptor);
>
> g_dbus_unregister_interface(btd_get_dbus_connection(), chrc->path,
> --
> 2.17.2

Can you try with this set to see it does work properly.

--
Luiz Augusto von Dentz

2018-11-20 09:31:07

by Gal Ben Haim

Subject: Re: [PATCH BlueZ 1/5] gatt: Fix invalid read when disconnecting

I can't apply this patch, not to the revision in master and not to the 5.50 tag

On Tue, Nov 20, 2018 at 10:50 AM Luiz Augusto von Dentz
<[email protected]> wrote:
>
> On Mon, Nov 19, 2018 at 5:43 PM Luiz Augusto von Dentz
> <[email protected]> wrote:
> >
> > From: Luiz Augusto von Dentz <[email protected]>
> >
> > In case there is a client of AcquireNotify and a disconnect happens the
> > code not only have to free the client object but also destroy the io
> > associated with it, for this reason the client object cannot be freed
> > until the io is destroyed otherwise it may lead to the following error:
> >
> > Invalid read of size 4
> > at 0x63920: notify_io_destroy (gatt-client.c:1461)
> > by 0x63EDB: pipe_io_destroy (gatt-client.c:1082)
> > by 0x6405B: characteristic_free (gatt-client.c:1663)
> > by 0x81F33: remove_interface (object.c:667)
> > by 0x826CB: g_dbus_unregister_interface (object.c:1391)
> > by 0x85D2B: queue_remove_all (queue.c:354)
> > by 0x635F7: unregister_service (gatt-client.c:1893)
> > by 0x85CF7: queue_remove_all (queue.c:339)
> > by 0x661DF: btd_gatt_client_service_removed (gatt-client.c:2199)
> > by 0x695CB: gatt_service_removed (device.c:3747)
> > by 0x85B17: queue_foreach (queue.c:220)
> > by 0x91283: notify_service_changed (gatt-db.c:280)
> > by 0x91283: gatt_db_service_destroy (gatt-db.c:291)
> > Address 0x515ed48 is 0 bytes inside a block of size 20 free'd
> > at 0x483EAD0: free (vg_replace_malloc.c:530)
> > by 0x85D2B: queue_remove_all (queue.c:354)
> > by 0x636D3: unregister_characteristic (gatt-client.c:1741)
> > by 0x85D2B: queue_remove_all (queue.c:354)
> > by 0x635F7: unregister_service (gatt-client.c:1893)
> > by 0x85CF7: queue_remove_all (queue.c:339)
> > by 0x661DF: btd_gatt_client_service_removed (gatt-client.c:2199)
> > by 0x695CB: gatt_service_removed (device.c:3747)
> > by 0x85B17: queue_foreach (queue.c:220)
> > by 0x91283: notify_service_changed (gatt-db.c:280)
> > by 0x91283: gatt_db_service_destroy (gatt-db.c:291)
> > by 0x85D2B: queue_remove_all (queue.c:354)
> > by 0x91387: gatt_db_clear_range (gatt-db.c:475)
> > ---
> > src/gatt-client.c | 24 ++++++++++++------------
> > 1 file changed, 12 insertions(+), 12 deletions(-)
> >
> > diff --git a/src/gatt-client.c b/src/gatt-client.c
> > index 234f46ed7..55aa5e423 100644
> > --- a/src/gatt-client.c
> > +++ b/src/gatt-client.c
> > @@ -1645,13 +1645,22 @@ static const GDBusMethodTable characteristic_methods[] = {
> > { }
> > };
> >
> > +static void remove_client(void *data)
> > +{
> > + struct notify_client *ntfy_client = data;
> > + struct btd_gatt_client *client = ntfy_client->chrc->service->client;
> > +
> > + queue_remove(client->all_notify_clients, ntfy_client);
> > +
> > + notify_client_unref(ntfy_client);
> > +}
> > +
> > static void characteristic_free(void *data)
> > {
> > struct characteristic *chrc = data;
> >
> > /* List should be empty here */
> > queue_destroy(chrc->descs, NULL);
> > - queue_destroy(chrc->notify_clients, NULL);
> >
> > if (chrc->write_io) {
> > queue_remove(chrc->service->client->ios, chrc->write_io->io);
> > @@ -1663,6 +1672,8 @@ static void characteristic_free(void *data)
> > pipe_io_destroy(chrc->notify_io);
> > }
> >
> > + queue_destroy(chrc->notify_clients, remove_client);
> > +
> > g_free(chrc->path);
> > free(chrc);
> > }
> > @@ -1715,16 +1726,6 @@ static struct characteristic *characteristic_create(
> > return chrc;
> > }
> >
> > -static void remove_client(void *data)
> > -{
> > - struct notify_client *ntfy_client = data;
> > - struct btd_gatt_client *client = ntfy_client->chrc->service->client;
> > -
> > - queue_remove(client->all_notify_clients, ntfy_client);
> > -
> > - notify_client_unref(ntfy_client);
> > -}
> > -
> > static void unregister_characteristic(void *data)
> > {
> > struct characteristic *chrc = data;
> > @@ -1738,7 +1739,6 @@ static void unregister_characteristic(void *data)
> > if (chrc->write_op)
> > bt_gatt_client_cancel(gatt, chrc->write_op->id);
> >
> > - queue_remove_all(chrc->notify_clients, NULL, NULL, remove_client);
> > queue_remove_all(chrc->descs, NULL, NULL, unregister_descriptor);
> >
> > g_dbus_unregister_interface(btd_get_dbus_connection(), chrc->path,
> > --
> > 2.17.2
>
> Can you try with this set to see it does work properly.
>
> --
> Luiz Augusto von Dentz

2018-11-20 10:35:35

by Luiz Augusto von Dentz

Subject: Re: [PATCH BlueZ 1/5] gatt: Fix invalid read when disconnecting

Hi Gal,
On Tue, Nov 20, 2018 at 11:31 AM Gal Ben Haim <[email protected]> wrote:
>
> I can't apply this patch, not to the revision in master and not to the 5.50 tag

They are on top o master, how are you trying to apply them, with git am?

> On Tue, Nov 20, 2018 at 10:50 AM Luiz Augusto von Dentz
> <[email protected]> wrote:
> >
> > On Mon, Nov 19, 2018 at 5:43 PM Luiz Augusto von Dentz
> > <[email protected]> wrote:
> > >
> > > From: Luiz Augusto von Dentz <[email protected]>
> > >
> > > In case there is a client of AcquireNotify and a disconnect happens the
> > > code not only have to free the client object but also destroy the io
> > > associated with it, for this reason the client object cannot be freed
> > > until the io is destroyed otherwise it may lead to the following error:
> > >
> > > Invalid read of size 4
> > > at 0x63920: notify_io_destroy (gatt-client.c:1461)
> > > by 0x63EDB: pipe_io_destroy (gatt-client.c:1082)
> > > by 0x6405B: characteristic_free (gatt-client.c:1663)
> > > by 0x81F33: remove_interface (object.c:667)
> > > by 0x826CB: g_dbus_unregister_interface (object.c:1391)
> > > by 0x85D2B: queue_remove_all (queue.c:354)
> > > by 0x635F7: unregister_service (gatt-client.c:1893)
> > > by 0x85CF7: queue_remove_all (queue.c:339)
> > > by 0x661DF: btd_gatt_client_service_removed (gatt-client.c:2199)
> > > by 0x695CB: gatt_service_removed (device.c:3747)
> > > by 0x85B17: queue_foreach (queue.c:220)
> > > by 0x91283: notify_service_changed (gatt-db.c:280)
> > > by 0x91283: gatt_db_service_destroy (gatt-db.c:291)
> > > Address 0x515ed48 is 0 bytes inside a block of size 20 free'd
> > > at 0x483EAD0: free (vg_replace_malloc.c:530)
> > > by 0x85D2B: queue_remove_all (queue.c:354)
> > > by 0x636D3: unregister_characteristic (gatt-client.c:1741)
> > > by 0x85D2B: queue_remove_all (queue.c:354)
> > > by 0x635F7: unregister_service (gatt-client.c:1893)
> > > by 0x85CF7: queue_remove_all (queue.c:339)
> > > by 0x661DF: btd_gatt_client_service_removed (gatt-client.c:2199)
> > > by 0x695CB: gatt_service_removed (device.c:3747)
> > > by 0x85B17: queue_foreach (queue.c:220)
> > > by 0x91283: notify_service_changed (gatt-db.c:280)
> > > by 0x91283: gatt_db_service_destroy (gatt-db.c:291)
> > > by 0x85D2B: queue_remove_all (queue.c:354)
> > > by 0x91387: gatt_db_clear_range (gatt-db.c:475)
> > > ---
> > > src/gatt-client.c | 24 ++++++++++++------------
> > > 1 file changed, 12 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/src/gatt-client.c b/src/gatt-client.c
> > > index 234f46ed7..55aa5e423 100644
> > > --- a/src/gatt-client.c
> > > +++ b/src/gatt-client.c
> > > @@ -1645,13 +1645,22 @@ static const GDBusMethodTable characteristic_methods[] = {
> > > { }
> > > };
> > >
> > > +static void remove_client(void *data)
> > > +{
> > > + struct notify_client *ntfy_client = data;
> > > + struct btd_gatt_client *client = ntfy_client->chrc->service->client;
> > > +
> > > + queue_remove(client->all_notify_clients, ntfy_client);
> > > +
> > > + notify_client_unref(ntfy_client);
> > > +}
> > > +
> > > static void characteristic_free(void *data)
> > > {
> > > struct characteristic *chrc = data;
> > >
> > > /* List should be empty here */
> > > queue_destroy(chrc->descs, NULL);
> > > - queue_destroy(chrc->notify_clients, NULL);
> > >
> > > if (chrc->write_io) {
> > > queue_remove(chrc->service->client->ios, chrc->write_io->io);
> > > @@ -1663,6 +1672,8 @@ static void characteristic_free(void *data)
> > > pipe_io_destroy(chrc->notify_io);
> > > }
> > >
> > > + queue_destroy(chrc->notify_clients, remove_client);
> > > +
> > > g_free(chrc->path);
> > > free(chrc);
> > > }
> > > @@ -1715,16 +1726,6 @@ static struct characteristic *characteristic_create(
> > > return chrc;
> > > }
> > >
> > > -static void remove_client(void *data)
> > > -{
> > > - struct notify_client *ntfy_client = data;
> > > - struct btd_gatt_client *client = ntfy_client->chrc->service->client;
> > > -
> > > - queue_remove(client->all_notify_clients, ntfy_client);
> > > -
> > > - notify_client_unref(ntfy_client);
> > > -}
> > > -
> > > static void unregister_characteristic(void *data)
> > > {
> > > struct characteristic *chrc = data;
> > > @@ -1738,7 +1739,6 @@ static void unregister_characteristic(void *data)
> > > if (chrc->write_op)
> > > bt_gatt_client_cancel(gatt, chrc->write_op->id);
> > >
> > > - queue_remove_all(chrc->notify_clients, NULL, NULL, remove_client);
> > > queue_remove_all(chrc->descs, NULL, NULL, unregister_descriptor);
> > >
> > > g_dbus_unregister_interface(btd_get_dbus_connection(), chrc->path,
> > > --
> > > 2.17.2
> >
> > Can you try with this set to see it does work properly.
> >
> > --
> > Luiz Augusto von Dentz



--
Luiz Augusto von Dentz

2018-11-20 12:36:08

by Gal Ben Haim

Subject: Re: [PATCH BlueZ 1/5] gatt: Fix invalid read when disconnecting

i tried with both git am and git apply

On Tue, Nov 20, 2018 at 12:35 PM Luiz Augusto von Dentz
<[email protected]> wrote:
>
> Hi Gal,
> On Tue, Nov 20, 2018 at 11:31 AM Gal Ben Haim <[email protected]> wrote:
> >
> > I can't apply this patch, not to the revision in master and not to the 5.50 tag
>
> They are on top o master, how are you trying to apply them, with git am?
>
> > On Tue, Nov 20, 2018 at 10:50 AM Luiz Augusto von Dentz
> > <[email protected]> wrote:
> > >
> > > On Mon, Nov 19, 2018 at 5:43 PM Luiz Augusto von Dentz
> > > <[email protected]> wrote:
> > > >
> > > > From: Luiz Augusto von Dentz <[email protected]>
> > > >
> > > > In case there is a client of AcquireNotify and a disconnect happens the
> > > > code not only have to free the client object but also destroy the io
> > > > associated with it, for this reason the client object cannot be freed
> > > > until the io is destroyed otherwise it may lead to the following error:
> > > >
> > > > Invalid read of size 4
> > > > at 0x63920: notify_io_destroy (gatt-client.c:1461)
> > > > by 0x63EDB: pipe_io_destroy (gatt-client.c:1082)
> > > > by 0x6405B: characteristic_free (gatt-client.c:1663)
> > > > by 0x81F33: remove_interface (object.c:667)
> > > > by 0x826CB: g_dbus_unregister_interface (object.c:1391)
> > > > by 0x85D2B: queue_remove_all (queue.c:354)
> > > > by 0x635F7: unregister_service (gatt-client.c:1893)
> > > > by 0x85CF7: queue_remove_all (queue.c:339)
> > > > by 0x661DF: btd_gatt_client_service_removed (gatt-client.c:2199)
> > > > by 0x695CB: gatt_service_removed (device.c:3747)
> > > > by 0x85B17: queue_foreach (queue.c:220)
> > > > by 0x91283: notify_service_changed (gatt-db.c:280)
> > > > by 0x91283: gatt_db_service_destroy (gatt-db.c:291)
> > > > Address 0x515ed48 is 0 bytes inside a block of size 20 free'd
> > > > at 0x483EAD0: free (vg_replace_malloc.c:530)
> > > > by 0x85D2B: queue_remove_all (queue.c:354)
> > > > by 0x636D3: unregister_characteristic (gatt-client.c:1741)
> > > > by 0x85D2B: queue_remove_all (queue.c:354)
> > > > by 0x635F7: unregister_service (gatt-client.c:1893)
> > > > by 0x85CF7: queue_remove_all (queue.c:339)
> > > > by 0x661DF: btd_gatt_client_service_removed (gatt-client.c:2199)
> > > > by 0x695CB: gatt_service_removed (device.c:3747)
> > > > by 0x85B17: queue_foreach (queue.c:220)
> > > > by 0x91283: notify_service_changed (gatt-db.c:280)
> > > > by 0x91283: gatt_db_service_destroy (gatt-db.c:291)
> > > > by 0x85D2B: queue_remove_all (queue.c:354)
> > > > by 0x91387: gatt_db_clear_range (gatt-db.c:475)
> > > > ---
> > > > src/gatt-client.c | 24 ++++++++++++------------
> > > > 1 file changed, 12 insertions(+), 12 deletions(-)
> > > >
> > > > diff --git a/src/gatt-client.c b/src/gatt-client.c
> > > > index 234f46ed7..55aa5e423 100644
> > > > --- a/src/gatt-client.c
> > > > +++ b/src/gatt-client.c
> > > > @@ -1645,13 +1645,22 @@ static const GDBusMethodTable characteristic_methods[] = {
> > > > { }
> > > > };
> > > >
> > > > +static void remove_client(void *data)
> > > > +{
> > > > + struct notify_client *ntfy_client = data;
> > > > + struct btd_gatt_client *client = ntfy_client->chrc->service->client;
> > > > +
> > > > + queue_remove(client->all_notify_clients, ntfy_client);
> > > > +
> > > > + notify_client_unref(ntfy_client);
> > > > +}
> > > > +
> > > > static void characteristic_free(void *data)
> > > > {
> > > > struct characteristic *chrc = data;
> > > >
> > > > /* List should be empty here */
> > > > queue_destroy(chrc->descs, NULL);
> > > > - queue_destroy(chrc->notify_clients, NULL);
> > > >
> > > > if (chrc->write_io) {
> > > > queue_remove(chrc->service->client->ios, chrc->write_io->io);
> > > > @@ -1663,6 +1672,8 @@ static void characteristic_free(void *data)
> > > > pipe_io_destroy(chrc->notify_io);
> > > > }
> > > >
> > > > + queue_destroy(chrc->notify_clients, remove_client);
> > > > +
> > > > g_free(chrc->path);
> > > > free(chrc);
> > > > }
> > > > @@ -1715,16 +1726,6 @@ static struct characteristic *characteristic_create(
> > > > return chrc;
> > > > }
> > > >
> > > > -static void remove_client(void *data)
> > > > -{
> > > > - struct notify_client *ntfy_client = data;
> > > > - struct btd_gatt_client *client = ntfy_client->chrc->service->client;
> > > > -
> > > > - queue_remove(client->all_notify_clients, ntfy_client);
> > > > -
> > > > - notify_client_unref(ntfy_client);
> > > > -}
> > > > -
> > > > static void unregister_characteristic(void *data)
> > > > {
> > > > struct characteristic *chrc = data;
> > > > @@ -1738,7 +1739,6 @@ static void unregister_characteristic(void *data)
> > > > if (chrc->write_op)
> > > > bt_gatt_client_cancel(gatt, chrc->write_op->id);
> > > >
> > > > - queue_remove_all(chrc->notify_clients, NULL, NULL, remove_client);
> > > > queue_remove_all(chrc->descs, NULL, NULL, unregister_descriptor);
> > > >
> > > > g_dbus_unregister_interface(btd_get_dbus_connection(), chrc->path,
> > > > --
> > > > 2.17.2
> > >
> > > Can you try with this set to see it does work properly.
> > >
> > > --
> > > Luiz Augusto von Dentz
>
>
>
> --
> Luiz Augusto von Dentz

2018-11-20 14:00:09

by Luiz Augusto von Dentz

Subject: Re: [PATCH BlueZ 1/5] gatt: Fix invalid read when disconnecting

Hi Gal,

On Tue, Nov 20, 2018 at 2:35 PM Gal Ben Haim <[email protected]> wrote:
>
> i tried with both git am and git apply

Does that applies to v2 as well? I just rebased it on top master, are
you sure you don't have your own changes conflicting with it?

> On Tue, Nov 20, 2018 at 12:35 PM Luiz Augusto von Dentz
> <[email protected]> wrote:
> >
> > Hi Gal,
> > On Tue, Nov 20, 2018 at 11:31 AM Gal Ben Haim <[email protected]> wrote:
> > >
> > > I can't apply this patch, not to the revision in master and not to the 5.50 tag
> >
> > They are on top o master, how are you trying to apply them, with git am?
> >
> > > On Tue, Nov 20, 2018 at 10:50 AM Luiz Augusto von Dentz
> > > <[email protected]> wrote:
> > > >
> > > > On Mon, Nov 19, 2018 at 5:43 PM Luiz Augusto von Dentz
> > > > <[email protected]> wrote:
> > > > >
> > > > > From: Luiz Augusto von Dentz <[email protected]>
> > > > >
> > > > > In case there is a client of AcquireNotify and a disconnect happens the
> > > > > code not only have to free the client object but also destroy the io
> > > > > associated with it, for this reason the client object cannot be freed
> > > > > until the io is destroyed otherwise it may lead to the following error:
> > > > >
> > > > > Invalid read of size 4
> > > > > at 0x63920: notify_io_destroy (gatt-client.c:1461)
> > > > > by 0x63EDB: pipe_io_destroy (gatt-client.c:1082)
> > > > > by 0x6405B: characteristic_free (gatt-client.c:1663)
> > > > > by 0x81F33: remove_interface (object.c:667)
> > > > > by 0x826CB: g_dbus_unregister_interface (object.c:1391)
> > > > > by 0x85D2B: queue_remove_all (queue.c:354)
> > > > > by 0x635F7: unregister_service (gatt-client.c:1893)
> > > > > by 0x85CF7: queue_remove_all (queue.c:339)
> > > > > by 0x661DF: btd_gatt_client_service_removed (gatt-client.c:2199)
> > > > > by 0x695CB: gatt_service_removed (device.c:3747)
> > > > > by 0x85B17: queue_foreach (queue.c:220)
> > > > > by 0x91283: notify_service_changed (gatt-db.c:280)
> > > > > by 0x91283: gatt_db_service_destroy (gatt-db.c:291)
> > > > > Address 0x515ed48 is 0 bytes inside a block of size 20 free'd
> > > > > at 0x483EAD0: free (vg_replace_malloc.c:530)
> > > > > by 0x85D2B: queue_remove_all (queue.c:354)
> > > > > by 0x636D3: unregister_characteristic (gatt-client.c:1741)
> > > > > by 0x85D2B: queue_remove_all (queue.c:354)
> > > > > by 0x635F7: unregister_service (gatt-client.c:1893)
> > > > > by 0x85CF7: queue_remove_all (queue.c:339)
> > > > > by 0x661DF: btd_gatt_client_service_removed (gatt-client.c:2199)
> > > > > by 0x695CB: gatt_service_removed (device.c:3747)
> > > > > by 0x85B17: queue_foreach (queue.c:220)
> > > > > by 0x91283: notify_service_changed (gatt-db.c:280)
> > > > > by 0x91283: gatt_db_service_destroy (gatt-db.c:291)
> > > > > by 0x85D2B: queue_remove_all (queue.c:354)
> > > > > by 0x91387: gatt_db_clear_range (gatt-db.c:475)
> > > > > ---
> > > > > src/gatt-client.c | 24 ++++++++++++------------
> > > > > 1 file changed, 12 insertions(+), 12 deletions(-)
> > > > >
> > > > > diff --git a/src/gatt-client.c b/src/gatt-client.c
> > > > > index 234f46ed7..55aa5e423 100644
> > > > > --- a/src/gatt-client.c
> > > > > +++ b/src/gatt-client.c
> > > > > @@ -1645,13 +1645,22 @@ static const GDBusMethodTable characteristic_methods[] = {
> > > > > { }
> > > > > };
> > > > >
> > > > > +static void remove_client(void *data)
> > > > > +{
> > > > > + struct notify_client *ntfy_client = data;
> > > > > + struct btd_gatt_client *client = ntfy_client->chrc->service->client;
> > > > > +
> > > > > + queue_remove(client->all_notify_clients, ntfy_client);
> > > > > +
> > > > > + notify_client_unref(ntfy_client);
> > > > > +}
> > > > > +
> > > > > static void characteristic_free(void *data)
> > > > > {
> > > > > struct characteristic *chrc = data;
> > > > >
> > > > > /* List should be empty here */
> > > > > queue_destroy(chrc->descs, NULL);
> > > > > - queue_destroy(chrc->notify_clients, NULL);
> > > > >
> > > > > if (chrc->write_io) {
> > > > > queue_remove(chrc->service->client->ios, chrc->write_io->io);
> > > > > @@ -1663,6 +1672,8 @@ static void characteristic_free(void *data)
> > > > > pipe_io_destroy(chrc->notify_io);
> > > > > }
> > > > >
> > > > > + queue_destroy(chrc->notify_clients, remove_client);
> > > > > +
> > > > > g_free(chrc->path);
> > > > > free(chrc);
> > > > > }
> > > > > @@ -1715,16 +1726,6 @@ static struct characteristic *characteristic_create(
> > > > > return chrc;
> > > > > }
> > > > >
> > > > > -static void remove_client(void *data)
> > > > > -{
> > > > > - struct notify_client *ntfy_client = data;
> > > > > - struct btd_gatt_client *client = ntfy_client->chrc->service->client;
> > > > > -
> > > > > - queue_remove(client->all_notify_clients, ntfy_client);
> > > > > -
> > > > > - notify_client_unref(ntfy_client);
> > > > > -}
> > > > > -
> > > > > static void unregister_characteristic(void *data)
> > > > > {
> > > > > struct characteristic *chrc = data;
> > > > > @@ -1738,7 +1739,6 @@ static void unregister_characteristic(void *data)
> > > > > if (chrc->write_op)
> > > > > bt_gatt_client_cancel(gatt, chrc->write_op->id);
> > > > >
> > > > > - queue_remove_all(chrc->notify_clients, NULL, NULL, remove_client);
> > > > > queue_remove_all(chrc->descs, NULL, NULL, unregister_descriptor);
> > > > >
> > > > > g_dbus_unregister_interface(btd_get_dbus_connection(), chrc->path,
> > > > > --
> > > > > 2.17.2
> > > >
> > > > Can you try with this set to see it does work properly.
> > > >
> > > > --
> > > > Luiz Augusto von Dentz
> >
> >
> >
> > --
> > Luiz Augusto von Dentz



--
Luiz Augusto von Dentz

2018-11-20 18:32:07

by Gal Ben Haim

Subject: Re: [PATCH BlueZ 1/5] gatt: Fix invalid read when disconnecting

ok I patched it. i'll test it tomorrow in the office and let you know.

On Tue, Nov 20, 2018 at 3:59 PM Luiz Augusto von Dentz
<[email protected]> wrote:
>
> Hi Gal,
>
> On Tue, Nov 20, 2018 at 2:35 PM Gal Ben Haim <[email protected]> wrote:
> >
> > i tried with both git am and git apply
>
> Does that applies to v2 as well? I just rebased it on top master, are
> you sure you don't have your own changes conflicting with it?
>
> > On Tue, Nov 20, 2018 at 12:35 PM Luiz Augusto von Dentz
> > <[email protected]> wrote:
> > >
> > > Hi Gal,
> > > On Tue, Nov 20, 2018 at 11:31 AM Gal Ben Haim <[email protected]> wrote:
> > > >
> > > > I can't apply this patch, not to the revision in master and not to the 5.50 tag
> > >
> > > They are on top o master, how are you trying to apply them, with git am?
> > >
> > > > On Tue, Nov 20, 2018 at 10:50 AM Luiz Augusto von Dentz
> > > > <[email protected]> wrote:
> > > > >
> > > > > On Mon, Nov 19, 2018 at 5:43 PM Luiz Augusto von Dentz
> > > > > <[email protected]> wrote:
> > > > > >
> > > > > > From: Luiz Augusto von Dentz <[email protected]>
> > > > > >
> > > > > > In case there is a client of AcquireNotify and a disconnect happens the
> > > > > > code not only have to free the client object but also destroy the io
> > > > > > associated with it, for this reason the client object cannot be freed
> > > > > > until the io is destroyed otherwise it may lead to the following error:
> > > > > >
> > > > > > Invalid read of size 4
> > > > > > at 0x63920: notify_io_destroy (gatt-client.c:1461)
> > > > > > by 0x63EDB: pipe_io_destroy (gatt-client.c:1082)
> > > > > > by 0x6405B: characteristic_free (gatt-client.c:1663)
> > > > > > by 0x81F33: remove_interface (object.c:667)
> > > > > > by 0x826CB: g_dbus_unregister_interface (object.c:1391)
> > > > > > by 0x85D2B: queue_remove_all (queue.c:354)
> > > > > > by 0x635F7: unregister_service (gatt-client.c:1893)
> > > > > > by 0x85CF7: queue_remove_all (queue.c:339)
> > > > > > by 0x661DF: btd_gatt_client_service_removed (gatt-client.c:2199)
> > > > > > by 0x695CB: gatt_service_removed (device.c:3747)
> > > > > > by 0x85B17: queue_foreach (queue.c:220)
> > > > > > by 0x91283: notify_service_changed (gatt-db.c:280)
> > > > > > by 0x91283: gatt_db_service_destroy (gatt-db.c:291)
> > > > > > Address 0x515ed48 is 0 bytes inside a block of size 20 free'd
> > > > > > at 0x483EAD0: free (vg_replace_malloc.c:530)
> > > > > > by 0x85D2B: queue_remove_all (queue.c:354)
> > > > > > by 0x636D3: unregister_characteristic (gatt-client.c:1741)
> > > > > > by 0x85D2B: queue_remove_all (queue.c:354)
> > > > > > by 0x635F7: unregister_service (gatt-client.c:1893)
> > > > > > by 0x85CF7: queue_remove_all (queue.c:339)
> > > > > > by 0x661DF: btd_gatt_client_service_removed (gatt-client.c:2199)
> > > > > > by 0x695CB: gatt_service_removed (device.c:3747)
> > > > > > by 0x85B17: queue_foreach (queue.c:220)
> > > > > > by 0x91283: notify_service_changed (gatt-db.c:280)
> > > > > > by 0x91283: gatt_db_service_destroy (gatt-db.c:291)
> > > > > > by 0x85D2B: queue_remove_all (queue.c:354)
> > > > > > by 0x91387: gatt_db_clear_range (gatt-db.c:475)
> > > > > > ---
> > > > > > src/gatt-client.c | 24 ++++++++++++------------
> > > > > > 1 file changed, 12 insertions(+), 12 deletions(-)
> > > > > >
> > > > > > diff --git a/src/gatt-client.c b/src/gatt-client.c
> > > > > > index 234f46ed7..55aa5e423 100644
> > > > > > --- a/src/gatt-client.c
> > > > > > +++ b/src/gatt-client.c
> > > > > > @@ -1645,13 +1645,22 @@ static const GDBusMethodTable characteristic_methods[] = {
> > > > > > { }
> > > > > > };
> > > > > >
> > > > > > +static void remove_client(void *data)
> > > > > > +{
> > > > > > + struct notify_client *ntfy_client = data;
> > > > > > + struct btd_gatt_client *client = ntfy_client->chrc->service->client;
> > > > > > +
> > > > > > + queue_remove(client->all_notify_clients, ntfy_client);
> > > > > > +
> > > > > > + notify_client_unref(ntfy_client);
> > > > > > +}
> > > > > > +
> > > > > > static void characteristic_free(void *data)
> > > > > > {
> > > > > > struct characteristic *chrc = data;
> > > > > >
> > > > > > /* List should be empty here */
> > > > > > queue_destroy(chrc->descs, NULL);
> > > > > > - queue_destroy(chrc->notify_clients, NULL);
> > > > > >
> > > > > > if (chrc->write_io) {
> > > > > > queue_remove(chrc->service->client->ios, chrc->write_io->io);
> > > > > > @@ -1663,6 +1672,8 @@ static void characteristic_free(void *data)
> > > > > > pipe_io_destroy(chrc->notify_io);
> > > > > > }
> > > > > >
> > > > > > + queue_destroy(chrc->notify_clients, remove_client);
> > > > > > +
> > > > > > g_free(chrc->path);
> > > > > > free(chrc);
> > > > > > }
> > > > > > @@ -1715,16 +1726,6 @@ static struct characteristic *characteristic_create(
> > > > > > return chrc;
> > > > > > }
> > > > > >
> > > > > > -static void remove_client(void *data)
> > > > > > -{
> > > > > > - struct notify_client *ntfy_client = data;
> > > > > > - struct btd_gatt_client *client = ntfy_client->chrc->service->client;
> > > > > > -
> > > > > > - queue_remove(client->all_notify_clients, ntfy_client);
> > > > > > -
> > > > > > - notify_client_unref(ntfy_client);
> > > > > > -}
> > > > > > -
> > > > > > static void unregister_characteristic(void *data)
> > > > > > {
> > > > > > struct characteristic *chrc = data;
> > > > > > @@ -1738,7 +1739,6 @@ static void unregister_characteristic(void *data)
> > > > > > if (chrc->write_op)
> > > > > > bt_gatt_client_cancel(gatt, chrc->write_op->id);
> > > > > >
> > > > > > - queue_remove_all(chrc->notify_clients, NULL, NULL, remove_client);
> > > > > > queue_remove_all(chrc->descs, NULL, NULL, unregister_descriptor);
> > > > > >
> > > > > > g_dbus_unregister_interface(btd_get_dbus_connection(), chrc->path,
> > > > > > --
> > > > > > 2.17.2
> > > > >
> > > > > Can you try with this set to see it does work properly.
> > > > >
> > > > > --
> > > > > Luiz Augusto von Dentz
> > >
> > >
> > >
> > > --
> > > Luiz Augusto von Dentz
>
>
>
> --
> Luiz Augusto von Dentz

2018-11-21 07:15:53

by Gal Ben Haim

Subject: Re: [PATCH BlueZ 1/5] gatt: Fix invalid read when disconnecting

it fixes the same case that cause the version from master to crash.
i haven't tested for anything else..

On Tue, Nov 20, 2018 at 8:31 PM Gal Ben Haim <[email protected]> wrote:
>
> ok I patched it. i'll test it tomorrow in the office and let you know.
>
> On Tue, Nov 20, 2018 at 3:59 PM Luiz Augusto von Dentz
> <[email protected]> wrote:
> >
> > Hi Gal,
> >
> > On Tue, Nov 20, 2018 at 2:35 PM Gal Ben Haim <[email protected]> wrote:
> > >
> > > i tried with both git am and git apply
> >
> > Does that applies to v2 as well? I just rebased it on top master, are
> > you sure you don't have your own changes conflicting with it?
> >
> > > On Tue, Nov 20, 2018 at 12:35 PM Luiz Augusto von Dentz
> > > <[email protected]> wrote:
> > > >
> > > > Hi Gal,
> > > > On Tue, Nov 20, 2018 at 11:31 AM Gal Ben Haim <[email protected]> wrote:
> > > > >
> > > > > I can't apply this patch, not to the revision in master and not to the 5.50 tag
> > > >
> > > > They are on top o master, how are you trying to apply them, with git am?
> > > >
> > > > > On Tue, Nov 20, 2018 at 10:50 AM Luiz Augusto von Dentz
> > > > > <[email protected]> wrote:
> > > > > >
> > > > > > On Mon, Nov 19, 2018 at 5:43 PM Luiz Augusto von Dentz
> > > > > > <[email protected]> wrote:
> > > > > > >
> > > > > > > From: Luiz Augusto von Dentz <[email protected]>
> > > > > > >
> > > > > > > In case there is a client of AcquireNotify and a disconnect happens the
> > > > > > > code not only have to free the client object but also destroy the io
> > > > > > > associated with it, for this reason the client object cannot be freed
> > > > > > > until the io is destroyed otherwise it may lead to the following error:
> > > > > > >
> > > > > > > Invalid read of size 4
> > > > > > > at 0x63920: notify_io_destroy (gatt-client.c:1461)
> > > > > > > by 0x63EDB: pipe_io_destroy (gatt-client.c:1082)
> > > > > > > by 0x6405B: characteristic_free (gatt-client.c:1663)
> > > > > > > by 0x81F33: remove_interface (object.c:667)
> > > > > > > by 0x826CB: g_dbus_unregister_interface (object.c:1391)
> > > > > > > by 0x85D2B: queue_remove_all (queue.c:354)
> > > > > > > by 0x635F7: unregister_service (gatt-client.c:1893)
> > > > > > > by 0x85CF7: queue_remove_all (queue.c:339)
> > > > > > > by 0x661DF: btd_gatt_client_service_removed (gatt-client.c:2199)
> > > > > > > by 0x695CB: gatt_service_removed (device.c:3747)
> > > > > > > by 0x85B17: queue_foreach (queue.c:220)
> > > > > > > by 0x91283: notify_service_changed (gatt-db.c:280)
> > > > > > > by 0x91283: gatt_db_service_destroy (gatt-db.c:291)
> > > > > > > Address 0x515ed48 is 0 bytes inside a block of size 20 free'd
> > > > > > > at 0x483EAD0: free (vg_replace_malloc.c:530)
> > > > > > > by 0x85D2B: queue_remove_all (queue.c:354)
> > > > > > > by 0x636D3: unregister_characteristic (gatt-client.c:1741)
> > > > > > > by 0x85D2B: queue_remove_all (queue.c:354)
> > > > > > > by 0x635F7: unregister_service (gatt-client.c:1893)
> > > > > > > by 0x85CF7: queue_remove_all (queue.c:339)
> > > > > > > by 0x661DF: btd_gatt_client_service_removed (gatt-client.c:2199)
> > > > > > > by 0x695CB: gatt_service_removed (device.c:3747)
> > > > > > > by 0x85B17: queue_foreach (queue.c:220)
> > > > > > > by 0x91283: notify_service_changed (gatt-db.c:280)
> > > > > > > by 0x91283: gatt_db_service_destroy (gatt-db.c:291)
> > > > > > > by 0x85D2B: queue_remove_all (queue.c:354)
> > > > > > > by 0x91387: gatt_db_clear_range (gatt-db.c:475)
> > > > > > > ---
> > > > > > > src/gatt-client.c | 24 ++++++++++++------------
> > > > > > > 1 file changed, 12 insertions(+), 12 deletions(-)
> > > > > > >
> > > > > > > diff --git a/src/gatt-client.c b/src/gatt-client.c
> > > > > > > index 234f46ed7..55aa5e423 100644
> > > > > > > --- a/src/gatt-client.c
> > > > > > > +++ b/src/gatt-client.c
> > > > > > > @@ -1645,13 +1645,22 @@ static const GDBusMethodTable characteristic_methods[] = {
> > > > > > > { }
> > > > > > > };
> > > > > > >
> > > > > > > +static void remove_client(void *data)
> > > > > > > +{
> > > > > > > + struct notify_client *ntfy_client = data;
> > > > > > > + struct btd_gatt_client *client = ntfy_client->chrc->service->client;
> > > > > > > +
> > > > > > > + queue_remove(client->all_notify_clients, ntfy_client);
> > > > > > > +
> > > > > > > + notify_client_unref(ntfy_client);
> > > > > > > +}
> > > > > > > +
> > > > > > > static void characteristic_free(void *data)
> > > > > > > {
> > > > > > > struct characteristic *chrc = data;
> > > > > > >
> > > > > > > /* List should be empty here */
> > > > > > > queue_destroy(chrc->descs, NULL);
> > > > > > > - queue_destroy(chrc->notify_clients, NULL);
> > > > > > >
> > > > > > > if (chrc->write_io) {
> > > > > > > queue_remove(chrc->service->client->ios, chrc->write_io->io);
> > > > > > > @@ -1663,6 +1672,8 @@ static void characteristic_free(void *data)
> > > > > > > pipe_io_destroy(chrc->notify_io);
> > > > > > > }
> > > > > > >
> > > > > > > + queue_destroy(chrc->notify_clients, remove_client);
> > > > > > > +
> > > > > > > g_free(chrc->path);
> > > > > > > free(chrc);
> > > > > > > }
> > > > > > > @@ -1715,16 +1726,6 @@ static struct characteristic *characteristic_create(
> > > > > > > return chrc;
> > > > > > > }
> > > > > > >
> > > > > > > -static void remove_client(void *data)
> > > > > > > -{
> > > > > > > - struct notify_client *ntfy_client = data;
> > > > > > > - struct btd_gatt_client *client = ntfy_client->chrc->service->client;
> > > > > > > -
> > > > > > > - queue_remove(client->all_notify_clients, ntfy_client);
> > > > > > > -
> > > > > > > - notify_client_unref(ntfy_client);
> > > > > > > -}
> > > > > > > -
> > > > > > > static void unregister_characteristic(void *data)
> > > > > > > {
> > > > > > > struct characteristic *chrc = data;
> > > > > > > @@ -1738,7 +1739,6 @@ static void unregister_characteristic(void *data)
> > > > > > > if (chrc->write_op)
> > > > > > > bt_gatt_client_cancel(gatt, chrc->write_op->id);
> > > > > > >
> > > > > > > - queue_remove_all(chrc->notify_clients, NULL, NULL, remove_client);
> > > > > > > queue_remove_all(chrc->descs, NULL, NULL, unregister_descriptor);
> > > > > > >
> > > > > > > g_dbus_unregister_interface(btd_get_dbus_connection(), chrc->path,
> > > > > > > --
> > > > > > > 2.17.2
> > > > > >
> > > > > > Can you try with this set to see it does work properly.
> > > > > >
> > > > > > --
> > > > > > Luiz Augusto von Dentz
> > > >
> > > >
> > > >
> > > > --
> > > > Luiz Augusto von Dentz
> >
> >
> >
> > --
> > Luiz Augusto von Dentz

2018-11-21 09:15:45

by Luiz Augusto von Dentz

Subject: Re: [PATCH BlueZ 1/5] gatt: Fix invalid read when disconnecting

Hi,
On Wed, Nov 21, 2018 at 9:15 AM Gal Ben Haim <[email protected]> wrote:
>
> it fixes the same case that cause the version from master to crash.
> i haven't tested for anything else..
>
> On Tue, Nov 20, 2018 at 8:31 PM Gal Ben Haim <[email protected]> wrote:
> >
> > ok I patched it. i'll test it tomorrow in the office and let you know.
> >
> > On Tue, Nov 20, 2018 at 3:59 PM Luiz Augusto von Dentz
> > <[email protected]> wrote:
> > >
> > > Hi Gal,
> > >
> > > On Tue, Nov 20, 2018 at 2:35 PM Gal Ben Haim <[email protected]> wrote:
> > > >
> > > > i tried with both git am and git apply
> > >
> > > Does that applies to v2 as well? I just rebased it on top master, are
> > > you sure you don't have your own changes conflicting with it?
> > >
> > > > On Tue, Nov 20, 2018 at 12:35 PM Luiz Augusto von Dentz
> > > > <[email protected]> wrote:
> > > > >
> > > > > Hi Gal,
> > > > > On Tue, Nov 20, 2018 at 11:31 AM Gal Ben Haim <[email protected]> wrote:
> > > > > >
> > > > > > I can't apply this patch, not to the revision in master and not to the 5.50 tag
> > > > >
> > > > > They are on top o master, how are you trying to apply them, with git am?
> > > > >
> > > > > > On Tue, Nov 20, 2018 at 10:50 AM Luiz Augusto von Dentz
> > > > > > <[email protected]> wrote:
> > > > > > >
> > > > > > > On Mon, Nov 19, 2018 at 5:43 PM Luiz Augusto von Dentz
> > > > > > > <[email protected]> wrote:
> > > > > > > >
> > > > > > > > From: Luiz Augusto von Dentz <[email protected]>
> > > > > > > >
> > > > > > > > In case there is a client of AcquireNotify and a disconnect happens the
> > > > > > > > code not only have to free the client object but also destroy the io
> > > > > > > > associated with it, for this reason the client object cannot be freed
> > > > > > > > until the io is destroyed otherwise it may lead to the following error:
> > > > > > > >
> > > > > > > > Invalid read of size 4
> > > > > > > > at 0x63920: notify_io_destroy (gatt-client.c:1461)
> > > > > > > > by 0x63EDB: pipe_io_destroy (gatt-client.c:1082)
> > > > > > > > by 0x6405B: characteristic_free (gatt-client.c:1663)
> > > > > > > > by 0x81F33: remove_interface (object.c:667)
> > > > > > > > by 0x826CB: g_dbus_unregister_interface (object.c:1391)
> > > > > > > > by 0x85D2B: queue_remove_all (queue.c:354)
> > > > > > > > by 0x635F7: unregister_service (gatt-client.c:1893)
> > > > > > > > by 0x85CF7: queue_remove_all (queue.c:339)
> > > > > > > > by 0x661DF: btd_gatt_client_service_removed (gatt-client.c:2199)
> > > > > > > > by 0x695CB: gatt_service_removed (device.c:3747)
> > > > > > > > by 0x85B17: queue_foreach (queue.c:220)
> > > > > > > > by 0x91283: notify_service_changed (gatt-db.c:280)
> > > > > > > > by 0x91283: gatt_db_service_destroy (gatt-db.c:291)
> > > > > > > > Address 0x515ed48 is 0 bytes inside a block of size 20 free'd
> > > > > > > > at 0x483EAD0: free (vg_replace_malloc.c:530)
> > > > > > > > by 0x85D2B: queue_remove_all (queue.c:354)
> > > > > > > > by 0x636D3: unregister_characteristic (gatt-client.c:1741)
> > > > > > > > by 0x85D2B: queue_remove_all (queue.c:354)
> > > > > > > > by 0x635F7: unregister_service (gatt-client.c:1893)
> > > > > > > > by 0x85CF7: queue_remove_all (queue.c:339)
> > > > > > > > by 0x661DF: btd_gatt_client_service_removed (gatt-client.c:2199)
> > > > > > > > by 0x695CB: gatt_service_removed (device.c:3747)
> > > > > > > > by 0x85B17: queue_foreach (queue.c:220)
> > > > > > > > by 0x91283: notify_service_changed (gatt-db.c:280)
> > > > > > > > by 0x91283: gatt_db_service_destroy (gatt-db.c:291)
> > > > > > > > by 0x85D2B: queue_remove_all (queue.c:354)
> > > > > > > > by 0x91387: gatt_db_clear_range (gatt-db.c:475)
> > > > > > > > ---
> > > > > > > > src/gatt-client.c | 24 ++++++++++++------------
> > > > > > > > 1 file changed, 12 insertions(+), 12 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/src/gatt-client.c b/src/gatt-client.c
> > > > > > > > index 234f46ed7..55aa5e423 100644
> > > > > > > > --- a/src/gatt-client.c
> > > > > > > > +++ b/src/gatt-client.c
> > > > > > > > @@ -1645,13 +1645,22 @@ static const GDBusMethodTable characteristic_methods[] = {
> > > > > > > > { }
> > > > > > > > };
> > > > > > > >
> > > > > > > > +static void remove_client(void *data)
> > > > > > > > +{
> > > > > > > > + struct notify_client *ntfy_client = data;
> > > > > > > > + struct btd_gatt_client *client = ntfy_client->chrc->service->client;
> > > > > > > > +
> > > > > > > > + queue_remove(client->all_notify_clients, ntfy_client);
> > > > > > > > +
> > > > > > > > + notify_client_unref(ntfy_client);
> > > > > > > > +}
> > > > > > > > +
> > > > > > > > static void characteristic_free(void *data)
> > > > > > > > {
> > > > > > > > struct characteristic *chrc = data;
> > > > > > > >
> > > > > > > > /* List should be empty here */
> > > > > > > > queue_destroy(chrc->descs, NULL);
> > > > > > > > - queue_destroy(chrc->notify_clients, NULL);
> > > > > > > >
> > > > > > > > if (chrc->write_io) {
> > > > > > > > queue_remove(chrc->service->client->ios, chrc->write_io->io);
> > > > > > > > @@ -1663,6 +1672,8 @@ static void characteristic_free(void *data)
> > > > > > > > pipe_io_destroy(chrc->notify_io);
> > > > > > > > }
> > > > > > > >
> > > > > > > > + queue_destroy(chrc->notify_clients, remove_client);
> > > > > > > > +
> > > > > > > > g_free(chrc->path);
> > > > > > > > free(chrc);
> > > > > > > > }
> > > > > > > > @@ -1715,16 +1726,6 @@ static struct characteristic *characteristic_create(
> > > > > > > > return chrc;
> > > > > > > > }
> > > > > > > >
> > > > > > > > -static void remove_client(void *data)
> > > > > > > > -{
> > > > > > > > - struct notify_client *ntfy_client = data;
> > > > > > > > - struct btd_gatt_client *client = ntfy_client->chrc->service->client;
> > > > > > > > -
> > > > > > > > - queue_remove(client->all_notify_clients, ntfy_client);
> > > > > > > > -
> > > > > > > > - notify_client_unref(ntfy_client);
> > > > > > > > -}
> > > > > > > > -
> > > > > > > > static void unregister_characteristic(void *data)
> > > > > > > > {
> > > > > > > > struct characteristic *chrc = data;
> > > > > > > > @@ -1738,7 +1739,6 @@ static void unregister_characteristic(void *data)
> > > > > > > > if (chrc->write_op)
> > > > > > > > bt_gatt_client_cancel(gatt, chrc->write_op->id);
> > > > > > > >
> > > > > > > > - queue_remove_all(chrc->notify_clients, NULL, NULL, remove_client);
> > > > > > > > queue_remove_all(chrc->descs, NULL, NULL, unregister_descriptor);
> > > > > > > >
> > > > > > > > g_dbus_unregister_interface(btd_get_dbus_connection(), chrc->path,
> > > > > > > > --
> > > > > > > > 2.17.2

Applied.

--
Luiz Augusto von Dentz

2018-12-15 06:45:18

by Gal Ben Haim

Subject: Re: [PATCH BlueZ 1/5] gatt: Fix invalid read when disconnecting

I still get segmentation faults occasionally after applying this patch,
i'm not 100% sure that it's related but it happens in the same area...
its not something that I can reproduce immediately like with the
previous segfault. I think that it doesn't happen if I use my original
patch https://marc.info/?l=linux-bluetooth&m=154250398526173&w=2

bluetoothd[2599]: src/device.c:att_disconnected_cb()
bluetoothd[2599]: src/device.c:att_disconnected_cb() Connection reset
by peer (104)
bluetoothd[2599]: src/service.c:change_state() 0x4edae0: device
DA:51:61:24:3C:36 profile gap-profile state changed: connected ->
disconnecting (0)
bluetoothd[2599]: src/service.c:change_state() 0x4edae0: device
DA:51:61:24:3C:36 profile gap-profile state changed: disconnecting ->
disconnected (0)
bluetoothd[2599]: src/gatt-client.c:btd_gatt_client_disconnected()
Device disconnected. Cleaning up.
bluetoothd[2599]: src/device.c:att_disconnected_cb() Automatic
connection disabled
bluetoothd[2599]: src/gatt-database.c:btd_gatt_database_att_disconnected()
bluetoothd[2599]: attrib/gattrib.c:g_attrib_unref() 0x4ee410: g_attrib_unref=0
bluetoothd[2599]: src/device.c:btd_device_set_temporary() temporary 1
bluetoothd[2599]: src/device.c:device_remove() Removing device
/org/bluez/hci0/dev_DA_51_61_24_3C_36
bluetoothd[2599]: src/service.c:change_state() 0x4edae0: device
DA:51:61:24:3C:36 profile gap-profile state changed: disconnected ->
unavailable (0)
bluetoothd[2599]: profiles/gap/gas.c:gap_remove() GAP profile remove
(DA:51:61:24:3C:36)
bluetoothd[2599]: src/service.c:btd_service_unref() 0x4edae0: ref=0
bluetoothd[2599]: src/device.c:btd_device_unref() Freeing device
/org/bluez/hci0/dev_DA_51_61_24_3C_36
bluetoothd[2599]: src/gatt-client.c:unregister_service() Removing GATT
service: /org/bluez/hci0/dev_DA_51_61_24_3C_36/service0008
bluetoothd[2599]: src/gatt-client.c:unregister_service() Removing GATT
service: /org/bluez/hci0/dev_DA_51_61_24_3C_36/service0009
bluetoothd[2599]: src/gatt-client.c:unregister_characteristic()
Removing GATT characteristic:
/org/bluez/hci0/dev_DA_51_61_24_3C_36/service0009/char000a
bluetoothd[2599]: src/gatt-client.c:unregister_characteristic()
Removing GATT characteristic:
/org/bluez/hci0/dev_DA_51_61_24_3C_36/service0009/char000c
bluetoothd[2599]: src/gatt-client.c:unregister_descriptor() Removing
GATT descriptor:
/org/bluez/hci0/dev_DA_51_61_24_3C_36/service0009/char000c/desc000e
Segmentation fault


On Wed, Nov 21, 2018 at 11:15 AM Luiz Augusto von Dentz
<[email protected]> wrote:
>
> Hi,
> On Wed, Nov 21, 2018 at 9:15 AM Gal Ben Haim <[email protected]> wrote:
> >
> > it fixes the same case that cause the version from master to crash.
> > i haven't tested for anything else..
> >
> > On Tue, Nov 20, 2018 at 8:31 PM Gal Ben Haim <[email protected]> wrote:
> > >
> > > ok I patched it. i'll test it tomorrow in the office and let you know.
> > >
> > > On Tue, Nov 20, 2018 at 3:59 PM Luiz Augusto von Dentz
> > > <[email protected]> wrote:
> > > >
> > > > Hi Gal,
> > > >
> > > > On Tue, Nov 20, 2018 at 2:35 PM Gal Ben Haim <[email protected]> wrote:
> > > > >
> > > > > i tried with both git am and git apply
> > > >
> > > > Does that applies to v2 as well? I just rebased it on top master, are
> > > > you sure you don't have your own changes conflicting with it?
> > > >
> > > > > On Tue, Nov 20, 2018 at 12:35 PM Luiz Augusto von Dentz
> > > > > <[email protected]> wrote:
> > > > > >
> > > > > > Hi Gal,
> > > > > > On Tue, Nov 20, 2018 at 11:31 AM Gal Ben Haim <[email protected]> wrote:
> > > > > > >
> > > > > > > I can't apply this patch, not to the revision in master and not to the 5.50 tag
> > > > > >
> > > > > > They are on top o master, how are you trying to apply them, with git am?
> > > > > >
> > > > > > > On Tue, Nov 20, 2018 at 10:50 AM Luiz Augusto von Dentz
> > > > > > > <[email protected]> wrote:
> > > > > > > >
> > > > > > > > On Mon, Nov 19, 2018 at 5:43 PM Luiz Augusto von Dentz
> > > > > > > > <[email protected]> wrote:
> > > > > > > > >
> > > > > > > > > From: Luiz Augusto von Dentz <[email protected]>
> > > > > > > > >
> > > > > > > > > In case there is a client of AcquireNotify and a disconnect happens the
> > > > > > > > > code not only have to free the client object but also destroy the io
> > > > > > > > > associated with it, for this reason the client object cannot be freed
> > > > > > > > > until the io is destroyed otherwise it may lead to the following error:
> > > > > > > > >
> > > > > > > > > Invalid read of size 4
> > > > > > > > > at 0x63920: notify_io_destroy (gatt-client.c:1461)
> > > > > > > > > by 0x63EDB: pipe_io_destroy (gatt-client.c:1082)
> > > > > > > > > by 0x6405B: characteristic_free (gatt-client.c:1663)
> > > > > > > > > by 0x81F33: remove_interface (object.c:667)
> > > > > > > > > by 0x826CB: g_dbus_unregister_interface (object.c:1391)
> > > > > > > > > by 0x85D2B: queue_remove_all (queue.c:354)
> > > > > > > > > by 0x635F7: unregister_service (gatt-client.c:1893)
> > > > > > > > > by 0x85CF7: queue_remove_all (queue.c:339)
> > > > > > > > > by 0x661DF: btd_gatt_client_service_removed (gatt-client.c:2199)
> > > > > > > > > by 0x695CB: gatt_service_removed (device.c:3747)
> > > > > > > > > by 0x85B17: queue_foreach (queue.c:220)
> > > > > > > > > by 0x91283: notify_service_changed (gatt-db.c:280)
> > > > > > > > > by 0x91283: gatt_db_service_destroy (gatt-db.c:291)
> > > > > > > > > Address 0x515ed48 is 0 bytes inside a block of size 20 free'd
> > > > > > > > > at 0x483EAD0: free (vg_replace_malloc.c:530)
> > > > > > > > > by 0x85D2B: queue_remove_all (queue.c:354)
> > > > > > > > > by 0x636D3: unregister_characteristic (gatt-client.c:1741)
> > > > > > > > > by 0x85D2B: queue_remove_all (queue.c:354)
> > > > > > > > > by 0x635F7: unregister_service (gatt-client.c:1893)
> > > > > > > > > by 0x85CF7: queue_remove_all (queue.c:339)
> > > > > > > > > by 0x661DF: btd_gatt_client_service_removed (gatt-client.c:2199)
> > > > > > > > > by 0x695CB: gatt_service_removed (device.c:3747)
> > > > > > > > > by 0x85B17: queue_foreach (queue.c:220)
> > > > > > > > > by 0x91283: notify_service_changed (gatt-db.c:280)
> > > > > > > > > by 0x91283: gatt_db_service_destroy (gatt-db.c:291)
> > > > > > > > > by 0x85D2B: queue_remove_all (queue.c:354)
> > > > > > > > > by 0x91387: gatt_db_clear_range (gatt-db.c:475)
> > > > > > > > > ---
> > > > > > > > > src/gatt-client.c | 24 ++++++++++++------------
> > > > > > > > > 1 file changed, 12 insertions(+), 12 deletions(-)
> > > > > > > > >
> > > > > > > > > diff --git a/src/gatt-client.c b/src/gatt-client.c
> > > > > > > > > index 234f46ed7..55aa5e423 100644
> > > > > > > > > --- a/src/gatt-client.c
> > > > > > > > > +++ b/src/gatt-client.c
> > > > > > > > > @@ -1645,13 +1645,22 @@ static const GDBusMethodTable characteristic_methods[] = {
> > > > > > > > > { }
> > > > > > > > > };
> > > > > > > > >
> > > > > > > > > +static void remove_client(void *data)
> > > > > > > > > +{
> > > > > > > > > + struct notify_client *ntfy_client = data;
> > > > > > > > > + struct btd_gatt_client *client = ntfy_client->chrc->service->client;
> > > > > > > > > +
> > > > > > > > > + queue_remove(client->all_notify_clients, ntfy_client);
> > > > > > > > > +
> > > > > > > > > + notify_client_unref(ntfy_client);
> > > > > > > > > +}
> > > > > > > > > +
> > > > > > > > > static void characteristic_free(void *data)
> > > > > > > > > {
> > > > > > > > > struct characteristic *chrc = data;
> > > > > > > > >
> > > > > > > > > /* List should be empty here */
> > > > > > > > > queue_destroy(chrc->descs, NULL);
> > > > > > > > > - queue_destroy(chrc->notify_clients, NULL);
> > > > > > > > >
> > > > > > > > > if (chrc->write_io) {
> > > > > > > > > queue_remove(chrc->service->client->ios, chrc->write_io->io);
> > > > > > > > > @@ -1663,6 +1672,8 @@ static void characteristic_free(void *data)
> > > > > > > > > pipe_io_destroy(chrc->notify_io);
> > > > > > > > > }
> > > > > > > > >
> > > > > > > > > + queue_destroy(chrc->notify_clients, remove_client);
> > > > > > > > > +
> > > > > > > > > g_free(chrc->path);
> > > > > > > > > free(chrc);
> > > > > > > > > }
> > > > > > > > > @@ -1715,16 +1726,6 @@ static struct characteristic *characteristic_create(
> > > > > > > > > return chrc;
> > > > > > > > > }
> > > > > > > > >
> > > > > > > > > -static void remove_client(void *data)
> > > > > > > > > -{
> > > > > > > > > - struct notify_client *ntfy_client = data;
> > > > > > > > > - struct btd_gatt_client *client = ntfy_client->chrc->service->client;
> > > > > > > > > -
> > > > > > > > > - queue_remove(client->all_notify_clients, ntfy_client);
> > > > > > > > > -
> > > > > > > > > - notify_client_unref(ntfy_client);
> > > > > > > > > -}
> > > > > > > > > -
> > > > > > > > > static void unregister_characteristic(void *data)
> > > > > > > > > {
> > > > > > > > > struct characteristic *chrc = data;
> > > > > > > > > @@ -1738,7 +1739,6 @@ static void unregister_characteristic(void *data)
> > > > > > > > > if (chrc->write_op)
> > > > > > > > > bt_gatt_client_cancel(gatt, chrc->write_op->id);
> > > > > > > > >
> > > > > > > > > - queue_remove_all(chrc->notify_clients, NULL, NULL, remove_client);
> > > > > > > > > queue_remove_all(chrc->descs, NULL, NULL, unregister_descriptor);
> > > > > > > > >
> > > > > > > > > g_dbus_unregister_interface(btd_get_dbus_connection(), chrc->path,
> > > > > > > > > --
> > > > > > > > > 2.17.2
>
> Applied.
>
> --
> Luiz Augusto von Dentz

2018-12-22 18:51:32

by Gal Ben Haim

Subject: Re: [PATCH BlueZ 1/5] gatt: Fix invalid read when disconnecting

it definitely happens with this patch and not with mine

On Sat, Dec 15, 2018 at 8:45 AM Gal Ben Haim <[email protected]> wrote:
>
> I still get segmentation faults occasionally after applying this patch,
> i'm not 100% sure that it's related but it happens in the same area...
> its not something that I can reproduce immediately like with the
> previous segfault. I think that it doesn't happen if I use my original
> patch https://marc.info/?l=linux-bluetooth&m=154250398526173&w=2
>
> bluetoothd[2599]: src/device.c:att_disconnected_cb()
> bluetoothd[2599]: src/device.c:att_disconnected_cb() Connection reset
> by peer (104)
> bluetoothd[2599]: src/service.c:change_state() 0x4edae0: device
> DA:51:61:24:3C:36 profile gap-profile state changed: connected ->
> disconnecting (0)
> bluetoothd[2599]: src/service.c:change_state() 0x4edae0: device
> DA:51:61:24:3C:36 profile gap-profile state changed: disconnecting ->
> disconnected (0)
> bluetoothd[2599]: src/gatt-client.c:btd_gatt_client_disconnected()
> Device disconnected. Cleaning up.
> bluetoothd[2599]: src/device.c:att_disconnected_cb() Automatic
> connection disabled
> bluetoothd[2599]: src/gatt-database.c:btd_gatt_database_att_disconnected()
> bluetoothd[2599]: attrib/gattrib.c:g_attrib_unref() 0x4ee410: g_attrib_unref=0
> bluetoothd[2599]: src/device.c:btd_device_set_temporary() temporary 1
> bluetoothd[2599]: src/device.c:device_remove() Removing device
> /org/bluez/hci0/dev_DA_51_61_24_3C_36
> bluetoothd[2599]: src/service.c:change_state() 0x4edae0: device
> DA:51:61:24:3C:36 profile gap-profile state changed: disconnected ->
> unavailable (0)
> bluetoothd[2599]: profiles/gap/gas.c:gap_remove() GAP profile remove
> (DA:51:61:24:3C:36)
> bluetoothd[2599]: src/service.c:btd_service_unref() 0x4edae0: ref=0
> bluetoothd[2599]: src/device.c:btd_device_unref() Freeing device
> /org/bluez/hci0/dev_DA_51_61_24_3C_36
> bluetoothd[2599]: src/gatt-client.c:unregister_service() Removing GATT
> service: /org/bluez/hci0/dev_DA_51_61_24_3C_36/service0008
> bluetoothd[2599]: src/gatt-client.c:unregister_service() Removing GATT
> service: /org/bluez/hci0/dev_DA_51_61_24_3C_36/service0009
> bluetoothd[2599]: src/gatt-client.c:unregister_characteristic()
> Removing GATT characteristic:
> /org/bluez/hci0/dev_DA_51_61_24_3C_36/service0009/char000a
> bluetoothd[2599]: src/gatt-client.c:unregister_characteristic()
> Removing GATT characteristic:
> /org/bluez/hci0/dev_DA_51_61_24_3C_36/service0009/char000c
> bluetoothd[2599]: src/gatt-client.c:unregister_descriptor() Removing
> GATT descriptor:
> /org/bluez/hci0/dev_DA_51_61_24_3C_36/service0009/char000c/desc000e
> Segmentation fault
>
>
> On Wed, Nov 21, 2018 at 11:15 AM Luiz Augusto von Dentz
> <[email protected]> wrote:
> >
> > Hi,
> > On Wed, Nov 21, 2018 at 9:15 AM Gal Ben Haim <[email protected]> wrote:
> > >
> > > it fixes the same case that cause the version from master to crash.
> > > i haven't tested for anything else..
> > >
> > > On Tue, Nov 20, 2018 at 8:31 PM Gal Ben Haim <[email protected]> wrote:
> > > >
> > > > ok I patched it. i'll test it tomorrow in the office and let you know.
> > > >
> > > > On Tue, Nov 20, 2018 at 3:59 PM Luiz Augusto von Dentz
> > > > <[email protected]> wrote:
> > > > >
> > > > > Hi Gal,
> > > > >
> > > > > On Tue, Nov 20, 2018 at 2:35 PM Gal Ben Haim <[email protected]> wrote:
> > > > > >
> > > > > > i tried with both git am and git apply
> > > > >
> > > > > Does that applies to v2 as well? I just rebased it on top master, are
> > > > > you sure you don't have your own changes conflicting with it?
> > > > >
> > > > > > On Tue, Nov 20, 2018 at 12:35 PM Luiz Augusto von Dentz
> > > > > > <[email protected]> wrote:
> > > > > > >
> > > > > > > Hi Gal,
> > > > > > > On Tue, Nov 20, 2018 at 11:31 AM Gal Ben Haim <[email protected]> wrote:
> > > > > > > >
> > > > > > > > I can't apply this patch, not to the revision in master and not to the 5.50 tag
> > > > > > >
> > > > > > > They are on top o master, how are you trying to apply them, with git am?
> > > > > > >
> > > > > > > > On Tue, Nov 20, 2018 at 10:50 AM Luiz Augusto von Dentz
> > > > > > > > <[email protected]> wrote:
> > > > > > > > >
> > > > > > > > > On Mon, Nov 19, 2018 at 5:43 PM Luiz Augusto von Dentz
> > > > > > > > > <[email protected]> wrote:
> > > > > > > > > >
> > > > > > > > > > From: Luiz Augusto von Dentz <[email protected]>
> > > > > > > > > >
> > > > > > > > > > In case there is a client of AcquireNotify and a disconnect happens the
> > > > > > > > > > code not only have to free the client object but also destroy the io
> > > > > > > > > > associated with it, for this reason the client object cannot be freed
> > > > > > > > > > until the io is destroyed otherwise it may lead to the following error:
> > > > > > > > > >
> > > > > > > > > > Invalid read of size 4
> > > > > > > > > > at 0x63920: notify_io_destroy (gatt-client.c:1461)
> > > > > > > > > > by 0x63EDB: pipe_io_destroy (gatt-client.c:1082)
> > > > > > > > > > by 0x6405B: characteristic_free (gatt-client.c:1663)
> > > > > > > > > > by 0x81F33: remove_interface (object.c:667)
> > > > > > > > > > by 0x826CB: g_dbus_unregister_interface (object.c:1391)
> > > > > > > > > > by 0x85D2B: queue_remove_all (queue.c:354)
> > > > > > > > > > by 0x635F7: unregister_service (gatt-client.c:1893)
> > > > > > > > > > by 0x85CF7: queue_remove_all (queue.c:339)
> > > > > > > > > > by 0x661DF: btd_gatt_client_service_removed (gatt-client.c:2199)
> > > > > > > > > > by 0x695CB: gatt_service_removed (device.c:3747)
> > > > > > > > > > by 0x85B17: queue_foreach (queue.c:220)
> > > > > > > > > > by 0x91283: notify_service_changed (gatt-db.c:280)
> > > > > > > > > > by 0x91283: gatt_db_service_destroy (gatt-db.c:291)
> > > > > > > > > > Address 0x515ed48 is 0 bytes inside a block of size 20 free'd
> > > > > > > > > > at 0x483EAD0: free (vg_replace_malloc.c:530)
> > > > > > > > > > by 0x85D2B: queue_remove_all (queue.c:354)
> > > > > > > > > > by 0x636D3: unregister_characteristic (gatt-client.c:1741)
> > > > > > > > > > by 0x85D2B: queue_remove_all (queue.c:354)
> > > > > > > > > > by 0x635F7: unregister_service (gatt-client.c:1893)
> > > > > > > > > > by 0x85CF7: queue_remove_all (queue.c:339)
> > > > > > > > > > by 0x661DF: btd_gatt_client_service_removed (gatt-client.c:2199)
> > > > > > > > > > by 0x695CB: gatt_service_removed (device.c:3747)
> > > > > > > > > > by 0x85B17: queue_foreach (queue.c:220)
> > > > > > > > > > by 0x91283: notify_service_changed (gatt-db.c:280)
> > > > > > > > > > by 0x91283: gatt_db_service_destroy (gatt-db.c:291)
> > > > > > > > > > by 0x85D2B: queue_remove_all (queue.c:354)
> > > > > > > > > > by 0x91387: gatt_db_clear_range (gatt-db.c:475)
> > > > > > > > > > ---
> > > > > > > > > > src/gatt-client.c | 24 ++++++++++++------------
> > > > > > > > > > 1 file changed, 12 insertions(+), 12 deletions(-)
> > > > > > > > > >
> > > > > > > > > > diff --git a/src/gatt-client.c b/src/gatt-client.c
> > > > > > > > > > index 234f46ed7..55aa5e423 100644
> > > > > > > > > > --- a/src/gatt-client.c
> > > > > > > > > > +++ b/src/gatt-client.c
> > > > > > > > > > @@ -1645,13 +1645,22 @@ static const GDBusMethodTable characteristic_methods[] = {
> > > > > > > > > > { }
> > > > > > > > > > };
> > > > > > > > > >
> > > > > > > > > > +static void remove_client(void *data)
> > > > > > > > > > +{
> > > > > > > > > > + struct notify_client *ntfy_client = data;
> > > > > > > > > > + struct btd_gatt_client *client = ntfy_client->chrc->service->client;
> > > > > > > > > > +
> > > > > > > > > > + queue_remove(client->all_notify_clients, ntfy_client);
> > > > > > > > > > +
> > > > > > > > > > + notify_client_unref(ntfy_client);
> > > > > > > > > > +}
> > > > > > > > > > +
> > > > > > > > > > static void characteristic_free(void *data)
> > > > > > > > > > {
> > > > > > > > > > struct characteristic *chrc = data;
> > > > > > > > > >
> > > > > > > > > > /* List should be empty here */
> > > > > > > > > > queue_destroy(chrc->descs, NULL);
> > > > > > > > > > - queue_destroy(chrc->notify_clients, NULL);
> > > > > > > > > >
> > > > > > > > > > if (chrc->write_io) {
> > > > > > > > > > queue_remove(chrc->service->client->ios, chrc->write_io->io);
> > > > > > > > > > @@ -1663,6 +1672,8 @@ static void characteristic_free(void *data)
> > > > > > > > > > pipe_io_destroy(chrc->notify_io);
> > > > > > > > > > }
> > > > > > > > > >
> > > > > > > > > > + queue_destroy(chrc->notify_clients, remove_client);
> > > > > > > > > > +
> > > > > > > > > > g_free(chrc->path);
> > > > > > > > > > free(chrc);
> > > > > > > > > > }
> > > > > > > > > > @@ -1715,16 +1726,6 @@ static struct characteristic *characteristic_create(
> > > > > > > > > > return chrc;
> > > > > > > > > > }
> > > > > > > > > >
> > > > > > > > > > -static void remove_client(void *data)
> > > > > > > > > > -{
> > > > > > > > > > - struct notify_client *ntfy_client = data;
> > > > > > > > > > - struct btd_gatt_client *client = ntfy_client->chrc->service->client;
> > > > > > > > > > -
> > > > > > > > > > - queue_remove(client->all_notify_clients, ntfy_client);
> > > > > > > > > > -
> > > > > > > > > > - notify_client_unref(ntfy_client);
> > > > > > > > > > -}
> > > > > > > > > > -
> > > > > > > > > > static void unregister_characteristic(void *data)
> > > > > > > > > > {
> > > > > > > > > > struct characteristic *chrc = data;
> > > > > > > > > > @@ -1738,7 +1739,6 @@ static void unregister_characteristic(void *data)
> > > > > > > > > > if (chrc->write_op)
> > > > > > > > > > bt_gatt_client_cancel(gatt, chrc->write_op->id);
> > > > > > > > > >
> > > > > > > > > > - queue_remove_all(chrc->notify_clients, NULL, NULL, remove_client);
> > > > > > > > > > queue_remove_all(chrc->descs, NULL, NULL, unregister_descriptor);
> > > > > > > > > >
> > > > > > > > > > g_dbus_unregister_interface(btd_get_dbus_connection(), chrc->path,
> > > > > > > > > > --
> > > > > > > > > > 2.17.2
> >
> > Applied.
> >
> > --
> > Luiz Augusto von Dentz

2019-01-01 19:27:28

by Luiz Augusto von Dentz

Subject: Re: [PATCH BlueZ 1/5] gatt: Fix invalid read when disconnecting

Hi Gal,

On Sat, Dec 22, 2018 at 3:51 PM Gal Ben Haim <[email protected]> wrote:
>
> it definitely happens with this patch and not with mine

Can you try with valgrind so we got to see if the backtrace is really
the same, also is that using GATT caching or is that disabled?

> On Sat, Dec 15, 2018 at 8:45 AM Gal Ben Haim <[email protected]> wrote:
> >
> > I still get segmentation faults occasionally after applying this patch,
> > i'm not 100% sure that it's related but it happens in the same area...
> > its not something that I can reproduce immediately like with the
> > previous segfault. I think that it doesn't happen if I use my original
> > patch https://marc.info/?l=linux-bluetooth&m=154250398526173&w=2
> >
> > bluetoothd[2599]: src/device.c:att_disconnected_cb()
> > bluetoothd[2599]: src/device.c:att_disconnected_cb() Connection reset
> > by peer (104)
> > bluetoothd[2599]: src/service.c:change_state() 0x4edae0: device
> > DA:51:61:24:3C:36 profile gap-profile state changed: connected ->
> > disconnecting (0)
> > bluetoothd[2599]: src/service.c:change_state() 0x4edae0: device
> > DA:51:61:24:3C:36 profile gap-profile state changed: disconnecting ->
> > disconnected (0)
> > bluetoothd[2599]: src/gatt-client.c:btd_gatt_client_disconnected()
> > Device disconnected. Cleaning up.
> > bluetoothd[2599]: src/device.c:att_disconnected_cb() Automatic
> > connection disabled
> > bluetoothd[2599]: src/gatt-database.c:btd_gatt_database_att_disconnected()
> > bluetoothd[2599]: attrib/gattrib.c:g_attrib_unref() 0x4ee410: g_attrib_unref=0
> > bluetoothd[2599]: src/device.c:btd_device_set_temporary() temporary 1
> > bluetoothd[2599]: src/device.c:device_remove() Removing device
> > /org/bluez/hci0/dev_DA_51_61_24_3C_36
> > bluetoothd[2599]: src/service.c:change_state() 0x4edae0: device
> > DA:51:61:24:3C:36 profile gap-profile state changed: disconnected ->
> > unavailable (0)
> > bluetoothd[2599]: profiles/gap/gas.c:gap_remove() GAP profile remove
> > (DA:51:61:24:3C:36)
> > bluetoothd[2599]: src/service.c:btd_service_unref() 0x4edae0: ref=0
> > bluetoothd[2599]: src/device.c:btd_device_unref() Freeing device
> > /org/bluez/hci0/dev_DA_51_61_24_3C_36
> > bluetoothd[2599]: src/gatt-client.c:unregister_service() Removing GATT
> > service: /org/bluez/hci0/dev_DA_51_61_24_3C_36/service0008
> > bluetoothd[2599]: src/gatt-client.c:unregister_service() Removing GATT
> > service: /org/bluez/hci0/dev_DA_51_61_24_3C_36/service0009
> > bluetoothd[2599]: src/gatt-client.c:unregister_characteristic()
> > Removing GATT characteristic:
> > /org/bluez/hci0/dev_DA_51_61_24_3C_36/service0009/char000a
> > bluetoothd[2599]: src/gatt-client.c:unregister_characteristic()
> > Removing GATT characteristic:
> > /org/bluez/hci0/dev_DA_51_61_24_3C_36/service0009/char000c
> > bluetoothd[2599]: src/gatt-client.c:unregister_descriptor() Removing
> > GATT descriptor:
> > /org/bluez/hci0/dev_DA_51_61_24_3C_36/service0009/char000c/desc000e
> > Segmentation fault
> >
> >
> > On Wed, Nov 21, 2018 at 11:15 AM Luiz Augusto von Dentz
> > <[email protected]> wrote:
> > >
> > > Hi,
> > > On Wed, Nov 21, 2018 at 9:15 AM Gal Ben Haim <[email protected]> wrote:
> > > >
> > > > it fixes the same case that cause the version from master to crash.
> > > > i haven't tested for anything else..
> > > >
> > > > On Tue, Nov 20, 2018 at 8:31 PM Gal Ben Haim <[email protected]> wrote:
> > > > >
> > > > > ok I patched it. i'll test it tomorrow in the office and let you know.
> > > > >
> > > > > On Tue, Nov 20, 2018 at 3:59 PM Luiz Augusto von Dentz
> > > > > <[email protected]> wrote:
> > > > > >
> > > > > > Hi Gal,
> > > > > >
> > > > > > On Tue, Nov 20, 2018 at 2:35 PM Gal Ben Haim <[email protected]> wrote:
> > > > > > >
> > > > > > > i tried with both git am and git apply
> > > > > >
> > > > > > Does that applies to v2 as well? I just rebased it on top master, are
> > > > > > you sure you don't have your own changes conflicting with it?
> > > > > >
> > > > > > > On Tue, Nov 20, 2018 at 12:35 PM Luiz Augusto von Dentz
> > > > > > > <[email protected]> wrote:
> > > > > > > >
> > > > > > > > Hi Gal,
> > > > > > > > On Tue, Nov 20, 2018 at 11:31 AM Gal Ben Haim <[email protected]> wrote:
> > > > > > > > >
> > > > > > > > > I can't apply this patch, not to the revision in master and not to the 5.50 tag
> > > > > > > >
> > > > > > > > They are on top o master, how are you trying to apply them, with git am?
> > > > > > > >
> > > > > > > > > On Tue, Nov 20, 2018 at 10:50 AM Luiz Augusto von Dentz
> > > > > > > > > <[email protected]> wrote:
> > > > > > > > > >
> > > > > > > > > > On Mon, Nov 19, 2018 at 5:43 PM Luiz Augusto von Dentz
> > > > > > > > > > <[email protected]> wrote:
> > > > > > > > > > >
> > > > > > > > > > > From: Luiz Augusto von Dentz <[email protected]>
> > > > > > > > > > >
> > > > > > > > > > > In case there is a client of AcquireNotify and a disconnect happens the
> > > > > > > > > > > code not only have to free the client object but also destroy the io
> > > > > > > > > > > associated with it, for this reason the client object cannot be freed
> > > > > > > > > > > until the io is destroyed otherwise it may lead to the following error:
> > > > > > > > > > >
> > > > > > > > > > > Invalid read of size 4
> > > > > > > > > > > at 0x63920: notify_io_destroy (gatt-client.c:1461)
> > > > > > > > > > > by 0x63EDB: pipe_io_destroy (gatt-client.c:1082)
> > > > > > > > > > > by 0x6405B: characteristic_free (gatt-client.c:1663)
> > > > > > > > > > > by 0x81F33: remove_interface (object.c:667)
> > > > > > > > > > > by 0x826CB: g_dbus_unregister_interface (object.c:1391)
> > > > > > > > > > > by 0x85D2B: queue_remove_all (queue.c:354)
> > > > > > > > > > > by 0x635F7: unregister_service (gatt-client.c:1893)
> > > > > > > > > > > by 0x85CF7: queue_remove_all (queue.c:339)
> > > > > > > > > > > by 0x661DF: btd_gatt_client_service_removed (gatt-client.c:2199)
> > > > > > > > > > > by 0x695CB: gatt_service_removed (device.c:3747)
> > > > > > > > > > > by 0x85B17: queue_foreach (queue.c:220)
> > > > > > > > > > > by 0x91283: notify_service_changed (gatt-db.c:280)
> > > > > > > > > > > by 0x91283: gatt_db_service_destroy (gatt-db.c:291)
> > > > > > > > > > > Address 0x515ed48 is 0 bytes inside a block of size 20 free'd
> > > > > > > > > > > at 0x483EAD0: free (vg_replace_malloc.c:530)
> > > > > > > > > > > by 0x85D2B: queue_remove_all (queue.c:354)
> > > > > > > > > > > by 0x636D3: unregister_characteristic (gatt-client.c:1741)
> > > > > > > > > > > by 0x85D2B: queue_remove_all (queue.c:354)
> > > > > > > > > > > by 0x635F7: unregister_service (gatt-client.c:1893)
> > > > > > > > > > > by 0x85CF7: queue_remove_all (queue.c:339)
> > > > > > > > > > > by 0x661DF: btd_gatt_client_service_removed (gatt-client.c:2199)
> > > > > > > > > > > by 0x695CB: gatt_service_removed (device.c:3747)
> > > > > > > > > > > by 0x85B17: queue_foreach (queue.c:220)
> > > > > > > > > > > by 0x91283: notify_service_changed (gatt-db.c:280)
> > > > > > > > > > > by 0x91283: gatt_db_service_destroy (gatt-db.c:291)
> > > > > > > > > > > by 0x85D2B: queue_remove_all (queue.c:354)
> > > > > > > > > > > by 0x91387: gatt_db_clear_range (gatt-db.c:475)
> > > > > > > > > > > ---
> > > > > > > > > > > src/gatt-client.c | 24 ++++++++++++------------
> > > > > > > > > > > 1 file changed, 12 insertions(+), 12 deletions(-)
> > > > > > > > > > >
> > > > > > > > > > > diff --git a/src/gatt-client.c b/src/gatt-client.c
> > > > > > > > > > > index 234f46ed7..55aa5e423 100644
> > > > > > > > > > > --- a/src/gatt-client.c
> > > > > > > > > > > +++ b/src/gatt-client.c
> > > > > > > > > > > @@ -1645,13 +1645,22 @@ static const GDBusMethodTable characteristic_methods[] = {
> > > > > > > > > > > { }
> > > > > > > > > > > };
> > > > > > > > > > >
> > > > > > > > > > > +static void remove_client(void *data)
> > > > > > > > > > > +{
> > > > > > > > > > > + struct notify_client *ntfy_client = data;
> > > > > > > > > > > + struct btd_gatt_client *client = ntfy_client->chrc->service->client;
> > > > > > > > > > > +
> > > > > > > > > > > + queue_remove(client->all_notify_clients, ntfy_client);
> > > > > > > > > > > +
> > > > > > > > > > > + notify_client_unref(ntfy_client);
> > > > > > > > > > > +}
> > > > > > > > > > > +
> > > > > > > > > > > static void characteristic_free(void *data)
> > > > > > > > > > > {
> > > > > > > > > > > struct characteristic *chrc = data;
> > > > > > > > > > >
> > > > > > > > > > > /* List should be empty here */
> > > > > > > > > > > queue_destroy(chrc->descs, NULL);
> > > > > > > > > > > - queue_destroy(chrc->notify_clients, NULL);
> > > > > > > > > > >
> > > > > > > > > > > if (chrc->write_io) {
> > > > > > > > > > > queue_remove(chrc->service->client->ios, chrc->write_io->io);
> > > > > > > > > > > @@ -1663,6 +1672,8 @@ static void characteristic_free(void *data)
> > > > > > > > > > > pipe_io_destroy(chrc->notify_io);
> > > > > > > > > > > }
> > > > > > > > > > >
> > > > > > > > > > > + queue_destroy(chrc->notify_clients, remove_client);
> > > > > > > > > > > +
> > > > > > > > > > > g_free(chrc->path);
> > > > > > > > > > > free(chrc);
> > > > > > > > > > > }
> > > > > > > > > > > @@ -1715,16 +1726,6 @@ static struct characteristic *characteristic_create(
> > > > > > > > > > > return chrc;
> > > > > > > > > > > }
> > > > > > > > > > >
> > > > > > > > > > > -static void remove_client(void *data)
> > > > > > > > > > > -{
> > > > > > > > > > > - struct notify_client *ntfy_client = data;
> > > > > > > > > > > - struct btd_gatt_client *client = ntfy_client->chrc->service->client;
> > > > > > > > > > > -
> > > > > > > > > > > - queue_remove(client->all_notify_clients, ntfy_client);
> > > > > > > > > > > -
> > > > > > > > > > > - notify_client_unref(ntfy_client);
> > > > > > > > > > > -}
> > > > > > > > > > > -
> > > > > > > > > > > static void unregister_characteristic(void *data)
> > > > > > > > > > > {
> > > > > > > > > > > struct characteristic *chrc = data;
> > > > > > > > > > > @@ -1738,7 +1739,6 @@ static void unregister_characteristic(void *data)
> > > > > > > > > > > if (chrc->write_op)
> > > > > > > > > > > bt_gatt_client_cancel(gatt, chrc->write_op->id);
> > > > > > > > > > >
> > > > > > > > > > > - queue_remove_all(chrc->notify_clients, NULL, NULL, remove_client);
> > > > > > > > > > > queue_remove_all(chrc->descs, NULL, NULL, unregister_descriptor);
> > > > > > > > > > >
> > > > > > > > > > > g_dbus_unregister_interface(btd_get_dbus_connection(), chrc->path,
> > > > > > > > > > > --
> > > > > > > > > > > 2.17.2
> > >
> > > Applied.
> > >
> > > --
> > > Luiz Augusto von Dentz



--
Luiz Augusto von Dentz

2019-02-05 07:12:01

by Gal Ben Haim

Subject: Re: [PATCH BlueZ 1/5] gatt: Fix invalid read when disconnecting

I didn't get to it... with the current patch BlueZ definitely crashes,
but this is without newer patches if anything related was changed.

On Sun, Jan 6, 2019 at 11:04 AM Gal Ben Haim <[email protected]> wrote:
>
> Yes, it is with GATT caching enabled. However, we are deleting the device on disconnection, if its related...
>
> It will take me some time to get to Valgrind.
>
> On Tue, Jan 1, 2019, 21:27 Luiz Augusto von Dentz <[email protected] wrote:
>>
>> Hi Gal,
>>
>> On Sat, Dec 22, 2018 at 3:51 PM Gal Ben Haim <[email protected]> wrote:
>> >
>> > it definitely happens with this patch and not with mine
>>
>> Can you try with valgrind so we got to see if the backtrace is really
>> the same, also is that using GATT caching or is that disabled?
>>
>> > On Sat, Dec 15, 2018 at 8:45 AM Gal Ben Haim <[email protected]> wrote:
>> > >
>> > > I still get segmentation faults occasionally after applying this patch,
>> > > i'm not 100% sure that it's related but it happens in the same area...
>> > > its not something that I can reproduce immediately like with the
>> > > previous segfault. I think that it doesn't happen if I use my original
>> > > patch https://marc.info/?l=linux-bluetooth&m=154250398526173&w=2
>> > >
>> > > bluetoothd[2599]: src/device.c:att_disconnected_cb()
>> > > bluetoothd[2599]: src/device.c:att_disconnected_cb() Connection reset
>> > > by peer (104)
>> > > bluetoothd[2599]: src/service.c:change_state() 0x4edae0: device
>> > > DA:51:61:24:3C:36 profile gap-profile state changed: connected ->
>> > > disconnecting (0)
>> > > bluetoothd[2599]: src/service.c:change_state() 0x4edae0: device
>> > > DA:51:61:24:3C:36 profile gap-profile state changed: disconnecting ->
>> > > disconnected (0)
>> > > bluetoothd[2599]: src/gatt-client.c:btd_gatt_client_disconnected()
>> > > Device disconnected. Cleaning up.
>> > > bluetoothd[2599]: src/device.c:att_disconnected_cb() Automatic
>> > > connection disabled
>> > > bluetoothd[2599]: src/gatt-database.c:btd_gatt_database_att_disconnected()
>> > > bluetoothd[2599]: attrib/gattrib.c:g_attrib_unref() 0x4ee410: g_attrib_unref=0
>> > > bluetoothd[2599]: src/device.c:btd_device_set_temporary() temporary 1
>> > > bluetoothd[2599]: src/device.c:device_remove() Removing device
>> > > /org/bluez/hci0/dev_DA_51_61_24_3C_36
>> > > bluetoothd[2599]: src/service.c:change_state() 0x4edae0: device
>> > > DA:51:61:24:3C:36 profile gap-profile state changed: disconnected ->
>> > > unavailable (0)
>> > > bluetoothd[2599]: profiles/gap/gas.c:gap_remove() GAP profile remove
>> > > (DA:51:61:24:3C:36)
>> > > bluetoothd[2599]: src/service.c:btd_service_unref() 0x4edae0: ref=0
>> > > bluetoothd[2599]: src/device.c:btd_device_unref() Freeing device
>> > > /org/bluez/hci0/dev_DA_51_61_24_3C_36
>> > > bluetoothd[2599]: src/gatt-client.c:unregister_service() Removing GATT
>> > > service: /org/bluez/hci0/dev_DA_51_61_24_3C_36/service0008
>> > > bluetoothd[2599]: src/gatt-client.c:unregister_service() Removing GATT
>> > > service: /org/bluez/hci0/dev_DA_51_61_24_3C_36/service0009
>> > > bluetoothd[2599]: src/gatt-client.c:unregister_characteristic()
>> > > Removing GATT characteristic:
>> > > /org/bluez/hci0/dev_DA_51_61_24_3C_36/service0009/char000a
>> > > bluetoothd[2599]: src/gatt-client.c:unregister_characteristic()
>> > > Removing GATT characteristic:
>> > > /org/bluez/hci0/dev_DA_51_61_24_3C_36/service0009/char000c
>> > > bluetoothd[2599]: src/gatt-client.c:unregister_descriptor() Removing
>> > > GATT descriptor:
>> > > /org/bluez/hci0/dev_DA_51_61_24_3C_36/service0009/char000c/desc000e
>> > > Segmentation fault
>> > >
>> > >
>> > > On Wed, Nov 21, 2018 at 11:15 AM Luiz Augusto von Dentz
>> > > <[email protected]> wrote:
>> > > >
>> > > > Hi,
>> > > > On Wed, Nov 21, 2018 at 9:15 AM Gal Ben Haim <[email protected]> wrote:
>> > > > >
>> > > > > it fixes the same case that cause the version from master to crash.
>> > > > > i haven't tested for anything else..
>> > > > >
>> > > > > On Tue, Nov 20, 2018 at 8:31 PM Gal Ben Haim <[email protected]> wrote:
>> > > > > >
>> > > > > > ok I patched it. i'll test it tomorrow in the office and let you know.
>> > > > > >
>> > > > > > On Tue, Nov 20, 2018 at 3:59 PM Luiz Augusto von Dentz
>> > > > > > <[email protected]> wrote:
>> > > > > > >
>> > > > > > > Hi Gal,
>> > > > > > >
>> > > > > > > On Tue, Nov 20, 2018 at 2:35 PM Gal Ben Haim <[email protected]> wrote:
>> > > > > > > >
>> > > > > > > > i tried with both git am and git apply
>> > > > > > >
>> > > > > > > Does that applies to v2 as well? I just rebased it on top master, are
>> > > > > > > you sure you don't have your own changes conflicting with it?
>> > > > > > >
>> > > > > > > > On Tue, Nov 20, 2018 at 12:35 PM Luiz Augusto von Dentz
>> > > > > > > > <[email protected]> wrote:
>> > > > > > > > >
>> > > > > > > > > Hi Gal,
>> > > > > > > > > On Tue, Nov 20, 2018 at 11:31 AM Gal Ben Haim <[email protected]> wrote:
>> > > > > > > > > >
>> > > > > > > > > > I can't apply this patch, not to the revision in master and not to the 5.50 tag
>> > > > > > > > >
>> > > > > > > > > They are on top o master, how are you trying to apply them, with git am?
>> > > > > > > > >
>> > > > > > > > > > On Tue, Nov 20, 2018 at 10:50 AM Luiz Augusto von Dentz
>> > > > > > > > > > <[email protected]> wrote:
>> > > > > > > > > > >
>> > > > > > > > > > > On Mon, Nov 19, 2018 at 5:43 PM Luiz Augusto von Dentz
>> > > > > > > > > > > <[email protected]> wrote:
>> > > > > > > > > > > >
>> > > > > > > > > > > > From: Luiz Augusto von Dentz <[email protected]>
>> > > > > > > > > > > >
>> > > > > > > > > > > > In case there is a client of AcquireNotify and a disconnect happens the
>> > > > > > > > > > > > code not only have to free the client object but also destroy the io
>> > > > > > > > > > > > associated with it, for this reason the client object cannot be freed
>> > > > > > > > > > > > until the io is destroyed otherwise it may lead to the following error:
>> > > > > > > > > > > >
>> > > > > > > > > > > > Invalid read of size 4
>> > > > > > > > > > > > at 0x63920: notify_io_destroy (gatt-client.c:1461)
>> > > > > > > > > > > > by 0x63EDB: pipe_io_destroy (gatt-client.c:1082)
>> > > > > > > > > > > > by 0x6405B: characteristic_free (gatt-client.c:1663)
>> > > > > > > > > > > > by 0x81F33: remove_interface (object.c:667)
>> > > > > > > > > > > > by 0x826CB: g_dbus_unregister_interface (object.c:1391)
>> > > > > > > > > > > > by 0x85D2B: queue_remove_all (queue.c:354)
>> > > > > > > > > > > > by 0x635F7: unregister_service (gatt-client.c:1893)
>> > > > > > > > > > > > by 0x85CF7: queue_remove_all (queue.c:339)
>> > > > > > > > > > > > by 0x661DF: btd_gatt_client_service_removed (gatt-client.c:2199)
>> > > > > > > > > > > > by 0x695CB: gatt_service_removed (device.c:3747)
>> > > > > > > > > > > > by 0x85B17: queue_foreach (queue.c:220)
>> > > > > > > > > > > > by 0x91283: notify_service_changed (gatt-db.c:280)
>> > > > > > > > > > > > by 0x91283: gatt_db_service_destroy (gatt-db.c:291)
>> > > > > > > > > > > > Address 0x515ed48 is 0 bytes inside a block of size 20 free'd
>> > > > > > > > > > > > at 0x483EAD0: free (vg_replace_malloc.c:530)
>> > > > > > > > > > > > by 0x85D2B: queue_remove_all (queue.c:354)
>> > > > > > > > > > > > by 0x636D3: unregister_characteristic (gatt-client.c:1741)
>> > > > > > > > > > > > by 0x85D2B: queue_remove_all (queue.c:354)
>> > > > > > > > > > > > by 0x635F7: unregister_service (gatt-client.c:1893)
>> > > > > > > > > > > > by 0x85CF7: queue_remove_all (queue.c:339)
>> > > > > > > > > > > > by 0x661DF: btd_gatt_client_service_removed (gatt-client.c:2199)
>> > > > > > > > > > > > by 0x695CB: gatt_service_removed (device.c:3747)
>> > > > > > > > > > > > by 0x85B17: queue_foreach (queue.c:220)
>> > > > > > > > > > > > by 0x91283: notify_service_changed (gatt-db.c:280)
>> > > > > > > > > > > > by 0x91283: gatt_db_service_destroy (gatt-db.c:291)
>> > > > > > > > > > > > by 0x85D2B: queue_remove_all (queue.c:354)
>> > > > > > > > > > > > by 0x91387: gatt_db_clear_range (gatt-db.c:475)
>> > > > > > > > > > > > ---
>> > > > > > > > > > > > src/gatt-client.c | 24 ++++++++++++------------
>> > > > > > > > > > > > 1 file changed, 12 insertions(+), 12 deletions(-)
>> > > > > > > > > > > >
>> > > > > > > > > > > > diff --git a/src/gatt-client.c b/src/gatt-client.c
>> > > > > > > > > > > > index 234f46ed7..55aa5e423 100644
>> > > > > > > > > > > > --- a/src/gatt-client.c
>> > > > > > > > > > > > +++ b/src/gatt-client.c
>> > > > > > > > > > > > @@ -1645,13 +1645,22 @@ static const GDBusMethodTable characteristic_methods[] = {
>> > > > > > > > > > > > { }
>> > > > > > > > > > > > };
>> > > > > > > > > > > >
>> > > > > > > > > > > > +static void remove_client(void *data)
>> > > > > > > > > > > > +{
>> > > > > > > > > > > > + struct notify_client *ntfy_client = data;
>> > > > > > > > > > > > + struct btd_gatt_client *client = ntfy_client->chrc->service->client;
>> > > > > > > > > > > > +
>> > > > > > > > > > > > + queue_remove(client->all_notify_clients, ntfy_client);
>> > > > > > > > > > > > +
>> > > > > > > > > > > > + notify_client_unref(ntfy_client);
>> > > > > > > > > > > > +}
>> > > > > > > > > > > > +
>> > > > > > > > > > > > static void characteristic_free(void *data)
>> > > > > > > > > > > > {
>> > > > > > > > > > > > struct characteristic *chrc = data;
>> > > > > > > > > > > >
>> > > > > > > > > > > > /* List should be empty here */
>> > > > > > > > > > > > queue_destroy(chrc->descs, NULL);
>> > > > > > > > > > > > - queue_destroy(chrc->notify_clients, NULL);
>> > > > > > > > > > > >
>> > > > > > > > > > > > if (chrc->write_io) {
>> > > > > > > > > > > > queue_remove(chrc->service->client->ios, chrc->write_io->io);
>> > > > > > > > > > > > @@ -1663,6 +1672,8 @@ static void characteristic_free(void *data)
>> > > > > > > > > > > > pipe_io_destroy(chrc->notify_io);
>> > > > > > > > > > > > }
>> > > > > > > > > > > >
>> > > > > > > > > > > > + queue_destroy(chrc->notify_clients, remove_client);
>> > > > > > > > > > > > +
>> > > > > > > > > > > > g_free(chrc->path);
>> > > > > > > > > > > > free(chrc);
>> > > > > > > > > > > > }
>> > > > > > > > > > > > @@ -1715,16 +1726,6 @@ static struct characteristic *characteristic_create(
>> > > > > > > > > > > > return chrc;
>> > > > > > > > > > > > }
>> > > > > > > > > > > >
>> > > > > > > > > > > > -static void remove_client(void *data)
>> > > > > > > > > > > > -{
>> > > > > > > > > > > > - struct notify_client *ntfy_client = data;
>> > > > > > > > > > > > - struct btd_gatt_client *client = ntfy_client->chrc->service->client;
>> > > > > > > > > > > > -
>> > > > > > > > > > > > - queue_remove(client->all_notify_clients, ntfy_client);
>> > > > > > > > > > > > -
>> > > > > > > > > > > > - notify_client_unref(ntfy_client);
>> > > > > > > > > > > > -}
>> > > > > > > > > > > > -
>> > > > > > > > > > > > static void unregister_characteristic(void *data)
>> > > > > > > > > > > > {
>> > > > > > > > > > > > struct characteristic *chrc = data;
>> > > > > > > > > > > > @@ -1738,7 +1739,6 @@ static void unregister_characteristic(void *data)
>> > > > > > > > > > > > if (chrc->write_op)
>> > > > > > > > > > > > bt_gatt_client_cancel(gatt, chrc->write_op->id);
>> > > > > > > > > > > >
>> > > > > > > > > > > > - queue_remove_all(chrc->notify_clients, NULL, NULL, remove_client);
>> > > > > > > > > > > > queue_remove_all(chrc->descs, NULL, NULL, unregister_descriptor);
>> > > > > > > > > > > >
>> > > > > > > > > > > > g_dbus_unregister_interface(btd_get_dbus_connection(), chrc->path,
>> > > > > > > > > > > > --
>> > > > > > > > > > > > 2.17.2
>> > > >
>> > > > Applied.
>> > > >
>> > > > --
>> > > > Luiz Augusto von Dentz
>>
>>
>>
>> --
>> Luiz Augusto von Dentz