Bug 231975 – JSGenericTypedArrayView<Adaptor>::set crashes if the length + objectOffset is > UINT32_MAX

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Robin Morisset]

UINT_MAX (and anything above it) is not a valid array index, so we cannot use JSObject::get(JSGlobalObject*, unsigned) with an index that big.
This was pointed by Yusuke in his review of my recent patch that introduced the problem (https://bugs.webkit.org/show_bug.cgi?id=229353#c21), but I misunderstood the code and thought we could never get values that big at that point, thus only putting a RELEASE_ASSERT. In this patch I instead apply his original suggestion to have a first loop using the (fast) JSObject::get(), and a second loop for any large indices, using a slower but safe code path.

[Robin Morisset]

rdar://84402043

[Robin Morisset]

Created attachment 441780 [details]
Patch

The new test typed-array-set-large.js is quite slow (4s in release mode on an M1 MBP), so I'd like to put it in a special directory that is not run by EWS by default, but I don't know how to do that.
Let's have it checked by EWS at least once right now, since it already caught 2 bugs for me.

[Robin Morisset]

Created attachment 441823 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 441823 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:328
> +        size_t safeLengthForGet = std::min(length, static_cast<size_t>(std::numeric_limits<uint32_t>::max())) - objectOffset;

Isn't it possible that objectOffset is larger than `std::numeric_limits<uint32_t>::max()`?

[Robin Morisset]

Created attachment 442092 [details]
Patch

[Robin Morisset]

(In reply to Yusuke Suzuki from comment #4)
> Comment on attachment 441823 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=441823&action=review
> 
> > Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:328
> > +        size_t safeLengthForGet = std::min(length, static_cast<size_t>(std::numeric_limits<uint32_t>::max())) - objectOffset;
> 
> Isn't it possible that objectOffset is larger than
> `std::numeric_limits<uint32_t>::max()`?

I could not create a test case that hits that case, but I still reworked the code to be safe just in case it becomes possible in the future.
The reason why it is hard (and probably impossible) to hit that case currently is the following:
- the only caller that can pass objectOffset != 0 is genericTypedArrayViewProtoFuncSlice (it passes `begin`)
- genericTypedArrayViewProtoFuncSlice returns early without calling JSGenericTypedArrayView<Adaptor>::set if length is == 0.
- length is end - begin
- end and begin are at most thisLength which is at most 4GB.
So if objectOffset/begin is > UINT32_MAX, it must be 4GB, so length is at most 0, and so JSGenericTypedArrayView<Adaptor>::set will never be called at all.

[Robin Morisset]

Comment on attachment 442092 [details]
Patch

Some tests are failing, removing r? and cq? until I look into it.

[Robin Morisset]

Created attachment 442197 [details]
Patch

Fixed: I had changed an int into a size_t, and the code was relying on it being decremented until it no longer compared >= 0.

[Robin Morisset]

Created attachment 442234 [details]
Patch

Fix the last bug, which only occurred on some x86_64 machines.
Turns out converting from size_t to double to size_t is not guaranteed to roundtrip, and this caused an issue in genericTypedArrayViewProtoFuncSet.
The fix was fairly simple:
```
-        offset = static_cast<size_t>(std::min(offsetNumber, static_cast<double>(std::numeric_limits<size_t>::max())));
+        if (offsetNumber <= maxSafeInteger() && offsetNumber <= static_cast<double>(std::numeric_limits<size_t>::max()))
+            offset = offsetNumber;
+        else
+            offset = std::numeric_limits<size_t>::max();
```

[Yusuke Suzuki]

Comment on attachment 442234 [details]
Patch

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

r=me

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:328
> +        size_t safeUnadjustedLength = std::min(length, static_cast<size_t>(MAX_ARRAY_INDEX + 1));

Let's change this to `static_cast<size_t>(MAX_ARRAY_INDEX) + 1` to ensure MAX_ARRAY_INDEX will exceed it's max value.
In reality, since it is UINT32_MAX - 1, +1 does not make it 0, but the above form is anyway extra careful without any cost.

> Source/JavaScriptCore/runtime/JSGenericTypedArrayViewInlines.h:339
> +            PropertyName property = PropertyName(ident);

This is not necessary since it has implicit constructor from ident. Let's just pass ident directly to object->get.

JSValue value = object->get(globalObject, ident);

[Robin Morisset]

Created attachment 442243 [details]
Patch

Apply Yusuke's suggestions.

[Robin Morisset]

Created attachment 442863 [details]
Patch

Trivial rebase, sent to EWS again as I really don't see how the hell I could have broken this test, especially in this particular way (the symptom looks exactly like I turned ENABLE_APPLE_PAY_COUPON_CODE from 1 to 0).

[EWS]

Committed r285117 (243758@main): <https://commits.webkit.org/243758@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 442863 [details].