| Message ID | 20200420135112.6749-2-scerveau@collabora.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Luiz Von Dentz |
| Headers | show |
| Series | add hog ref in hog_attach_instance | expand |
Hi Stéphane, On Mon, Apr 20, 2020 at 9:39 AM Stéphane Cerveau <scerveau@collabora.com> wrote: > > To avoid a double hog free, need to add a ref > when adding the hog to the slist. > > This bug has been reproduced with gamepad-8718 > which was connecting/disconnecting frantically. > > Fix also a typo in the method hog_attach_instance > --- > profiles/input/hog-lib.c | 8 ++++---- > 1 file changed, 4 insertions(+), 4 deletions(-) > > diff --git a/profiles/input/hog-lib.c b/profiles/input/hog-lib.c > index 9c5c814a7..b9b5d565c 100644 > --- a/profiles/input/hog-lib.c > +++ b/profiles/input/hog-lib.c > @@ -1357,7 +1357,7 @@ static struct bt_hog *hog_new(int fd, const char *name, uint16_t vendor, > return hog; > } > > -static void hog_attach_instace(struct bt_hog *hog, > +static void hog_attach_instance(struct bt_hog *hog, > struct gatt_db_attribute *attr) > { > struct bt_hog *instance; > @@ -1373,14 +1373,14 @@ static void hog_attach_instace(struct bt_hog *hog, > if (!instance) > return; > > - hog->instances = g_slist_append(hog->instances, instance); > + hog->instances = g_slist_append(hog->instances, bt_hog_ref(instance)); That sounds like like a double ref since bt_hog_new already does add a reference, so chances are that you may be fixing the the symptom not the real problem which seems that we cannot unref the instances because they are not removed from the parent as it they should, in fact we might need to redesign it a little bit since bt_hog might actually be inefficient when there are multiple instances as it does allocate a new uhid instance of each of then so we might do something like: struct bt_hog_instance { struct bt_hog *parent; struct gatt_db_attribute *attr; struct gatt_primary *primary; GAttrib *attrib; GSList *reports; gboolean has_report_id; uint16_t bcdhid; uint8_t bcountrycode; uint16_t proto_mode_handle; uint16_t ctrlpt_handle; uint8_t flags; unsigned int getrep_att; uint16_t getrep_id; unsigned int setrep_att; uint16_t setrep_id; } That way the instances are indenpendent of the bt_hog which should be 1:1 to a device, while we can have multple instances of bt_hog_instance, if you don't have time to do the rework then lets just add a parent pointer added to bt_hog so we can can remove child instances and resolve the double free. > } > > static void foreach_hog_service(struct gatt_db_attribute *attr, void *user_data) > { > struct bt_hog *hog = user_data; > > - hog_attach_instace(hog, attr); > + hog_attach_instance(hog, attr); > } > > static void dis_notify(uint8_t source, uint16_t vendor, uint16_t product, > @@ -1528,7 +1528,7 @@ static void hog_attach_hog(struct bt_hog *hog, struct gatt_primary *primary) > primary->range.end, find_included_cb, instance); > > bt_hog_attach(instance, hog->attrib); > - hog->instances = g_slist_append(hog->instances, instance); > + hog->instances = g_slist_append(hog->instances, bt_hog_ref(instance)); > } > > static void primary_cb(uint8_t status, GSList *services, void *user_data) > -- > 2.17.1 >
Dear Luiz, Thanks but see my comment below. On 20/4/20 19:21, Luiz Augusto von Dentz wrote: > Hi Stéphane, > > On Mon, Apr 20, 2020 at 9:39 AM Stéphane Cerveau <scerveau@collabora.com> wrote: >> >> To avoid a double hog free, need to add a ref >> when adding the hog to the slist. >> >> This bug has been reproduced with gamepad-8718 >> which was connecting/disconnecting frantically. >> >> Fix also a typo in the method hog_attach_instance >> --- >> profiles/input/hog-lib.c | 8 ++++---- >> 1 file changed, 4 insertions(+), 4 deletions(-) >> >> diff --git a/profiles/input/hog-lib.c b/profiles/input/hog-lib.c >> index 9c5c814a7..b9b5d565c 100644 >> --- a/profiles/input/hog-lib.c >> +++ b/profiles/input/hog-lib.c >> @@ -1357,7 +1357,7 @@ static struct bt_hog *hog_new(int fd, const char *name, uint16_t vendor, >> return hog; >> } >> >> -static void hog_attach_instace(struct bt_hog *hog, >> +static void hog_attach_instance(struct bt_hog *hog, >> struct gatt_db_attribute *attr) >> { >> struct bt_hog *instance; >> @@ -1373,14 +1373,14 @@ static void hog_attach_instace(struct bt_hog *hog, >> if (!instance) >> return; >> >> - hog->instances = g_slist_append(hog->instances, instance); >> + hog->instances = g_slist_append(hog->instances, bt_hog_ref(instance)); > > That sounds like like a double ref since bt_hog_new already does add a Yes but in `hog_attach_instance`, `hog_new` is used and not `bt_hog_new` which is indeed reffing. So I dont think there is double ref in this method. Unfortunately when I was preparing the patch, I noticed that another instance was added to slist and here you are totally right there is a double reference. I will remove this from the initial patch. > reference, so chances are that you may be fixing the the symptom not > the real problem which seems that we cannot unref the instances > because they are not removed from the parent as it they should, in > fact we might need to redesign it a little bit since bt_hog might > actually be inefficient when there are multiple instances as it does > allocate a new uhid instance of each of then so we might do something > like: > > struct bt_hog_instance { > struct bt_hog *parent; > struct gatt_db_attribute *attr; > struct gatt_primary *primary; > GAttrib *attrib; > GSList *reports; > gboolean has_report_id; > uint16_t bcdhid; > uint8_t bcountrycode; > uint16_t proto_mode_handle; > uint16_t ctrlpt_handle; > uint8_t flags; > unsigned int getrep_att; > uint16_t getrep_id; > unsigned int setrep_att; > uint16_t setrep_id; > } > > That way the instances are indenpendent of the bt_hog which should be > 1:1 to a device, while we can have multple instances of > bt_hog_instance, if you don't have time to do the rework then lets > just add a parent pointer added to bt_hog so we can can remove child > instances and resolve the double free. > >> } >> >> static void foreach_hog_service(struct gatt_db_attribute *attr, void *user_data) >> { >> struct bt_hog *hog = user_data; >> >> - hog_attach_instace(hog, attr); >> + hog_attach_instance(hog, attr); >> } >> >> static void dis_notify(uint8_t source, uint16_t vendor, uint16_t product, >> @@ -1528,7 +1528,7 @@ static void hog_attach_hog(struct bt_hog *hog, struct gatt_primary *primary) >> primary->range.end, find_included_cb, instance); >> >> bt_hog_attach(instance, hog->attrib); >> - hog->instances = g_slist_append(hog->instances, instance); >> + hog->instances = g_slist_append(hog->instances, bt_hog_ref(instance)); >> } >> >> static void primary_cb(uint8_t status, GSList *services, void *user_data) >> -- >> 2.17.1 >> > >
Hi Scerveau, On Mon, Apr 20, 2020 at 11:37 AM scerveau <scerveau@collabora.com> wrote: > > Dear Luiz, > > Thanks but see my comment below. > > On 20/4/20 19:21, Luiz Augusto von Dentz wrote: > > Hi Stéphane, > > > > On Mon, Apr 20, 2020 at 9:39 AM Stéphane Cerveau <scerveau@collabora.com> wrote: > >> > >> To avoid a double hog free, need to add a ref > >> when adding the hog to the slist. > >> > >> This bug has been reproduced with gamepad-8718 > >> which was connecting/disconnecting frantically. > >> > >> Fix also a typo in the method hog_attach_instance > >> --- > >> profiles/input/hog-lib.c | 8 ++++---- > >> 1 file changed, 4 insertions(+), 4 deletions(-) > >> > >> diff --git a/profiles/input/hog-lib.c b/profiles/input/hog-lib.c > >> index 9c5c814a7..b9b5d565c 100644 > >> --- a/profiles/input/hog-lib.c > >> +++ b/profiles/input/hog-lib.c > >> @@ -1357,7 +1357,7 @@ static struct bt_hog *hog_new(int fd, const char *name, uint16_t vendor, > >> return hog; > >> } > >> > >> -static void hog_attach_instace(struct bt_hog *hog, > >> +static void hog_attach_instance(struct bt_hog *hog, > >> struct gatt_db_attribute *attr) > >> { > >> struct bt_hog *instance; > >> @@ -1373,14 +1373,14 @@ static void hog_attach_instace(struct bt_hog *hog, > >> if (!instance) > >> return; > >> > >> - hog->instances = g_slist_append(hog->instances, instance); > >> + hog->instances = g_slist_append(hog->instances, bt_hog_ref(instance)); > > > > That sounds like like a double ref since bt_hog_new already does add a > > Yes but in `hog_attach_instance`, `hog_new` is used and not `bt_hog_new` > which is indeed reffing. So I dont think there is double ref in this method. Yep, you are rigth it is not adding a reference, but that seems on purpose since on it doesn't use bt_hog_unref: g_slist_free_full(hog->instances, hog_free); > Unfortunately when I was preparing the patch, I noticed that another > instance was added to slist and here you are totally right there is a > double reference. I will remove this from the initial patch. Right, I see now that is really missing the ref because destroy_gatt_req does actually unref its own reference but since we use hog_new that would destroy the instance so adding the ref should be the right way to fix this. > > > > reference, so chances are that you may be fixing the the symptom not > > the real problem which seems that we cannot unref the instances > > because they are not removed from the parent as it they should, in > > fact we might need to redesign it a little bit since bt_hog might > > actually be inefficient when there are multiple instances as it does > > allocate a new uhid instance of each of then so we might do something > > like: > > > > struct bt_hog_instance { > > struct bt_hog *parent; > > struct gatt_db_attribute *attr; > > struct gatt_primary *primary; > > GAttrib *attrib; > > GSList *reports; > > gboolean has_report_id; > > uint16_t bcdhid; > > uint8_t bcountrycode; > > uint16_t proto_mode_handle; > > uint16_t ctrlpt_handle; > > uint8_t flags; > > unsigned int getrep_att; > > uint16_t getrep_id; > > unsigned int setrep_att; > > uint16_t setrep_id; > > } > > > > That way the instances are indenpendent of the bt_hog which should be > > 1:1 to a device, while we can have multple instances of > > bt_hog_instance, if you don't have time to do the rework then lets > > just add a parent pointer added to bt_hog so we can can remove child > > instances and resolve the double free. > > > >> } > >> > >> static void foreach_hog_service(struct gatt_db_attribute *attr, void *user_data) > >> { > >> struct bt_hog *hog = user_data; > >> > >> - hog_attach_instace(hog, attr); > >> + hog_attach_instance(hog, attr); > >> } > >> > >> static void dis_notify(uint8_t source, uint16_t vendor, uint16_t product, > >> @@ -1528,7 +1528,7 @@ static void hog_attach_hog(struct bt_hog *hog, struct gatt_primary *primary) > >> primary->range.end, find_included_cb, instance); > >> > >> bt_hog_attach(instance, hog->attrib); > >> - hog->instances = g_slist_append(hog->instances, instance); > >> + hog->instances = g_slist_append(hog->instances, bt_hog_ref(instance)); > >> } > >> > >> static void primary_cb(uint8_t status, GSList *services, void *user_data) > >> -- > >> 2.17.1 > >> > > > >
diff --git a/profiles/input/hog-lib.c b/profiles/input/hog-lib.c index 9c5c814a7..b9b5d565c 100644 --- a/profiles/input/hog-lib.c +++ b/profiles/input/hog-lib.c @@ -1357,7 +1357,7 @@ static struct bt_hog *hog_new(int fd, const char *name, uint16_t vendor, return hog; } -static void hog_attach_instace(struct bt_hog *hog, +static void hog_attach_instance(struct bt_hog *hog, struct gatt_db_attribute *attr) { struct bt_hog *instance; @@ -1373,14 +1373,14 @@ static void hog_attach_instace(struct bt_hog *hog, if (!instance) return; - hog->instances = g_slist_append(hog->instances, instance); + hog->instances = g_slist_append(hog->instances, bt_hog_ref(instance)); } static void foreach_hog_service(struct gatt_db_attribute *attr, void *user_data) { struct bt_hog *hog = user_data; - hog_attach_instace(hog, attr); + hog_attach_instance(hog, attr); } static void dis_notify(uint8_t source, uint16_t vendor, uint16_t product, @@ -1528,7 +1528,7 @@ static void hog_attach_hog(struct bt_hog *hog, struct gatt_primary *primary) primary->range.end, find_included_cb, instance); bt_hog_attach(instance, hog->attrib); - hog->instances = g_slist_append(hog->instances, instance); + hog->instances = g_slist_append(hog->instances, bt_hog_ref(instance)); } static void primary_cb(uint8_t status, GSList *services, void *user_data)