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@xxxxxxxxxxxxx> 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
