Re: [BlueZ v2] gatt: more fixes with cleanup on disconnect/timeout
From: Luiz Augusto von Dentz <luiz.dentz@gmail.com>
Date: 2021-10-08 19:24:16
Hi Bernie, On Fri, Oct 8, 2021 at 12:01 PM Bernie Conrad [off-list ref] wrote:
quoted hunk ↗ jump to hunk
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 hunk ↗ jump to hunk
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.
quoted hunk ↗ jump to hunk
@@ -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