Bug 211205 – We can't cast toLength result to unsigned

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

[Saam Barati]

This is UB in C++, and does different things depending on what architecture you're on.

For example, on x86, this will give us a length of zero, which violates the spec:

double x = static_cast<double>(UINT_MAX) + 1;
unsigned length = x;

On arm64, this will give us UINT_MAX, which is also wrong.

[Radar WebKit Bug Importer]

<rdar://problem/62625562>

[Saam Barati]

Created attachment 398023 [details]
WIP

[Saam Barati]

Created attachment 398027 [details]
WIP

This is pretty annoying, but I think I'm close to done.

This is also making us more spec compliant. We'll also need to start throwing errors when producing length > maxSafeInteger.

[Alexey Shvayka]

*** Bug 163417 has been marked as a duplicate of this bug. ***

[Saam Barati]

Created attachment 398122 [details]
patch

[Yusuke Suzuki]

Comment on attachment 398122 [details]
patch

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

r=me. Please ensure that performance is neutral.

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:930
> +        if (LIKELY(length + n <= std::numeric_limits<uint32_t>::max()))

Since maximum-array-index is not UINT32_MAX (MAX_ARRAY_INDEX is UINT32_MAX - 1), should we check `length + n <= MAX_ARRAY_INDEX` instead?

> Source/JavaScriptCore/runtime/JSObject.cpp:1963
> +    if (LIKELY(propertyName <= std::numeric_limits<unsigned>::max()))

Ditto.

> Source/JavaScriptCore/runtime/JSObject.h:214
> +        if (LIKELY(propertyName <= std::numeric_limits<uint32_t>::max()))

Ditto.

> Source/JavaScriptCore/runtime/JSObject.h:262
> +        if (LIKELY(propertyName <= std::numeric_limits<uint32_t>::max()))

Ditto.

> Source/JavaScriptCore/runtime/JSObject.h:312
> +        if (LIKELY(i <= std::numeric_limits<uint32_t>::max()))

Ditto.

> Source/JavaScriptCore/runtime/JSObject.h:380
> +        if (LIKELY(i <= std::numeric_limits<uint32_t>::max()))

Ditto.

> Source/JavaScriptCore/runtime/JSObjectInlines.h:154
> +    if (LIKELY(propertyName <= std::numeric_limits<uint32_t>::max()))

Ditto.

> Source/JavaScriptCore/runtime/JSObjectInlines.h:558
> +    if (LIKELY(propertyName <= std::numeric_limits<uint32_t>::max()))

Ditto.

> Source/JavaScriptCore/runtime/JSObjectInlines.h:566
> +    if (LIKELY(propertyName <= std::numeric_limits<uint32_t>::max()))

Ditto.

[Alexey Shvayka]

