Adding third party library hidapi

Adding third party library hidapi

BUG=290428

[Bei Zhang, 2013-10-02 23:42:49]

Hi Pawel,

Please review the change on licensecheck.pl.


We are introducing a triple licensed library that we would use it as BSD
licensed. Yet the licensecheck.pl seems having a hard time parsing the file. Is
there a reason that that line have to contain the "Use of this source code is
governed by" part?

Thanks,
Bei

[Paweł Hajdan Jr., 2013-10-03 01:14:25]

I think this is fine, I'll give final approval when it recognizes GPLv3.

Please make sure to follow
http://www.chromium.org/developers/adding-3rd-party-libraries and get a Chrome
Eng Review, open-source-third-party-reviews@google.com review and
security@chromium.org one.

If that raises any questions, I'll be happy to help.

https://codereview.chromium.org/25666010/diff/5001/third_party/devscripts/lic...
File third_party/devscripts/licensecheck.pl (right):

https://codereview.chromium.org/25666010/diff/5001/third_party/devscripts/lic...
third_party/devscripts/licensecheck.pl:471: } elsif ($licensetext =~ /is
distributed under the terms of the GNU General Public License,/
Let's also remove the "is distributed" part here, and in general try to make
that expression match the "under the terms of the GNU Public License v3" part of
the header.

https://codereview.chromium.org/25666010/diff/5001/third_party/hidapi/README....
File third_party/hidapi/README.chromium (right):

https://codereview.chromium.org/25666010/diff/5001/third_party/hidapi/README....
third_party/hidapi/README.chromium:3: Version: 0.7.0
You're aware this is 2 years old, right?

I've checked upstream and they seem to be making changes to the source code. How
about asking them to make a new release if possible?

[Bei Zhang, 2013-10-03 19:15:14]

https://codereview.chromium.org/25666010/diff/5001/third_party/devscripts/lic...
File third_party/devscripts/licensecheck.pl (right):

https://codereview.chromium.org/25666010/diff/5001/third_party/devscripts/lic...
third_party/devscripts/licensecheck.pl:471: } elsif ($licensetext =~ /is
distributed under the terms of the GNU General Public License,/
On 2013/10/03 01:14:25, Paweł Hajdan Jr. wrote:
> Let's also remove the "is distributed" part here, and in general try to make
> that expression match the "under the terms of the GNU Public License v3" part
of
> the header.

Done.

https://codereview.chromium.org/25666010/diff/5001/third_party/hidapi/README....
File third_party/hidapi/README.chromium (right):

https://codereview.chromium.org/25666010/diff/5001/third_party/hidapi/README....
third_party/hidapi/README.chromium:3: Version: 0.7.0
Due to the low activity of this project and simplicity of the functionality, we
will probably end up implementing our own back-end. So I'm not really there to
push them for it. They (if the company has more than one person, which I doubt)
might have abandoned the library as their official site doesn't even mention
0.7.0 and stays at 0.6.0.

On 2013/10/03 01:14:25, Paweł Hajdan Jr. wrote:
> You're aware this is 2 years old, right?
> 
> I've checked upstream and they seem to be making changes to the source code.
How
> about asking them to make a new release if possible?

[Paweł Hajdan Jr., 2013-10-03 19:40:36]

LGTM

[Bei Zhang, 2013-10-03 20:59:04]

Hi teams,

Please review.

Thanks,
Bei

[Bei Zhang, 2013-10-03 21:48:12]

Hi meacer,

Please review. I'm adding you to the OWNERS file.

Thanks,
Bei

[Bei Zhang, 2013-11-11 20:46:56]

Hi Carlos,

Please review.

Thanks,
Bei

[cpu_(ooo_6.6-7.5), 2013-11-13 01:08:50]

1- please add open-source-third-party-reviews@google.com and
security@chromium.org. to this review, you need their approval.

2- Please flesh out the bug, the rationale for this library, what it does, etc.

[cpu_(ooo_6.6-7.5), 2013-11-13 01:19:34]

the windows bits are pretty sketchy. I hope security review does a full pass
here.

[Daniel Berlin, 2013-12-03 22:49:52]

lgtm

[Kees Cook, 2013-12-04 20:49:36]

I didn't review Windows or Mac, but there's a lot of robustness changes needed
(some very minor security concerns) in at least the Linux side of things here.

https://codereview.chromium.org/25666010/diff/36001/third_party/hidapi/README...
File third_party/hidapi/README.chromium (right):

https://codereview.chromium.org/25666010/diff/36001/third_party/hidapi/README...
third_party/hidapi/README.chromium:3: Version: 0.7.0
This is 2 years old. Perhaps it'd be better to take the 0.8.0-rc1 snapshot, or
even newer?

https://codereview.chromium.org/25666010/diff/36001/third_party/hidapi/hidapi...
File third_party/hidapi/hidapi_linux.c (right):

https://codereview.chromium.org/25666010/diff/36001/third_party/hidapi/hidapi...
third_party/hidapi/hidapi_linux.c:85: hid_device *dev = calloc(1,
sizeof(hid_device));
Need to check for NULL return here to avoid crashing.

https://codereview.chromium.org/25666010/diff/36001/third_party/hidapi/hidapi...
third_party/hidapi/hidapi_linux.c:104: ret = calloc(wlen+1, sizeof(wchar_t));
NULL check needed.

https://codereview.chromium.org/25666010/diff/36001/third_party/hidapi/hidapi...
third_party/hidapi/hidapi_linux.c:127: int key = report_descriptor[i];
Why is "key" an int here? Would make more sense to be __u8.

https://codereview.chromium.org/25666010/diff/36001/third_party/hidapi/hidapi...
third_party/hidapi/hidapi_linux.c:209: value++;
Is a zero-length string in "value" considered valid?

https://codereview.chromium.org/25666010/diff/36001/third_party/hidapi/hidapi...
third_party/hidapi/hidapi_linux.c:246: char *product_name_utf8 = NULL;
space vs tab indenting?

https://codereview.chromium.org/25666010/diff/36001/third_party/hidapi/hidapi...
third_party/hidapi/hidapi_linux.c:276: &product_name_utf8);
"ret" is unchecked, yet things like product_name_utf8, etc get used below.

