Bug 231975 - JSGenericTypedArrayView<Adaptor>::set crashes if the length + objectOffset is > UINT32_MAX
Summary: JSGenericTypedArrayView<Adaptor>::set crashes if the length + objectOffset is...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P1 Normal
Assignee: Robin Morisset
URL:
Keywords: InRadar
Depends on: 229353
Blocks:
  Show dependency treegraph
 
Reported: 2021-10-19 12:29 PDT by Robin Morisset
Modified: 2021-11-01 11:01 PDT (History)
8 users (show)

See Also:


Attachments
Patch (7.84 KB, patch)
2021-10-19 13:17 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff
Patch (7.85 KB, patch)
2021-10-19 17:23 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff
Patch (28.07 KB, patch)
2021-10-21 17:09 PDT, Robin Morisset
rmorisset: review-
rmorisset: commit-queue-
Details | Formatted Diff | Diff
Patch (28.69 KB, patch)
2021-10-22 14:01 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff
Patch (28.80 KB, patch)
2021-10-22 17:56 PDT, Robin Morisset
ysuzuki: review+
Details | Formatted Diff | Diff
Patch (28.74 KB, patch)
2021-10-22 19:03 PDT, Robin Morisset
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (28.70 KB, patch)
2021-10-29 14:30 PDT, Robin Morisset
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Robin Morisset 2021-10-19 12:29:40 PDT
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.
Comment 1 Robin Morisset 2021-10-19 12:37:12 PDT
rdar://84402043
Comment 2 Robin Morisset 2021-10-19 13:17:07 PDT
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.
Comment 3 Robin Morisset 2021-10-19 17:23:41 PDT
Created attachment 441823 [details]
Patch
Comment 4 Yusuke Suzuki 2021-10-19 18:33:58 PDT
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()`?
Comment 5 Robin Morisset 2021-10-21 17:09:44 PDT
Created attachment 442092 [details]
Patch
Comment 6 Robin Morisset 2021-10-21 17:18:15 PDT
(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.
Comment 7 Robin Morisset 2021-10-22 09:34:55 PDT
Comment on attachment 442092 [details]
Patch

Some tests are failing, removing r? and cq? until I look into it.
Comment 8 Robin Morisset 2021-10-22 14:01:52 PDT
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.
Comment 9 Robin Morisset 2021-10-22 17:56:47 PDT
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();
```
Comment 10 Yusuke Suzuki 2021-10-22 18:43:20 PDT
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);
Comment 11 Robin Morisset 2021-10-22 19:03:32 PDT
Created attachment 442243 [details]
Patch

Apply Yusuke's suggestions.
Comment 12 Robin Morisset 2021-10-29 14:30:44 PDT
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).
Comment 13 EWS 2021-11-01 11:01:29 PDT
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].