Re: errors in usage.c - libusbhid
On Tue, 3 Jul 2018 07:55:47 +0200
Martin Pieuchot wrote:
> >
> > If wanted I can send the code I used to test this.
>
> It would be great if you could turn that into a regression test to put
> in /usr/src/regress/lib/libusbhid/nameofyourtest :)
Sure.
Should say that my Bourne shell & bsd.regress.mk knowledge/experience
is limited. Anyone feuer frei if any errors or unnecessary lines in the
files are found.
The regression test.
Added a group called usage and has three test cases.
I call them; hex, dec & static.
hex is disabled, as this(%x) is unhandled at the moment but included
for the future.
dec tests usages in page that has %u or %d formats.
static tests lines that has statically defined names.
Index: libusbhid/Makefile
===
RCS file: libusbhid/Makefile
diff -N libusbhid/Makefile
--- /dev/null 1 Jan 1970 00:00:00 -
+++ libusbhid/Makefile 4 Jul 2018 14:10:18 -
@@ -0,0 +1,7 @@
+# $OpenBSD$
+
+SUBDIR+= usage
+
+install:
+
+.include
Index: libusbhid/usage/Makefile
===
RCS file: libusbhid/usage/Makefile
diff -N libusbhid/usage/Makefile
--- /dev/null 1 Jan 1970 00:00:00 -
+++ libusbhid/usage/Makefile4 Jul 2018 14:32:49 -
@@ -0,0 +1,25 @@
+# $OpenBSD$
+
+LDADD= -lusbhid
+DPADD= ${LIBUSBHID}
+WARNINGS= Yes
+CFLAGS+= -Werror
+
+
+PROG= parsetest
+REGRESS_TARGETS+= run-regress-${PROG}-hex
+REGRESS_TARGETS+= run-regress-${PROG}-dec
+REGRESS_TARGETS+= run-regress-${PROG}-static
+
+.include
+
+# hextest is commented in runparse.sh
+run-regress-${PROG}-hex: ${PROG}
+ ./runparse.sh hex "12 100012"
+
+run-regress-${PROG}-dec: ${PROG}
+ ./runparse.sh dec "90002 A0002 810002"
+
+run-regress-${PROG}-static: ${PROG}
+ ./runparse.sh static "B0002 D0002 820012"
+
Index: libusbhid/usage/parsetest.c
===
RCS file: libusbhid/usage/parsetest.c
diff -N libusbhid/usage/parsetest.c
--- /dev/null 1 Jan 1970 00:00:00 -
+++ libusbhid/usage/parsetest.c 4 Jul 2018 14:33:06 -
@@ -0,0 +1,50 @@
+/* $OpenBSD$ */
+/*
+ * Copyright (c) 2018 David Bern
+ *
+ * Permission to use, copy, modify, and distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
+
+#include
+#include
+
+#include
+#include
+#include
+#include
+
+int
+main(int argc, char *argv[])
+{
+ char *table = NULL;
+ char usage[1024];
+ int testval;
+ const char *errstr;
+
+ if (hid_start(table) == -1)
+ errx(1, "\nUnable to load table");
+
+ /* No args given, just test if able to load table */
+ if (argc < 2)
+ return 0;
+
+ testval = strtonum(argv[1], 0x0, 0x, );
+ if (errstr != NULL)
+ errx(1, "\nInvalid argument");
+
+ snprintf(usage, sizeof(usage), "%s:%s",
+ hid_usage_page(HID_PAGE(testval)),
+ hid_usage_in_page(testval));
+
+ return (hid_parse_usage_in_page(usage) != testval);
+}
Index: libusbhid/usage/runparse.sh
===
RCS file: libusbhid/usage/runparse.sh
diff -N libusbhid/usage/runparse.sh
--- /dev/null 1 Jan 1970 00:00:00 -
+++ libusbhid/usage/runparse.sh 4 Jul 2018 14:33:19 -
@@ -0,0 +1,32 @@
+#!/bin/sh
+# $OpenBSD$
+
+run() {
+for i in $* ; do
+ ./parsetest $(echo "ibase=16; $i" | bc)
+ if [ $? -ne 0 ]
+ then
+ printf "\nFailed on 0x%s\n" "$i"
+ return 1
+ fi
+ printf " $i"
+done
+printf "\n"
+return 0
+}
+
+case $1 in
+hex)
+ printf "Unable to handle %%x format names - DISABLED\n"
+ # run $2
+ exit 0 ;;
+dec)
+ printf "Testing %%d & %%u format names"
+ run $2
+ exit $? ;;
+static)
+ printf "Testing staticly named usage names"
+ run $2
+ exit $? ;;
+esac
+
Re: errors in usage.c - libusbhid
On 02/07/18(Mon) 23:08, David Bern wrote:
> > Is it necessary to parse these examples? Or maybe we can live with
> > your strtonum() fix for now.
> I can live with that.
>
> > I don't know. So unless somebody else gives us some input I'd suggest
> > we move forward with your safe diff even if it doesn't fix all the
> > cases.
> >
> > If it fixes your use case it is already an improvement.
>
> I have attached a strtonum() fix.
> To give a short summary on how it works.
> In us == -1 cases, I locate the '%'-symbol stored page_contents[j].name
> to calculate length. The length is then used to compare the function
> input "name" with page_contents[j].name.
>
> If name match, I then use "len" to position "name" to the
> strtonum-function and use the return value to calculate the usage
> number.
>
> For people who want to test this patch;
> hid_start()
> snprintf(usage, sizeof(usage), "%s:%s",
>hid_usage_page(HID_PAGE(test_val)),
>hid_usage_in_page(test_val));
> test_val == hid_parse_usage_in_page(usage)
>
> As mentioned earlier the usage = (16 << 16) | 0xA (16 Unicode) and
> higher will fail, but 0 to 9 succeeds.
>
> If wanted I can send the code I used to test this.
It would be great if you could turn that into a regression test to put
in /usr/src/regress/lib/libusbhid/nameofyourtest :)
> 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 - 1.16
> +++ usage.c 2 Jul 2018 19:51:13 -
> @@ -264,9 +264,9 @@ hid_parse_usage_page(const char *name)
> int
> hid_parse_usage_in_page(const char *name)
> {
> - const char *sep;
> + const char *sep, *fmtsep, *errstr, *fmtname;
> unsigned int l;
> - int k, j;
> + int k, j, us, pu, len;
>
> sep = strchr(name, ':');
> if (sep == NULL)
> @@ -278,9 +278,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) {
> + fmtname = pages[k].page_contents[j].name;
> + fmtsep = strchr(fmtname, '%');
> + len = fmtsep - fmtname;
> + if (fmtsep != NULL && strncmp(sep, fmtname, len) == 0) {
> + pu = strtonum(sep + len, 0x1, 0x, );
> + if (errstr == NULL)
> + 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;
> }
Re: errors in usage.c - libusbhid
> Is it necessary to parse these examples? Or maybe we can live with
> your strtonum() fix for now.
I can live with that.
> I don't know. So unless somebody else gives us some input I'd suggest
> we move forward with your safe diff even if it doesn't fix all the
> cases.
>
> If it fixes your use case it is already an improvement.
I have attached a strtonum() fix.
To give a short summary on how it works.
In us == -1 cases, I locate the '%'-symbol stored page_contents[j].name
to calculate length. The length is then used to compare the function
input "name" with page_contents[j].name.
If name match, I then use "len" to position "name" to the
strtonum-function and use the return value to calculate the usage
number.
For people who want to test this patch;
hid_start()
snprintf(usage, sizeof(usage), "%s:%s",
hid_usage_page(HID_PAGE(test_val)),
hid_usage_in_page(test_val));
test_val == hid_parse_usage_in_page(usage)
As mentioned earlier the usage = (16 << 16) | 0xA (16 Unicode) and
higher will fail, but 0 to 9 succeeds.
If wanted I can send the code I used to test this.
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 - 1.16
+++ usage.c 2 Jul 2018 19:51:13 -
@@ -264,9 +264,9 @@ hid_parse_usage_page(const char *name)
int
hid_parse_usage_in_page(const char *name)
{
- const char *sep;
+ const char *sep, *fmtsep, *errstr, *fmtname;
unsigned int l;
- int k, j;
+ int k, j, us, pu, len;
sep = strchr(name, ':');
if (sep == NULL)
@@ -278,9 +278,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) {
+ fmtname = pages[k].page_contents[j].name;
+ fmtsep = strchr(fmtname, '%');
+ len = fmtsep - fmtname;
+ if (fmtsep != NULL && strncmp(sep, fmtname, len) == 0) {
+ pu = strtonum(sep + len, 0x1, 0x, );
+ if (errstr == NULL)
+ 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;
}
Re: errors in usage.c - libusbhid
On 30/06/18(Sat) 23:47, David Bern wrote: > > Note that your diff doesn't apply. You have tab vs spaces issues. > Sorry about that, it seems like I need to setup a proper mail-client. > > > I don't understand what you're talking about. Can you give example of > > the 3 scenarios you're talking about. > If you look inside /usr/share/misc/usb_hid_usages you will find it in > 9 Button, 10 Ordinal, 16 Unicode and 129 Monitor Enumerated Values. > > > There's also a similar fix in NetBSD's tree, did you look at it? > I took a look now and it enlightened a potential problem. > > NetBSD is using fmtcheck to make sure a "correct" format string is > used. I could perhaps make an effort on a patch adding that function into > stdio in OpenBSD, as that solves uses of %u compatible format strings > https://man.openbsd.org/NetBSD-7.1/fmtcheck.3 Better fix the current problem without adding a new function. > I do now also see what your concerns about scanf might of been. > My proposed patch open up for a two step "attack". > If a malicious user is able to alter the contents of usb_hid_usages > and change the name to add something like %s and if a program then > lets a user specify a name to be used by hid_parse_usage_in_page > it could then perform a buffer overflow. > > My first solution of using a "static" format-string would not been affected > by this potential attack or the version using strtonum(). but at the same > time > it would not be able to parse the 16 Unicode example. Is it necessary to parse these examples? Or maybe we can live with your strtonum() fix for now. > One middle way solution could be to write a "scanf" that only handles %u or > %x > format strings, in other words a simplified variant of fmtcheck. > > > Would really love to get some advice before continuing working on this > patch. I don't know. So unless somebody else gives us some input I'd suggest we move forward with your safe diff even if it doesn't fix all the cases. If it fixes your use case it is already an improvement.
Re: errors in usage.c - libusbhid
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 - 1.16
+++ usage.c 18 Jun 2018 18:38:04 -
@@ -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,
) > 0
+ & 0x1 > 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 :
> 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 - 1.16
> +++ usage.c 14 Jun 2018 21:56:30 -
> @@ -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, 0x,
> );
> + 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) |
>
Re: errors in usage.c - libusbhid
I have given the following part some more thought
> + us = pages[k].page_contents[j].usage;
> + if (us == -1) {
> + usage_sep = strchr(sep, '_');
> + if (usage_sep == NULL)
> + return -1;
As I have come to understand how it works today, "us == -1" only occurs
when they are alone.
I guess its possible to mix between format-defined
and statically named usages with usage numbers? If yes to that question
it would be wise of me to put rest of parsing in a "if (usage_sep != NULL)"
instead or what do you think mpi@?
2018-06-15 0:11 GMT+02:00 David Bern :
> 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 - 1.16
> +++ usage.c 14 Jun 2018 21:56:30 -
> @@ -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, 0x,
> );
> + 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 :
>
>> 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 - 1.16
>> > +++ usage.c 29 May 2018 19:45:25 -
>> > @@ -265,8 +265,9 @@ int
>> > hid_parse_usage_in_page(const char *name)
>> > {
>> > const char *sep;
>> > + const char *usage_sep;
>> > unsigned int
Re: errors in usage.c - libusbhid
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 - 1.16
+++ usage.c 14 Jun 2018 21:56:30 -
@@ -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, 0x,
);
+ 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 :
> 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/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
>
> 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 - 1.16
> > +++ usage.c 29 May 2018 19:45:25 -
> > @@ -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", _usage))
> > + return (pages[k].usage << 16) |
> > + parsed_usage;
> > + }
> > if
Re: errors in usage.c - libusbhid
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/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
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 - 1.16
> +++ usage.c 29 May 2018 19:45:25 -
> @@ -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", _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 :
>
> > 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", _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 :
> >
> >> While I was waiting for comments and feedback I came up with some
> >> improvements.
> >> The "logic" is
Re: errors in usage.c - libusbhid
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 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 - 1.16
> +++ usage.c 29 May 2018 19:45:25 -
> @@ -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", _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 :
>
>> 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", _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 :
>>
>>> 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:
Re: errors in usage.c - libusbhid
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 - 1.16
+++ usage.c 29 May 2018 19:45:25 -
@@ -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", _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 :
> 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", _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 :
>
>> 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 - 1.16
>> +++ usage.c 24 May 2018 16:37:54 -
>> @@ -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
Re: errors in usage.c - libusbhid
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", _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 :
> 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 - 1.16
> +++ usage.c 24 May 2018 16:37:54 -
> @@ -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", _usage))
> + return (pages[k].usage << 16) |
> +
Re: errors in usage.c - libusbhid
On Mon, May 21, 2018 at 11:12:37PM +0200, David Bern wrote:
> First diff "solves" point 1 & 2. Second diff is on the parts of the manual
> that does not match the usbhid.h
>
i've committed the man part - i have to leave the rest for someone else
to hopefully pick up.
jmc
> 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 - 1.16
> +++ usage.c 21 May 2018 21:06:24 -
> @@ -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", _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.313 May 2014 14:05:02 - 1.17
> +++ usbhid.321 May 2018 21:02:21 -
> @@ -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 :
>
> > 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
Re: errors in usage.c - libusbhid
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 - 1.16
+++ usage.c 24 May 2018 16:37:54 -
@@ -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", _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 :
> 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 - 1.16
> +++ usage.c 21 May 2018 21:06:24 -
> @@ -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;
> +
Re: errors in usage.c - libusbhid
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 - 1.16
+++ usage.c 21 May 2018 21:06:24 -
@@ -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", _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.313 May 2014 14:05:02 - 1.17
+++ usbhid.321 May 2018 21:02:21 -
@@ -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 :
> 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
Re: errors in usage.c - libusbhid
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)