https://codereview.chromium.org/25666010/diff/36001/third_party/hidapi/hidapi...
third_party/hidapi/hidapi_linux.c:285: retm = mbstowcs(string,
product_name_utf8, maxlen);
If retm == maxlen, this string will not be NULL terminated. Same both for
DEVICE_STRING_SERIAL and USB cases below.

https://codereview.chromium.org/25666010/diff/36001/third_party/hidapi/hidapi...
third_party/hidapi/hidapi_linux.c:329: free(product_name_utf8);
space vs tab indenting.

https://codereview.chromium.org/25666010/diff/36001/third_party/hidapi/hidapi...
third_party/hidapi/hidapi_linux.c:439: tmp = malloc(sizeof(struct
hid_device_info));
Check for NULL return.

https://codereview.chromium.org/25666010/diff/36001/third_party/hidapi/hidapi...
third_party/hidapi/hidapi_linux.c:633: perror("HIDIOCGRDESCSIZE");
Reports failure but continues execution? desc_size may be in an unknown state
(but is likely still all 0s).

https://codereview.chromium.org/25666010/diff/36001/third_party/hidapi/hidapi...
third_party/hidapi/hidapi_linux.c:702: if (bytes_read >= 0 &&
This needs to be > 0, otherwise bytes_read-- will wrap around.

https://codereview.chromium.org/25666010/diff/36001/third_party/hidapi/hidapi...
File third_party/hidapi/hidapi_linux.c.patch (right):

https://codereview.chromium.org/25666010/diff/36001/third_party/hidapi/hidapi...
third_party/hidapi/hidapi_linux.c.patch:1: diff --git
a/third_party/hidapi/hidapi_linux.c b/third_party/hidapi/hidapi_linux.c
What's the purpose of this patch? Is this just silencing compiler warnings about
a useless compare against unsigned enum?

[Bei Zhang, 2013-12-10 22:24:10]

Hi Kees,

Thanks for the review!
Do you happened to have time to take a look at the other files?

Thanks,
Bei

https://codereview.chromium.org/25666010/diff/36001/third_party/hidapi/README...
File third_party/hidapi/README.chromium (right):

https://codereview.chromium.org/25666010/diff/36001/third_party/hidapi/README...
third_party/hidapi/README.chromium:3: Version: 0.7.0
On 2013/12/04 20:49:36, Kees Cook wrote:
> This is 2 years old. Perhaps it'd be better to take the 0.8.0-rc1 snapshot, or
> even newer?

Done.

https://codereview.chromium.org/25666010/diff/36001/third_party/hidapi/hidapi...
File third_party/hidapi/hidapi_linux.c (right):

https://codereview.chromium.org/25666010/diff/36001/third_party/hidapi/hidapi...
third_party/hidapi/hidapi_linux.c:85: hid_device *dev = calloc(1,
sizeof(hid_device));
On 2013/12/04 20:49:36, Kees Cook wrote:
> Need to check for NULL return here to avoid crashing.

Done.

https://codereview.chromium.org/25666010/diff/36001/third_party/hidapi/hidapi...
third_party/hidapi/hidapi_linux.c:104: ret = calloc(wlen+1, sizeof(wchar_t));
On 2013/12/04 20:49:36, Kees Cook wrote:
> NULL check needed.

Done.

https://codereview.chromium.org/25666010/diff/36001/third_party/hidapi/hidapi...
third_party/hidapi/hidapi_linux.c:127: int key = report_descriptor[i];
On 2013/12/04 20:49:36, Kees Cook wrote:
> Why is "key" an int here? Would make more sense to be __u8.

Done.

https://codereview.chromium.org/25666010/diff/36001/third_party/hidapi/hidapi...
third_party/hidapi/hidapi_linux.c:209: value++;
I don't think it will cause any problem here.

On 2013/12/04 20:49:36, Kees Cook wrote:
> Is a zero-length string in "value" considered valid?

https://codereview.chromium.org/25666010/diff/36001/third_party/hidapi/hidapi...
third_party/hidapi/hidapi_linux.c:246: char *product_name_utf8 = NULL;
On 2013/12/04 20:49:36, Kees Cook wrote:
> space vs tab indenting?

Done. Converted all tabs to spaces.

https://codereview.chromium.org/25666010/diff/36001/third_party/hidapi/hidapi...
third_party/hidapi/hidapi_linux.c:276: &product_name_utf8);
On 2013/12/04 20:49:36, Kees Cook wrote:
> "ret" is unchecked, yet things like product_name_utf8, etc get used below.

