Hi Luiz,
On Thu, Jan 21, 2021 at 4:37 PM Luiz Augusto von Dentz
<luiz.dentz@xxxxxxxxx> wrote:
>
> Hi Sonny,
>
> On Thu, Jan 21, 2021 at 4:18 PM Sonny Sasaka <sonnysasaka@xxxxxxxxxxxx> wrote:
> >
> > The report map in the cache could be dirty, for example when reading a
> > report map from peer was cancelled, we should be able to detect it and
> > not try to create UHID. Instead we will read it again from the peer.
>
> Don't we clean the cache if it had failed? Or you meant to say the
> read long procedure was not complete so we got just part of the report
> map?
Looks like this is the case. It happened to me once when I cancel
reconnection (trigger pairing mode during reconnection) from the
keyboard side. It's hard to confirm since I have to get the timing
right.
> In that case we should have failed
I agree. However it seems that the code already tries to fail by
looking at the status inside report_map_read_cb, but somehow it still
gets through. It could be the keyboard bug that we have to detect
anyway?
> also if we need to protect
> uhid from malformed report map, which sounds like a kernel bug, then
> we should at least have it inside bt_uhid instance so we can at least
> attempt to have some unit testing done with broken report maps.
>
> > ---
> > profiles/input/hog-lib.c | 21 ++++++++++++++++++---
> > 1 file changed, 18 insertions(+), 3 deletions(-)
> >
> > diff --git a/profiles/input/hog-lib.c b/profiles/input/hog-lib.c
> > index 089f42826..d6a3bda4d 100644
> > --- a/profiles/input/hog-lib.c
> > +++ b/profiles/input/hog-lib.c
> > @@ -946,7 +946,7 @@ static void uhid_create(struct bt_hog *hog, uint8_t *report_map,
> > struct uhid_event ev;
> > ssize_t vlen = report_map_len;
> > char itemstr[20]; /* 5x3 (data) + 4 (continuation) + 1 (null) */
> > - int i, err;
> > + int i, err, collection_depth = 0;
> > GError *gerr = NULL;
> >
> > DBG("Report MAP:");
> > @@ -960,6 +960,14 @@ static void uhid_create(struct bt_hog *hog, uint8_t *report_map,
> > if (!long_item && (value[i] & 0xfc) == 0x84)
> > hog->has_report_id = TRUE;
> >
> > + // Start Collection
> > + if (value[i] == 0xa1)
> > + collection_depth++;
> > +
> > + // End Collection
> > + if (value[i] == 0xc0)
> > + collection_depth--;
> > +
> > DBG("\t%s", item2string(itemstr, &value[i], ilen));
> >
> > i += ilen;
> > @@ -968,10 +976,15 @@ static void uhid_create(struct bt_hog *hog, uint8_t *report_map,
> >
> > /* Just print remaining items at once and break */
> > DBG("\t%s", item2string(itemstr, &value[i], vlen - i));
> > - break;
> > + return;
> > }
> > }
> >
> > + if (collection_depth != 0) {
> > + error("Report Map error: unbalanced collection");
> > + return;
> > + }
> > +
> > /* create uHID device */
> > memset(&ev, 0, sizeof(ev));
> > ev.type = UHID_CREATE;
> > @@ -1365,7 +1378,9 @@ static void foreach_hog_chrc(struct gatt_db_attribute *attr, void *user_data)
> > * UHID to optimize reconnection.
> > */
> > uhid_create(hog, report_map.value, report_map.length);
> > - } else {
> > + }
> > +
> > + if (!hog->uhid_created) {
> > read_char(hog, hog->attrib, value_handle,
> > report_map_read_cb, hog);
> > }
> > --
> > 2.29.2
> >
>
>
> --
> Luiz Augusto von Dentz
