[Chromium] WebGL typed array constructor crashes on exception

• Component: WebGL
• Status: RESOLVED FIXED

An object whose 'length' property is an accessor that throws an exception might crash the WebGL typed array constructor when passed as a single parameter. The underlying cause is a missing check for an empty handle after the 'length' property is read. This only applies to the V8 bindings. I have attached a simplified test case that reproduces the problem.

Note that I strongly suspect that the same problem exists when one of the elements is defined as a throwing accessor, but I didn't write a repro for this yet.

Created attachment 181661 [details]
Test case to reproduce bug.

[Kenneth Russell]

Created attachment 182243 [details]
Patch

[Kentaro Hara]

Comment on attachment 182243 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=182243&action=review

> Source/WebCore/bindings/v8/custom/V8ArrayBufferViewCustom.h:176
> +        v8::Local<v8::Value> val = srcArray->Get(v8::String::NewSymbol("length"));
> +        if (val.IsEmpty()) {
> +            // Exception thrown during fetch of length property.

Another way to detect an exception is:

  v8::TryCatch block;
  v8::Local<v8::Value> val = srcArray->Get(v8::String::NewSymbol("length"));
  if (block->hadException())
    return v8Null();

But as far as I see other code, it looks like we are likely to check IsEmpty() rather than using v8::TryCatch. So your code looks fine.

> Source/WebCore/bindings/v8/custom/V8ArrayBufferViewCustom.h:177
> +            return v8::Null();

Nit: v8::Null() => v8Null()

Are you sure that this should be not v8Undefined() but v8Null()?

[Kenneth Russell]

Comment on attachment 182243 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=182243&action=review

>> Source/WebCore/bindings/v8/custom/V8ArrayBufferViewCustom.h:176
>> +            // Exception thrown during fetch of length property.
> 
> Another way to detect an exception is:
> 
>   v8::TryCatch block;
>   v8::Local<v8::Value> val = srcArray->Get(v8::String::NewSymbol("length"));
>   if (block->hadException())
>     return v8Null();
> 
> But as far as I see other code, it looks like we are likely to check IsEmpty() rather than using v8::TryCatch. So your code looks fine.

Yes, I saw the TryCatch class in the V8 API but it seemed more direct to check for the failure case.

>> Source/WebCore/bindings/v8/custom/V8ArrayBufferViewCustom.h:177
>> +            return v8::Null();
> 
> Nit: v8::Null() => v8Null()
> 
> Are you sure that this should be not v8Undefined() but v8Null()?

Looking at other similar code it should be v8Undefined. The return value is ignored however since the exception is thrown to the calling code. I'll upload a new patch.

[Kenneth Russell]

Created attachment 182359 [details]
Patch

[Kentaro Hara]

Comment on attachment 182359 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=182359&action=review

> Source/WebCore/bindings/v8/custom/V8ArrayBufferViewCustom.h:179
> +        len = toUInt32(val);

This conversion can fail too. So you might want to do:

  bool ok;
  len = toUInt32(val, ok);
  if (!ok) {
    ...;
  }

Either way let's fix it in a follow-up patch.

[WebKit Review Bot]

Comment on attachment 182359 [details]
Patch

Clearing flags on attachment: 182359

Committed r139459: <http://trac.webkit.org/changeset/139459>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.