Done.

https://codereview.chromium.org/25666010/diff/36001/third_party/hidapi/hidapi...
third_party/hidapi/hidapi_linux.c:285: retm = mbstowcs(string,
product_name_utf8, maxlen);
On 2013/12/04 20:49:36, Kees Cook wrote:
> If retm == maxlen, this string will not be NULL terminated. Same both for
> DEVICE_STRING_SERIAL and USB cases below.

Done.

https://codereview.chromium.org/25666010/diff/36001/third_party/hidapi/hidapi...
third_party/hidapi/hidapi_linux.c:329: free(product_name_utf8);
Ditto.

On 2013/12/04 20:49:36, Kees Cook wrote:
> space vs tab indenting.

https://codereview.chromium.org/25666010/diff/36001/third_party/hidapi/hidapi...
third_party/hidapi/hidapi_linux.c:439: tmp = malloc(sizeof(struct
hid_device_info));
On 2013/12/04 20:49:36, Kees Cook wrote:
> Check for NULL return.

Done.

https://codereview.chromium.org/25666010/diff/36001/third_party/hidapi/hidapi...
third_party/hidapi/hidapi_linux.c:633: perror("HIDIOCGRDESCSIZE");
Done. Hidapi supports devices that does not contain a report descriptor.
Therefore the dev is still returned.

On 2013/12/04 20:49:36, Kees Cook wrote:
> Reports failure but continues execution? desc_size may be in an unknown state
> (but is likely still all 0s).

