I take back my previous answer to this question:
> Are you sure the name always contain an underscore? Can't it be
> "Button:Button42" ?
In my previous response I had only discovered three cases of usage -1.
I took another look and discovered that there indeed is a scenario as you
mention other then just "_%d".
In my efforts to resolve that scenario, I discovered a simple solution I
had not thought about earlier.
With the help of pages[k].page_contents[j].name I'm able to parse current
and
future versions " * %99[^\n]" lines the usb_hid_usages.
This patch is back to using sscanf again & I still haven't been able to
figure
out what could be unsafe about this use of it.
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 18 Jun 2018 18:38:04 -0000
@@ -266,7 +266,7 @@ hid_parse_usage_in_page(const char *name
{
const char *sep;
unsigned int l;
- int k, j;
+ int k, j, us, pu;
sep = strchr(name, ':');
if (sep == NULL)
@@ -278,9 +278,16 @@ 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) {
+ if (sscanf(sep, pages[k].page_contents[j].name,
&pu) > 0
+ & 0x10000 > pu & pu > 0x0)
+ return (pages[k].usage << 16) | pu;
+ }
if (strcmp(pages[k].page_contents[j].name, sep) == 0)
return (pages[k].usage << 16) |
pages[k].page_contents[j].usage;
+ }
return -1;
}
2018-06-15 0:11 GMT+02:00 David Bern <david.ml.b...@gmail.com>:
> Thank you mpi for the response.
>
> I will try to answer to the best of my knowledge.
>
> > Are you sure the name always contain an underscore? Can't it be
> > "Button:Button42" ?
> It depends. At the moment it is dictated by the default page
> /usr/share/misc/usb_hid_usages and the name looks more like "Button:Button
> 42".
>
> So if someone decides to create another table, he/she could decide to name
> it
> "Button:Button-42" instead.
> However if that would occur other things would break as well, this
> because of how the
> table is loaded by hid_start().
>
> The patch is made to handle lines defined as " * %99[^\n]". When a line
> with spaces
> is found, hid_start() will replace the spaces with "_".
>
> > You're passing the string directly to sscanf(3), is that safe?
>
> I was initially uncertain about the safety of sscanf, But when reading the
> man-page I could
> only identify scenarios using it to scan "%s" as unsafe if you don't
> supply a width as this
> could lead to buffer overflows.
>
> > Don't you want to parse only unsigned numbers? Or is it possible to
> > have "Button:Button_-3" ?
>
> Yes. You are right, which has lead me to replace sscanf with strtonum
> instead.
> In that way I can do a simple check of input correctness.
> To be honest regarding input, I'm not certain about how high values the
> hid-specification
> allows, it does seem in the cases I have been able to identify, that it is
> possible to use
> the first two bytes.
>
> Thank you for the questions.
>
>
> 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 14 Jun 2018 21:56:30 -0000
> @@ -265,8 +265,10 @@ 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;
> + const char *errstr;
>
> sep = strchr(name, ':');
> if (sep == NULL)
> @@ -278,9 +280,21 @@ 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;
> + usage_sep++;
> + parsed_usage = strtonum(usage_sep, 0x1, 0xFFFF,
> &errstr);
> + if (errstr == NULL)
> + 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-06-14 14:54 GMT+02:00 Martin Pieuchot <m...@openbsd.org>:
>
>> On 29/05/18(Tue) 22:29, David Bern 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/usbhidac
>> tion.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
>>
>> Are you sure the name always contain an underscore? Can't it be
>> "Button:Button42" ?
>>
>> You're passing the string directly to sscanf(3), is that safe?
>>
>> Don't you want to parse only unsigned numbers? Or is it possible to
>> have "Button:Button_-3" ?
>>
>> > 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)
>> > >>>>
>> > >>>
>> > >>>
>> > >>
>> > >
>>
>
>
