Fix type truncation warnings in HID code.

Fix type truncation warnings in HID code.

GetShortData() and IOHIDElementGetUsage() both return uint32s, so raise the size
of various containing types to uint32.

I tried to look in the USB HID spec to see if any of the affected fields here
were limited to e.g. <2^16, meaning that it'd be safe to just cast down to the
existing uint16s instead, but I couldn't find any limits.

BUG=81439
TEST=none

Committed: https://crrev.com/4c3b672cc2c1515b7e16cb535e1cd6e69e326607
Cr-Commit-Position: refs/heads/master@{#300201}

[Peter Kasting, 2014-10-13 22:07:01]

pkasting@chromium.org changed reviewers:
+ reillyg@chromium.org

[Peter Kasting, 2014-10-13 22:07:02]

Feel free to push back if this is actually wrong.  Even after attempting to peer
through the spec I feel like I don't know what I'm doing.

[Reilly Grant (use Gerrit), 2014-10-14 00:19:21]

HID usage and usage page IDs are definitely 16-bits. Instead of changing these
we should check and reject the device if we get something too large. For the
report sizes an upper bound is less defined. An over 64kB report would be huge
and stretch the limitations of USB's HID transport. The integers stored in the
HID report descriptor are variable size so they could be any size but we should
pick sane limits as these values are passed to malloc.

Peter, I've gotten conflicting answers for whether or not unsigned integers
should be used for computing buffer sizes like this. What is your opinion?

[Peter Kasting, 2014-10-14 00:27:11]

On 2014/10/14 00:19:21, reillyg wrote:
> HID usage and usage page IDs are definitely 16-bits. Instead of changing these
> we should check and reject the device if we get something too large.

base::checked_cast<> can be used to narrow the type if you want failure to crash
the program.  If you want something else to happen, you'll have to either write
the patch or tell me what the right thing to do is :)

> For the
> report sizes an upper bound is less defined. An over 64kB report would be huge
> and stretch the limitations of USB's HID transport. The integers stored in the
> HID report descriptor are variable size so they could be any size but we
should
> pick sane limits as these values are passed to malloc.
> 
> Peter, I've gotten conflicting answers for whether or not unsigned integers
> should be used for computing buffer sizes like this. What is your opinion?

If you're talking about the size of something in memory, you should always use
size_t unless you have a specific reason to need something else.  size_t is
explicitly allowed by the Google style guide.

One way to do a change like this would be to use size_t for these report sizes
but add some sort of sanity check at the point where you're reading the value
from the device.  (Of course, then the question is what to do when the check
fails.  Crash?  Something else?)  Another way would be to just use size_t and
assume that if the size is truly too big malloc() will fail and crash us, but
that probably has the downsides of the "crash" version of the previous option
with the added downside that a malicious device could simply cause us to use
lots of RAM, but not so much that our allocation calls fail.

I would probably avoid other types unless other factors intrude -- e.g. in some
places we use explicitly-sized types because we need to pass them across IPC
barriers, or they're bitfields, or we want to pack values into a smaller size
because there will be many instances of the object in question in memory.  I
suspect none of these sorts of factors matter in your case.

[Peter Kasting, 2014-10-16 01:04:02]

Ping -- I don't know how to proceed here.

[Reilly Grant (use Gerrit), 2014-10-17 22:18:33]

On 2014/10/14 00:27:11, Peter Kasting wrote:
> On 2014/10/14 00:19:21, reillyg wrote:
> > HID usage and usage page IDs are definitely 16-bits. Instead of changing
these
> > we should check and reject the device if we get something too large.
> 
> base::checked_cast<> can be used to narrow the type if you want failure to
crash
> the program.  If you want something else to happen, you'll have to either
write
> the patch or tell me what the right thing to do is :)

If the usage or usage page value does not fit in 16-bits the device should
either be removed from the enumeration or the value should be truncated and a
warning generated. Depending on platform one may be easier than the other. The
HidUsageAndPage type will only accept 16-bit values.
 
> > For the
> > report sizes an upper bound is less defined. An over 64kB report would be
huge
> > and stretch the limitations of USB's HID transport. The integers stored in
the
> > HID report descriptor are variable size so they could be any size but we
> should
> > pick sane limits as these values are passed to malloc.
> > 
> > Peter, I've gotten conflicting answers for whether or not unsigned integers
> > should be used for computing buffer sizes like this. What is your opinion?
> 
> If you're talking about the size of something in memory, you should always use
> size_t unless you have a specific reason to need something else.  size_t is
> explicitly allowed by the Google style guide.
> 
> One way to do a change like this would be to use size_t for these report sizes
> but add some sort of sanity check at the point where you're reading the value
> from the device.  (Of course, then the question is what to do when the check
> fails.  Crash?  Something else?)  Another way would be to just use size_t and
> assume that if the size is truly too big malloc() will fail and crash us, but
> that probably has the downsides of the "crash" version of the previous option
> with the added downside that a malicious device could simply cause us to use
> lots of RAM, but not so much that our allocation calls fail.
> 
> I would probably avoid other types unless other factors intrude -- e.g. in
some
> places we use explicitly-sized types because we need to pass them across IPC
> barriers, or they're bitfields, or we want to pack values into a smaller size
> because there will be many instances of the object in question in memory.  I
> suspect none of these sorts of factors matter in your case.

Sounds like for the report sizes we should use size_t. Defining a maximum report
size is outside the scope of this change.

[Peter Kasting, 2014-10-18 00:52:04]

OK, new version up.  Please check that my error-handling in the case of
out-of-range usage values is actually correct (I don't know whether failing to
push items onto the relevant vectors is going to cause some sort of
size/indexing mismatch issue later).

[Reilly Grant (use Gerrit), 2014-10-18 00:55:39]

On 2014/10/18 00:52:04, Peter Kasting wrote:
> OK, new version up.  Please check that my error-handling in the case of
> out-of-range usage values is actually correct (I don't know whether failing to
> push items onto the relevant vectors is going to cause some sort of
> size/indexing mismatch issue later).

lgtm, how can I compile with these warnings in the future?

[Peter Kasting, 2014-10-18 01:08:28]

On 2014/10/18 00:55:39, reillyg wrote:
> On 2014/10/18 00:52:04, Peter Kasting wrote:
> > OK, new version up.  Please check that my error-handling in the case of
> > out-of-range usage values is actually correct (I don't know whether failing
to
> > push items onto the relevant vectors is going to cause some sort of
> > size/indexing mismatch issue later).
> 
> lgtm, how can I compile with these warnings in the future?

The MSVC warning in question is 4244.

My hope is to get to the point of turning it on someday, but considering that
before I started work it triggered in >1500 files, it's going to take a while.

[Peter Kasting, 2014-10-18 01:08:40]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2014-10-18 01:09:23]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/656583003/280001

[Peter Kasting, 2014-10-18 01:26:06]

The CQ bit was checked by pkasting@chromium.org

[commit-bot: I haz the power, 2014-10-18 01:27:30]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/656583003/300001

[commit-bot: I haz the power, 2014-10-18 02:13:10]

Committed patchset #4 (id:300001)

[commit-bot: I haz the power, 2014-10-18 02:13:59]

Patchset 4 (id:??) landed as
https://crrev.com/4c3b672cc2c1515b7e16cb535e1cd6e69e326607
Cr-Commit-Position: refs/heads/master@{#300201}