Hi Bernie,
On Fri, Oct 8, 2021 at 12:24 PM Luiz Augusto von Dentz
<luiz.dentz@xxxxxxxxx> wrote:
>
> 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.
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?
> > @@ -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