(In reply to Saam Barati from comment #5)
> Created attachment 398122 [details]
> patch

I wonder if `i > MAX_ARRAY_INDEX` check in JSObject::deletePropertyByIndex() is useful , considering `unsigned i`?

Would you please unskip test/built-ins/Array/* files in JSTests/test/config.yaml? They should be passing (and fast) with the patch.

[Saam Barati]

Comment on attachment 398122 [details]
patch

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

>> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:930
>> +        if (LIKELY(length + n <= std::numeric_limits<uint32_t>::max()))
> 
> Since maximum-array-index is not UINT32_MAX (MAX_ARRAY_INDEX is UINT32_MAX - 1), should we check `length + n <= MAX_ARRAY_INDEX` instead?

Yeah I think you’re right. Maybe I should add an isIndex() specialized on uint64_t parameter

[Mark Lam]

Comment on attachment 398122 [details]
patch

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

> Source/JavaScriptCore/ChangeLog:9
> +        toLength, according to the spec, returns an 53-bit integer. In our

/an/a/

> Source/JavaScriptCore/ChangeLog:17
> +        - Casting to unsigned from double is undefined behavior when the integer

/integer/integer is/

> Source/JavaScriptCore/ChangeLog:25
> +        This patch addresses each bad use the undefined behavior, and by doing so,

/bad use the/bad use of the/

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:385
> +        if (count + length > std::numeric_limits<uint32_t>::max()) {

Do we have a guarantee that count + length can never overflow?  I think in practice, this will never be, but can you add a debug ASSERT here just in case?  Either that or use CheckedInt32 and be sure.

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:926
> +    if (UNLIKELY(length + argCount > static_cast<uint64_t>(maxSafeInteger())))

Do we need an overflow check for length + argCount here?  Or do we have a guarantee that both length and argCount are less than UINT_MAX?  If so, can you add debug ASSERT on those?

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:1257
> +        if (UNLIKELY(length + nrArgs > static_cast<uint64_t>(maxSafeInteger())))

Ditto: overflow check?

[Alexey Shvayka]

Comment on attachment 398122 [details]
patch

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

If we want to support all edge cases of the spec, we should vet all OOM throws.

> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:1112
> +            throwOutOfMemoryError(globalObject, scope);

We should throw a RangeError here.

> Source/JavaScriptCore/runtime/JSONObject.cpp:482
> +                throwOutOfMemoryError(globalObject, scope);

If there are indexed getters between 0 and `length`, we should invoke them.

> Source/JavaScriptCore/runtime/JSONObject.cpp:657
> +                    throwOutOfMemoryError(m_globalObject, scope);

Ditto.

[Saam Barati]

Comment on attachment 398122 [details]
patch

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

>> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:385
>> +        if (count + length > std::numeric_limits<uint32_t>::max()) {
> 
> Do we have a guarantee that count + length can never overflow?  I think in practice, this will never be, but can you add a debug ASSERT here just in case?  Either that or use CheckedInt32 and be sure.

That's correct. Because these are expected to be 53-bit integers. I can assert in the prologue of this function.

>> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:926
>> +    if (UNLIKELY(length + argCount > static_cast<uint64_t>(maxSafeInteger())))
> 
> Do we need an overflow check for length + argCount here?  Or do we have a guarantee that both length and argCount are less than UINT_MAX?  If so, can you add debug ASSERT on those?

same reason. This is a 53-bit int since it came from toLength.

>> Source/JavaScriptCore/runtime/ArrayPrototype.cpp:1112
>> +            throwOutOfMemoryError(globalObject, scope);
> 
> We should throw a RangeError here.

Why? The spec essentially allows for engines to throw OOM whenever it feels like it. Why is RangeError better?

>> Source/JavaScriptCore/runtime/JSONObject.cpp:482
>> +                throwOutOfMemoryError(globalObject, scope);
> 
> If there are indexed getters between 0 and `length`, we should invoke them.

Same comment. My understand is spec allows us to OOM whenever we feel like it. This feels like a reasonable thing to do here.

>> Source/JavaScriptCore/runtime/JSONObject.cpp:657
>> +                    throwOutOfMemoryError(m_globalObject, scope);
> 
> Ditto.

ditto

[Saam Barati]

(In reply to Alexey Shvayka from comment #7)
> (In reply to Saam Barati from comment #5)
> > Created attachment 398122 [details]
> > patch
> 
> I wonder if `i > MAX_ARRAY_INDEX` check in JSObject::deletePropertyByIndex()
> is useful , considering `unsigned i`?
> 
> Would you please unskip test/built-ins/Array/* files in
> JSTests/test/config.yaml? They should be passing (and fast) with the patch.

What tests do you mean here?

[Saam Barati]

Created attachment 398164 [details]
patch for landing

[Saam Barati]

A quick smoke test shows this is neutral on JS2 on the cli

[Saam Barati]

Created attachment 398165 [details]
patch for landing

[EWS]

Committed r260990: <https://trac.webkit.org/changeset/260990>

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

[Saam Barati]

watchOS build fixes in:
https://trac.webkit.org/changeset/261005/webkit
https://trac.webkit.org/changeset/261006/webkit

[Alexey Shvayka]

*** Bug 164456 has been marked as a duplicate of this bug. ***

[Alexey Shvayka]

*** Bug 177326 has been marked as a duplicate of this bug. ***