Bump. This patch fixes "runtime" error in libusbhid when parsing usage strings with usage defined in default usbhid table as a format string.
On Tue, May 29, 2018, 10:29 PM David Bern <david.ml.b...@gmail.com> wrote: > Sorry for the spamming. > After some research and finding that my fix for issue nr: 2 ( > hid_usage_in_page() ) > will break the functionality inside /usr.bin/usbhidaction/usbhidaction.c > https://goo.gl/1cWFtR (link to usbhidaction.c) > > I now change my patch to only include a fix for issue nr: 1 > More details is described in the previous mail > > Index: usage.c > =================================================================== > RCS file: /cvs/src/lib/libusbhid/usage.c,v > retrieving revision 1.16 > diff -u -p -r1.16 usage.c > --- usage.c 8 Oct 2014 04:49:36 -0000 1.16 > +++ usage.c 29 May 2018 19:45:25 -0000 > @@ -265,8 +265,9 @@ int > hid_parse_usage_in_page(const char *name) > { > const char *sep; > + const char *usage_sep; > unsigned int l; > - int k, j; > + int k, j, us, parsed_usage; > > sep = strchr(name, ':'); > if (sep == NULL) > @@ -278,9 +279,19 @@ hid_parse_usage_in_page(const char *name > return -1; > found: > sep++; > - for (j = 0; j < pages[k].pagesize; j++) > + for (j = 0; j < pages[k].pagesize; j++) { > + us = pages[k].page_contents[j].usage; > + if (us == -1) { > + usage_sep = strchr(sep, '_'); > + if (usage_sep == NULL) > + return -1; > + if (sscanf(usage_sep, "_%d", &parsed_usage)) > + return (pages[k].usage << 16) | > + parsed_usage; > + } > if (strcmp(pages[k].page_contents[j].name, sep) == 0) > return (pages[k].usage << 16) | > pages[k].page_contents[j].usage; > + } > return -1; > } > > > comments? ok? > > 2018-05-28 13:01 GMT+02:00 David Bern <david.ml.b...@gmail.com>: > >> I was suggested off list to give an explanation on what the patch does. >> >> So please, tell me if I need to clarify more, or make further changes to >> the code. >> >> The patch tries to fix two things. >> 1. Changes in hid_parse_usage_in_page() fixes problems in parsing usages >> defined as: * Button %d >> >> hid_parse_usage_in_page(): >> Previously - With input "Button:Button_1" returns -1 >> Now - With input "Button:Button_1" returns 589825 >> >> In the scenario of parsing Button:Button_1 we will not find a usage name >> matching that string. For example Button:Button_1 is defined as >> Button %d in the table. >> >> We are still able to calculate the proper usage number in the same way we >> are >> able to calculate the proper usage name in hid_usage_in_page(). >> >> The first step is to identify if usage name is shortened. If it is, >> usage will hold a value of -1. Then I try to locate a separator char in >> the name as "_". >> If a separator char is found I use it to read the value as "_%d" to get >> the >> corresponding usage number >> >> >+ us = pages[k].page_contents[j].usage; >> >+ if (us == -1) { >> >+ usage_sep = strchr(sep, '_'); >> >+ if (usage_sep == NULL) >> >+ return -1; >> >+ if (sscanf(usage_sep, "_%d", &parsed_usage)) >> >+ return (pages[k].usage << 16) | >> >+ parsed_usage; >> >> >> 2. The text-string that is returned by hid_usage_in_page() misses page >> information. >> So the changes made in hid_usage_in_page() is to make it the inverse of >> hid_parse_usage_in_page() >> >> In details what the code previously did and now does. >> >> hid_usage_in_page(): >> Previously - With input 589825 returns Button_1 >> Now - With input 589825 returns Button:Button_1 >> >> >> The change just adds a pages[k].name to the string, a format that >> hid_parse_usage_in_page() expects it to have. >> I make formatting in two steps when us == -1 which it is when usage is >> shortened >> as for example: * Button %d. >> >> >+ snprintf(fmt, sizeof fmt, >> >+ "%%s:%s", pages[k].page_contents[j].name); >> >+ snprintf(b, sizeof b, fmt, pages[k].name, i); >> >> The first step is to create a format string that will result in something >> like >> "%s:Button_%d". >> The last step I use the fmt-string to create a complete string that will >> result in >> "Button:Button_1" >> >> >> >> >> 2018-05-24 18:44 GMT+02:00 David Bern <david.ml.b...@gmail.com>: >> >>> While I was waiting for comments and feedback I came up with some >>> improvements. >>> The "logic" is still the same, but the execution is hopefully more sane. >>> >>> Index: usage.c >>> =================================================================== >>> RCS file: /cvs/src/lib/libusbhid/usage.c,v >>> retrieving revision 1.16 >>> diff -u -p -r1.16 usage.c >>> --- usage.c 8 Oct 2014 04:49:36 -0000 1.16 >>> +++ usage.c 24 May 2018 16:37:54 -0000 >>> @@ -224,6 +224,7 @@ hid_usage_in_page(unsigned int u) >>> { >>> int i = HID_USAGE(u), j, k, us; >>> int page = HID_PAGE(u); >>> + char fmt[100]; >>> static char b[100]; >>> >>> for (k = 0; k < npages; k++) >>> @@ -234,12 +235,16 @@ hid_usage_in_page(unsigned int u) >>> for (j = 0; j < pages[k].pagesize; j++) { >>> us = pages[k].page_contents[j].usage; >>> if (us == -1) { >>> - snprintf(b, sizeof b, >>> - pages[k].page_contents[j].name, i); >>> + snprintf(fmt, sizeof fmt, >>> + "%%s:%s", pages[k].page_contents[j].name); >>> + snprintf(b, sizeof b, fmt, pages[k].name, i); >>> + return b; >>> + } >>> + if (us == i) { >>> + snprintf(b, sizeof b, "%s:%s", pages[k].name, >>> + pages[k].page_contents[j].name); >>> return b; >>> } >>> - if (us == i) >>> - return pages[k].page_contents[j].name; >>> } >>> bad: >>> snprintf(b, sizeof b, "0x%04x", i); >>> @@ -265,8 +270,9 @@ int >>> hid_parse_usage_in_page(const char *name) >>> { >>> const char *sep; >>> + const char *usage_sep; >>> unsigned int l; >>> - int k, j; >>> + int k, j, us, parsed_usage; >>> >>> sep = strchr(name, ':'); >>> if (sep == NULL) >>> @@ -278,9 +284,19 @@ hid_parse_usage_in_page(const char *name >>> return -1; >>> found: >>> sep++; >>> - for (j = 0; j < pages[k].pagesize; j++) >>> + for (j = 0; j < pages[k].pagesize; j++) { >>> + us = pages[k].page_contents[j].usage; >>> + if (us == -1) { >>> + usage_sep = strchr(sep, '_'); >>> + if (usage_sep == NULL) >>> + return -1; >>> + if (sscanf(usage_sep, "_%d", &parsed_usage)) >>> + return (pages[k].usage << 16) | >>> + parsed_usage; >>> + } >>> if (strcmp(pages[k].page_contents[j].name, sep) == 0) >>> return (pages[k].usage << 16) | >>> pages[k].page_contents[j].usage; >>> + } >>> return -1; >>> } >>> >>> >>> >>> 2018-05-21 23:12 GMT+02:00 David Bern <david.ml.b...@gmail.com>: >>> >>>> First diff "solves" point 1 & 2. Second diff is on the parts of the >>>> manual that does not match the usbhid.h >>>> >>>> Index: usage.c >>>> =================================================================== >>>> RCS file: /cvs/src/lib/libusbhid/usage.c,v >>>> retrieving revision 1.16 >>>> diff -u -p -r1.16 usage.c >>>> --- usage.c 8 Oct 2014 04:49:36 -0000 1.16 >>>> +++ usage.c 21 May 2018 21:06:24 -0000 >>>> @@ -224,6 +224,7 @@ hid_usage_in_page(unsigned int u) >>>> { >>>> int i = HID_USAGE(u), j, k, us; >>>> int page = HID_PAGE(u); >>>> + char usage_name[100]; >>>> static char b[100]; >>>> >>>> for (k = 0; k < npages; k++) >>>> @@ -234,12 +235,18 @@ hid_usage_in_page(unsigned int u) >>>> for (j = 0; j < pages[k].pagesize; j++) { >>>> us = pages[k].page_contents[j].usage; >>>> if (us == -1) { >>>> - snprintf(b, sizeof b, >>>> + snprintf(usage_name, sizeof usage_name, >>>> pages[k].page_contents[j].name, i); >>>> + snprintf(b, sizeof b, >>>> + "%s:%s", pages[k].name, usage_name); >>>> + return b; >>>> + } >>>> + if (us == i) { >>>> + snprintf(b, sizeof b, >>>> + "%s:%s", pages[k].name, >>>> + pages[k].page_contents[j].name); >>>> return b; >>>> } >>>> - if (us == i) >>>> - return pages[k].page_contents[j].name; >>>> } >>>> bad: >>>> snprintf(b, sizeof b, "0x%04x", i); >>>> @@ -265,8 +272,9 @@ int >>>> hid_parse_usage_in_page(const char *name) >>>> { >>>> const char *sep; >>>> + const char *usage_sep; >>>> unsigned int l; >>>> - int k, j; >>>> + int k, j, us, parsed_usage; >>>> >>>> sep = strchr(name, ':'); >>>> if (sep == NULL) >>>> @@ -277,10 +285,20 @@ hid_parse_usage_in_page(const char *name >>>> goto found; >>>> return -1; >>>> found: >>>> + usage_sep = strchr(sep, '_'); >>>> sep++; >>>> - for (j = 0; j < pages[k].pagesize; j++) >>>> + for (j = 0; j < pages[k].pagesize; j++) { >>>> + us = pages[k].page_contents[j].usage; >>>> + if (us == -1) { >>>> + if (usage_sep == NULL) >>>> + return -1; >>>> + if (sscanf(usage_sep, "_%d", &parsed_usage)) >>>> + return (pages[k].usage << 16 | >>>> + parsed_usage); >>>> + } >>>> if (strcmp(pages[k].page_contents[j].name, sep) == 0) >>>> return (pages[k].usage << 16) | >>>> pages[k].page_contents[j].usage; >>>> + } >>>> return -1; >>>> } >>>> >>>> Index: usbhid.3 >>>> =================================================================== >>>> RCS file: /cvs/src/lib/libusbhid/usbhid.3,v >>>> retrieving revision 1.17 >>>> diff -u -p -r1.17 usbhid.3 >>>> --- usbhid.3 13 May 2014 14:05:02 -0000 1.17 >>>> +++ usbhid.3 21 May 2018 21:02:21 -0000 >>>> @@ -65,22 +65,22 @@ >>>> .Fn hid_report_size "report_desc_t d" "hid_kind_t k" "int id" >>>> .Ft int >>>> .Fn hid_locate "report_desc_t d" "u_int usage" "hid_kind_t k" >>>> "hid_item_t *h" "int id" >>>> -.Ft char * >>>> +.Ft const char * >>>> .Fn hid_usage_page "int i" >>>> -.Ft char * >>>> +.Ft const char * >>>> .Fn hid_usage_in_page "u_int u" >>>> .Ft int >>>> .Fn hid_parse_usage_page "const char *" >>>> -.Ft char * >>>> +.Ft int >>>> .Fn hid_parse_usage_in_page "const char *" >>>> .Ft void >>>> .Fn hid_init "char *file" >>>> .Ft int >>>> .Fn hid_start "char *file" >>>> -.Ft int >>>> +.Ft int32_t >>>> .Fn hid_get_data "void *data" "hid_item_t *h" >>>> .Ft void >>>> -.Fn hid_set_data "void *data" "hid_item_t *h" "u_int data" >>>> +.Fn hid_set_data "void *data" "hid_item_t *h" "int32_t data" >>>> .Sh DESCRIPTION >>>> The >>>> .Nm >>>> >>>> >>>> >>>> 2018-05-21 12:07 GMT+02:00 Martin Pieuchot <m...@openbsd.org>: >>>> >>>>> On 18/05/18(Fri) 10:01, David Bern wrote: >>>>> > Hello! >>>>> > >>>>> > Have been using libusbhid and discovered a couple of discrepancies in >>>>> > the man-page (libusbhid.3) and the source of usage.c >>>>> > >>>>> > Some are just factual misses, but I also got (what I think is) 2 >>>>> errors. >>>>> > I will try to explain them; >>>>> > >>>>> > 1. (This is the I think is an error but not sure). The man-page >>>>> tells me >>>>> > that hid_usage_in_page and hid_parse_usage_in_page are each >>>>> > others inverse. >>>>> > If I haven't misunderstood the practical meaning of inverse in this >>>>> > case then this should be true: >>>>> > x == hid_parse_usage_in_page(hid_usage_in_page(x)). >>>>> > >>>>> > My observation: >>>>> > The main reason to why this isnt true, is that hid_usage_in_page() >>>>> > returns the data of pages[k].page_contents[j].name >>>>> > while hid_parse_usage_in_page() expects the data to >>>>> > contain "%s:%s", pages[k].name, pages[k].page_contents[j].name >>>>> > >>>>> > The reason I ask instead of just posting working code is this: >>>>> > Am I misunderstanding the manual? In that case, the solution I want >>>>> > to send in is a change in that sentence >>>>> > Or Is the manual correct and the behavior of hid_usage_in_page() >>>>> wrong, >>>>> > is this something I can correct without breaking other systems? >>>>> > >>>>> > >>>>> > 2. The second error I found is located in hid_parse_usage_in_page(). >>>>> > It is unable to parse values found in page Button. >>>>> > >>>>> > My observation: >>>>> > usage.c is using a standard table named usb_hid_pages. In that table >>>>> > we got a page called Buttons. >>>>> > the usages in that page is shortened to "* Button %d". >>>>> > I believe this is the cause of why hid_parse_usage_in_page() is >>>>> getting >>>>> > the pagesize "wrong" and therefor unable to parse any button in the >>>>> > button page. I guess this is the case with other similar cases as >>>>> well. >>>>> > >>>>> > my conclusion is that it would be possible to handle similar cases >>>>> in a >>>>> > similar way as it is handled in hid_usage_in_page(). >>>>> > >>>>> > >>>>> > As this is my first "issue" I would love to get as much feedback as >>>>> > possible so I can work on delivering a desirable and usable patch in >>>>> > my first try. >>>>> >>>>> Just send a diff, then we'll look at it 8) >>>>> >>>> >>>> >>> >> >
