Re: [BlueZ v2] gatt: more fixes with cleanup on disconnect/timeout

Hi Bernie,

On Fri, Oct 8, 2021 at 12:24 PM Luiz Augusto von Dentz
[off-list ref] wrote:

Hi Bernie,

On Fri, Oct 8, 2021 at 12:01 PM Bernie Conrad [off-list ref] wrote:

quoted

From: Bernie Conrad <redacted>

The changes in gatt-database.c fix a use after free that was introduced
after the last cleanup patch, ccc_new and write_new operations were not
being properly unregistered because they were not assigned a disconn_id.

The changes in gatt-db add similar cleanup to pending reads/writes where
timeouts after a disconnect would cause a similar use after free with
already cleaned up resoureces, this adds a simple cb to set on a pending
read/write if a disconnect has occurred to skip the use.

v2: Fixing formatting issues

---
 src/gatt-database.c  |  4 ++--
 src/shared/gatt-db.c | 38 ++++++++++++++++++++++++++++++++++++--
 2 files changed, 38 insertions(+), 4 deletions(-)

diff --git a/src/gatt-database.c b/src/gatt-database.c
index 475e7873c..00647cf08 100644
--- a/src/gatt-database.c
+++ b/src/gatt-database.c

@@ -978,7 +978,7 @@ static struct pending_op *pending_ccc_new(struct bt_att *att,
        op->attrib = attrib;
        op->link_type = link_type;

-       bt_att_register_disconnect(att,
+       op->disconn_id = bt_att_register_disconnect(att,
                                   pending_disconnect_cb,
                                   op,
                                   NULL);

@@ -2418,7 +2418,7 @@ static struct pending_op *pending_write_new(struct bt_att *att,
        op->prep_authorize = prep_authorize;
        queue_push_tail(owner_queue, op);

-       bt_att_register_disconnect(att,
+       op->disconn_id = bt_att_register_disconnect(att,
                            pending_disconnect_cb,
                            op, NULL);

These changes above shall be split into another patch.

quoted

diff --git a/src/shared/gatt-db.c b/src/shared/gatt-db.c
index 3a02289ce..8423961f8 100644
--- a/src/shared/gatt-db.c
+++ b/src/shared/gatt-db.c

@@ -77,17 +77,23 @@ struct attribute_notify {

 struct pending_read {
        struct gatt_db_attribute *attrib;
+       struct bt_att *att;
        unsigned int id;
        unsigned int timeout_id;
        gatt_db_attribute_read_t func;
+       bool disconn;
+       unsigned int disconn_id;
        void *user_data;
 };

 struct pending_write {
        struct gatt_db_attribute *attrib;
+       struct bt_att *att;
        unsigned int id;
        unsigned int timeout_id;
        gatt_db_attribute_write_t func;
+       bool disconn;
+       unsigned int disconn_id;
        void *user_data;
 };

@@ -139,8 +145,10 @@ static void pending_read_result(struct pending_read *p, int err,
        if (p->timeout_id > 0)
                timeout_remove(p->timeout_id);

-       p->func(p->attrib, err, data, length, p->user_data);
+       if (!p->disconn)
+               p->func(p->attrib, err, data, length, p->user_data);

+       bt_att_unregister_disconnect(p->att, p->disconn_id);
        free(p);
 }

@@ -156,8 +164,10 @@ static void pending_write_result(struct pending_write *p, int err)
        if (p->timeout_id > 0)
                timeout_remove(p->timeout_id);

-       p->func(p->attrib, err, p->user_data);
+       if (!p->disconn)
+               p->func(p->attrib, err, p->user_data);

+       bt_att_unregister_disconnect(p->att, p->disconn_id);
        free(p);
 }

I wonder if it wouldn't be better to use a specific error to inform it
the operation has been aborted e.g. -ECONNABORTED instead of
duplicating the handling of disconnection, btw if we don't call the
callback who is doing the cleanup in gatt-server.c, we still need to
call async_read_op_destroy/async_write_op_destroy or that is taken
care somewhere else?

Also it would be great if we had a test in unit/test-gatt.c that
covers such scenarios, e.g disconnect while read/write is pending.

Is there something not clear about the direction given here, or they
are understood but you still haven't had time to work on them?

quoted

@@ -1868,6 +1878,13 @@ bool gatt_db_attribute_set_fixed_length(struct gatt_db_attribute *attrib,
        return true;
 }

+static void pending_read_cb(int err, void *user_data)
+{
+       struct pending_read *p = user_data;
+
+       p->disconn = 1;
+}
+
 bool gatt_db_attribute_read(struct gatt_db_attribute *attrib, uint16_t offset,
                                uint8_t opcode, struct bt_att *att,
                                gatt_db_attribute_read_t func, void *user_data)

@@ -1901,6 +1918,11 @@ bool gatt_db_attribute_read(struct gatt_db_attribute *attrib, uint16_t offset,
                p->func = func;
                p->user_data = user_data;

+               p->disconn = 0;
+               p->disconn_id = bt_att_register_disconnect(att,
+                                       pending_read_cb, p, NULL);
+               p->att = att;
+
                queue_push_tail(attrib->pending_reads, p);

                attrib->read_func(attrib, p->id, offset, opcode, att,

@@ -1956,6 +1978,13 @@ static bool write_timeout(void *user_data)
        return false;
 }

+static void pending_write_cb(int err, void *user_data)
+{
+       struct pending_write *p = user_data;
+
+       p->disconn = 1;
+}
+
 bool gatt_db_attribute_write(struct gatt_db_attribute *attrib, uint16_t offset,
                                        const uint8_t *value, size_t len,
                                        uint8_t opcode, struct bt_att *att,

@@ -1995,6 +2024,11 @@ bool gatt_db_attribute_write(struct gatt_db_attribute *attrib, uint16_t offset,
                p->func = func;
                p->user_data = user_data;

+               p->disconn = 0;
+               p->disconn_id = bt_att_register_disconnect(att,
+                                       pending_write_cb, p, NULL);
+               p->att = att;
+
                queue_push_tail(attrib->pending_writes, p);

                attrib->write_func(attrib, p->id, offset, value, len, opcode,
--

2.17.1

--
Luiz Augusto von Dentz

-- 
Luiz Augusto von Dentz