Add bounds-checking in runtime implementations of %FixedArray{Get,Set}

Add bounds-checking in runtime implementations of %FixedArray{Get,Set}

The runtime versions of these intrinsics are used by full-codegen;
hopefully this doesn't noticeably degrade performance (in optimized
code, there's no bounds-checking).

BUG=chromium:504786
LOG=n

[adamk, 2015-06-26 15:10:13]

adamk@chromium.org changed reviewers:
+ arv@chromium.org

[adamk, 2015-06-26 15:15:35]

Thinking about this a bit more, I'm not sure how useful this is. Bounds-checking
here only solves the first-order problem.  If the object pulled out of the
FixedArray isn't either a JS primitive or some kind of JSObject, all bets are
off after this point.

[arv (Not doing code reviews), 2015-06-26 15:19:58]

On 2015/06/26 15:15:35, adamk wrote:
> Thinking about this a bit more, I'm not sure how useful this is.
Bounds-checking
> here only solves the first-order problem.  If the object pulled out of the
> FixedArray isn't either a JS primitive or some kind of JSObject, all bets are
> off after this point.

Yeah, it is not clear that we need to do this? Do we generally guard runtime
functions from invalid input?

[adamk, 2015-06-26 15:26:07]

On 2015/06/26 15:19:58, arv wrote:
> On 2015/06/26 15:15:35, adamk wrote:
> > Thinking about this a bit more, I'm not sure how useful this is.
> Bounds-checking
> > here only solves the first-order problem.  If the object pulled out of the
> > FixedArray isn't either a JS primitive or some kind of JSObject, all bets
are
> > off after this point.
> 
> Yeah, it is not clear that we need to do this? Do we generally guard runtime
> functions from invalid input?

Historically, yes (that's what the "CHECKED" in all the CONVERT_.*_CHECKED()
macros are about). But I'm thinking it makes little sense here.

This already CHECK-fails in debug mode. I'll push back on the bug.

[Michael Starzinger, 2015-06-26 16:09:03]

On 2015/06/26 15:26:07, adamk wrote:
> On 2015/06/26 15:19:58, arv wrote:
> > On 2015/06/26 15:15:35, adamk wrote:
> > > Thinking about this a bit more, I'm not sure how useful this is.
> > Bounds-checking
> > > here only solves the first-order problem.  If the object pulled out of the
> > > FixedArray isn't either a JS primitive or some kind of JSObject, all bets
> are
> > > off after this point.
> > 
> > Yeah, it is not clear that we need to do this? Do we generally guard runtime
> > functions from invalid input?
> 
> Historically, yes (that's what the "CHECKED" in all the CONVERT_.*_CHECKED()
> macros are about). But I'm thinking it makes little sense here.
> 
> This already CHECK-fails in debug mode. I'll push back on the bug.

Yes, I totally see your point here. And with these low-level intrinsics we will
have a hard time of turning all invariants into RUNTIME_CHECKS, instead of
DCHECKS somewhere deep within V8. I don't have an answer on how to solve that,
but I agree that our current strategy might probably need to change.

[adamk, 2015-06-30 18:58:09]

The plan is to have ClusterFuzz only fuzz with certain whitelisted runtime
functions: https://code.google.com/p/chromium/issues/detail?id=505566

Closing this issue.