JSGenericTypedArrayView::set() should check for exceptions.

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

Patch coming.

[Mark Lam]

<rdar://problem/28853775>

[Mark Lam]

Created attachment 292882 [details]
proposed patch.

[JF Bastien]

Comment on attachment 292882 [details]
proposed patch.

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

> JSTests/stress/typed-array-view-set-should-not-crash-on-exception.js:14
> +    get: Date.prototype.getSeconds,

Ew, this is horrible :-)

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:252
>          length = std::min(length, other->length());

This is weird because other->length() is uint32_t and length is unsigned. Realistically unsigned is also uint32_t but if it weren't then it would potentially truncate (or if other->length()'s type were changed) which could lead to OOB things happening below (in the code you're changing, validating ranges of truncated things can get weird). It may be good to static_assert here that sizeof and sign are the same?

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:256
> +        ASSERT(!scope.exception() == success);

I don't understand what this assertion is checking. Success is the same as scope having raised an exception? Why? Exceptions can only occur if the range was borked?

[Mark Lam]

Comment on attachment 292882 [details]
proposed patch.

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

>> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:252
>>          length = std::min(length, other->length());
> 
> This is weird because other->length() is uint32_t and length is unsigned. Realistically unsigned is also uint32_t but if it weren't then it would potentially truncate (or if other->length()'s type were changed) which could lead to OOB things happening below (in the code you're changing, validating ranges of truncated things can get weird). It may be good to static_assert here that sizeof and sign are the same?

This is pre-existing code.  In my understanding, our codebase pretty much uses unsigned and uint32_t interchangeably.  If that premise every becomes untrue, then all shorts of things will fail.  Do we really want to start putting thee static_asserts every where?

>> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:256
>> +        ASSERT(!scope.exception() == success);
> 
> I don't understand what this assertion is checking. Success is the same as scope having raised an exception? Why? Exceptions can only occur if the range was borked?

Calling scope.exception(0 has side effects of satisfying a check for the exception scope verification process.  This is how we do it (satisfy that check) when we want to check an alternate value (i.e. success in this case).

[Mark Lam]

(In reply to comment #4)
> Calling scope.exception(0 has ...

/scope.exception(0/scope.exception()/

[Geoffrey Garen]

Comment on attachment 292882 [details]
proposed patch.

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

>>> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:252
>>>          length = std::min(length, other->length());
>> 
>> This is weird because other->length() is uint32_t and length is unsigned. Realistically unsigned is also uint32_t but if it weren't then it would potentially truncate (or if other->length()'s type were changed) which could lead to OOB things happening below (in the code you're changing, validating ranges of truncated things can get weird). It may be good to static_assert here that sizeof and sign are the same?
> 
> This is pre-existing code.  In my understanding, our codebase pretty much uses unsigned and uint32_t interchangeably.  If that premise every becomes untrue, then all shorts of things will fail.  Do we really want to start putting thee static_asserts every where?

If there were a type mismatch between "length" and "other->length()", the call to min would fail to compile due to ambiguity, no?

For example:

scratch.cc:

int main()
{
    uint32_t x = 0;
    uint16_t y = 0;
    std::min(x, y);
}

clang++ -o scratch scratch.cc:

scratch.cc:16:5: error: no matching function for call to 'min'
    std::min(x, y);
    ^~~~~~~~
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/../include/c++/v1/algorithm:2589:1: note: candidate template ignored: deduced conflicting types for parameter '_Tp'
      ('unsigned int' vs. 'unsigned short')
min(const _Tp& __a, const _Tp& __b)

[JF Bastien]

(In reply to comment #6)
> Comment on attachment 292882 [details]
> proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=292882&action=review
> 
> >>> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:252
> >>>          length = std::min(length, other->length());
> >> 
> >> This is weird because other->length() is uint32_t and length is unsigned. Realistically unsigned is also uint32_t but if it weren't then it would potentially truncate (or if other->length()'s type were changed) which could lead to OOB things happening below (in the code you're changing, validating ranges of truncated things can get weird). It may be good to static_assert here that sizeof and sign are the same?
> > 
> > This is pre-existing code.  In my understanding, our codebase pretty much uses unsigned and uint32_t interchangeably.  If that premise every becomes untrue, then all shorts of things will fail.  Do we really want to start putting thee static_asserts every where?
> 
> If there were a type mismatch between "length" and "other->length()", the
> call to min would fail to compile due to ambiguity, no?
> 
> For example:
> 
> scratch.cc:
> 
> int main()
> {
>     uint32_t x = 0;
>     uint16_t y = 0;
>     std::min(x, y);
> }
> 
> clang++ -o scratch scratch.cc:
> 
> scratch.cc:16:5: error: no matching function for call to 'min'
>     std::min(x, y);
>     ^~~~~~~~
> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.
> xctoolchain/usr/bin/../include/c++/v1/algorithm:2589:1: note: candidate
> template ignored: deduced conflicting types for parameter '_Tp'
>       ('unsigned int' vs. 'unsigned short')
> min(const _Tp& __a, const _Tp& __b)

Oh yeah you're totally right! Ignore me :-)

[Mark Lam]

Thanks for the review.  Landed in r207906: <http://trac.webkit.org/r207906>.