Add bounds checks to SkTDynamicHash

Add bounds checks to SkTDynamicHash

BUG=391001
Committed: https://skia.googlesource.com/skia/+/2e09d18f78f5772d2d9506ff8024ca8542654358

[mtklein, 2014-07-08 18:39:04]

+sugoi

Duh, can't hurt to have someone with context review this.  Brian, you're of
course welcome to look too.

[sugoi1, 2014-07-08 18:45:22]

I don't have any problems with this code, but it's not actually going to fix the
bug you point to in your description.
Would it be too expensive to add these checks to the for loop conditions?

[mtklein, 2014-07-08 18:59:37]

On 2014/07/08 18:45:22, sugoi1 wrote:
> I don't have any problems with this code, but it's not actually going to fix
the
> bug you point to in your description.
> Would it be too expensive to add these checks to the for loop conditions?

Sure it will.  If we're reading off either end of the memory we've allocated
here, it could show up as that MSAN error.

This check _is_ done on the for-loop condition.  We check the bounds of every
index we'll look just before looking.

[sugoi, 2014-07-08 19:24:23]

On 2014/07/08 18:59:37, mtklein wrote:
> On 2014/07/08 18:45:22, sugoi1 wrote:
> > I don't have any problems with this code, but it's not actually going to fix
the
> > bug you point to in your description.
> > Would it be too expensive to add these checks to the for loop conditions?
> 
> Sure it will.  If we're reading off either end of the memory we've allocated
> here, it could show up as that MSAN error.
> 
> This check _is_ done on the for-loop condition.  We check the bounds of every
> index we'll look just before looking.

Ok, what's confusing me is that the code you added is DEBUG only and the bug
itself isn't, so I don't see how adding DEBUG only code would fix it.
What I meant was to add it to the condition so the the loop exits early, like:

for (int round = 0; round < fCapacity && index >= 0 && index < fCapacity;
round++) {

But maybe that's too expensive for the non DEBUG case or maybe we actually want
to crash, but SkASSERT won't do anything in RELEASE builds, right?

[mtklein, 2014-07-08 19:30:01]

> Ok, what's confusing me is that the code you added is DEBUG only and the bug
> itself isn't, so I don't see how adding DEBUG only code would fix it.
> What I meant was to add it to the condition so the the loop exits early, like:

Why would you suspect this code behaves differently in Release and Debug mode? 
If the bug's there in Release, surely it's there in Debug?

> for (int round = 0; round < fCapacity && index >= 0 && index < fCapacity;
> round++) {
> 
> But maybe that's too expensive for the non DEBUG case or maybe we actually
want
> to crash, but SkASSERT won't do anything in RELEASE builds, right?

Yes, I don't want to do any extra work in Release mode.

[mtklein, 2014-07-09 16:06:10]

Turns out going out of range on fArray is unrelated to the bug: index was
tainted by uninitialized data from a partly-uninitialized hash key, but was
mechanically forced into [0, fCapacity) range anyway.

Can't hurt to assert more invariants in Debug mode, so I'm going to land this
anyway.

[mtklein, 2014-07-09 16:06:16]

lgtm

[commit-bot: I haz the power, 2014-07-09 16:06:29]

CQ is trying da patch. Follow status at
 https://skia-tree-status.appspot.com/cq/mtklein@chromium.org/380443002/1

[commit-bot: I haz the power, 2014-07-09 16:21:17]

Change committed as 2e09d18f78f5772d2d9506ff8024ca8542654358