https://codereview.chromium.org/25666010/diff/36001/third_party/hidapi/hidapi...
third_party/hidapi/hidapi_linux.c:702: if (bytes_read >= 0 &&
On 2013/12/04 20:49:36, Kees Cook wrote:
> This needs to be > 0, otherwise bytes_read-- will wrap around.

Done.

https://codereview.chromium.org/25666010/diff/36001/third_party/hidapi/hidapi...
File third_party/hidapi/hidapi_linux.c.patch (right):

https://codereview.chromium.org/25666010/diff/36001/third_party/hidapi/hidapi...
third_party/hidapi/hidapi_linux.c.patch:1: diff --git
a/third_party/hidapi/hidapi_linux.c b/third_party/hidapi/hidapi_linux.c
On 2013/12/04 20:49:36, Kees Cook wrote:
> What's the purpose of this patch? Is this just silencing compiler warnings
about
> a useless compare against unsigned enum?

Yes, it is.

[Robert Sesek, 2014-01-14 23:24:58]

I was asked by wfh@ to review the Mac file and it's not lgtm on several points:

- //base has memory scopers (including for IOKit specifically), string
conversion utilities, and message loop/pump wrappers that obviate several
functions and would result in cleaner and safer code.
- The threading looks dubious, and I really don't think this should spawn
dedicated threads for its purposes. Chrome has too many already.
- There are large blocks of #if0'd and commented out code.
- The actual IOKit logic is fairly trivial.

Considering that cpu@ already said the Windows bits are sketchy and keescook@
identified serious issues with Linux, is there a good reason why we're using
this library?

[Will Harris, 2014-01-14 23:40:00]

Comments.  The Win implementation doesn't seem quite as bad as the Mac version,
but I do worry that we're introducing an unnecessary third_party library when it
seems the actual logic to interface with the Win HID API is pretty small and
could be done in Chromium land using Chrome constructs.

https://codereview.chromium.org/25666010/diff/236001/third_party/hidapi/hidap...
File third_party/hidapi/hidapi.gyp (right):

https://codereview.chromium.org/25666010/diff/236001/third_party/hidapi/hidap...
third_party/hidapi/hidapi.gyp:20: 'msvs_disabled_warnings': [ 4267 ],
is it not possible to fix these truncations?

https://codereview.chromium.org/25666010/diff/236001/third_party/hidapi/hidap...
File third_party/hidapi/hidapi_win.c (right):

https://codereview.chromium.org/25666010/diff/236001/third_party/hidapi/hidap...
third_party/hidapi/hidapi_win.c:67: #pragma warning(disable:4996)
prefer to disable in the gyp file rather than inline

https://codereview.chromium.org/25666010/diff/236001/third_party/hidapi/hidap...
third_party/hidapi/hidapi_win.c:196: lib_handle = LoadLibraryA("hid.dll");
where will this code be called from?  Prefer not to have random LoadLibary
happening.

Can you get the HIDAPI_USE_DDK define to work to avoid this?

https://codereview.chromium.org/25666010/diff/236001/third_party/hidapi/hidap...
third_party/hidapi/hidapi_win.c:318: device_interface_detail_data =
(SP_DEVICE_INTERFACE_DETAIL_DATA_A*) malloc(required_size);
check return value of malloc

https://codereview.chromium.org/25666010/diff/236001/third_party/hidapi/hidap...
third_party/hidapi/hidapi_win.c:340: char driver_name[256];
where is 256 from?

https://codereview.chromium.org/25666010/diff/236001/third_party/hidapi/hidap...
third_party/hidapi/hidapi_win.c:371: goto cont_close;
CloseHandle() on invalid handle later on

https://codereview.chromium.org/25666010/diff/236001/third_party/hidapi/hidap...
third_party/hidapi/hidapi_win.c:392: wchar_t wstr[WSTR_LEN]; /* TODO: Determine
Size */
seems like a rather scary TODO

https://codereview.chromium.org/25666010/diff/236001/third_party/hidapi/hidap...
third_party/hidapi/hidapi_win.c:430: res =
HidD_GetSerialNumberString(write_handle, wstr, sizeof(wstr));
these functions all take number of bytes but sizeof() is giving number of wchars

https://codereview.chromium.org/25666010/diff/236001/third_party/hidapi/hidap...
third_party/hidapi/hidapi_win.c:585: dev->read_buf = (char*)
malloc(dev->input_report_length);
check return value

https://codereview.chromium.org/25666010/diff/236001/third_party/hidapi/hidap...
third_party/hidapi/hidapi_win.c:723: int HID_API_EXPORT HID_API_CALL
hid_read(hid_device *dev, unsigned char *data, size_t length)
your calls from Chromeland are using the blocking version, have you considered
using the non-blocking version instead?

https://codereview.chromium.org/25666010/diff/236001/third_party/hidapi/hidap...
third_party/hidapi/hidapi_win.c:876: int __cdecl main(int argc, char* argv[])
can this test code be removed and/or replaced with proper unit tests?