Hi Bernie,
On Fri, Oct 8, 2021 at 12:01 PM Bernie Conrad <bernie@xxxxxxxxxxxxxxxxx> wrote:
>
> From: Bernie Conrad <bernie@xxxxxxxxxxxxxxxxx>
>
> 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.
> 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.
> @@ -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
