Bug 226503 – Fix inadvertent tag corruption in functionAddressOf

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

[Mikhail R. Gadelha]

Created attachment 430269 [details]
test case

The attached program triggers an assertion failure when running jsc in 32 bits and debug mode.

In 32 bits, asCell() returns a pointer to the payload and using reinterpret_cast will sign extend asNumber to FFFF FFFF + <value-address>, which will fail the isImpureNaN in jsNumber. 

So first cast the address to uint32_t then to uint64_t to prevent the tag corruption.

[Mikhail R. Gadelha]

Created attachment 430271 [details]
Patch

[Mikhail R. Gadelha]

Created attachment 430274 [details]
Patch

[Darin Adler]

Comment on attachment 430274 [details]
Patch

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

> Source/JavaScriptCore/jsc.cpp:1493
> +#if USE(JSVALUE32_64)
> +    // In 32 bits, asCell() returns a pointer to the payload and using reinterpret_cast
> +    // will sign extend asNumber to FFFF FFFF + <value-address>, which will fail the
> +    // isImpureNaN check in jsNumber. To prevent that, first reinterpret_cast the address 
> +    // to uint32_t then cast it to uint64_t. AddressOf in 32 bits will return the same 
> +    // address returned by describe() but in double
> +    uint64_t asNumber = static_cast<uint64_t>(reinterpret_cast<uint32_t>(value.asCell()));
> +#else
>      // Need to cast to uint64_t so bitwise_cast will play along.
>      uint64_t asNumber = reinterpret_cast<uint64_t>(value.asCell());
> +#endif

Better fix is to just replace the incorrect <uint64_t> with the correct <uintptr_t> instead of adding code and comments. No need to use static_cast to expand a 32-bit unsigned int to 64-bit.

[Mikhail R. Gadelha]

Indeed, <uintptr_t> sounds like a better solution here, but we would still need the static_cast to 64 bits because of the bitcast<double>.

In 32 bits <uintptr_t> is 32 bits long and the bitcast will fail.

[Darin Adler]

(In reply to Mikhail R. Gadelha from comment #4)
> Indeed, <uintptr_t> sounds like a better solution here, but we would still
> need the static_cast to 64 bits because of the bitcast<double>.

No static_cast needed. Write this:

    uint64_t asNumber = reinterpret_cast<uintptr_t>(value.asCell());

There’s no need for a static_cast on this line of code. The unsigned integer conversion, with possible promotion, can be done without any casting.

[Mikhail R. Gadelha]

I got what you mean now!

I'll send an updated patch shortly

[Mikhail R. Gadelha]

Created attachment 430302 [details]
Patch

[Darin Adler]

Comment on attachment 430302 [details]
Patch

I suspect that other callers of reinterpret_cast<uint64_t> throughout WebKit could have the same issue on 32-bit. There are 20 of them at least, and many are likely converting from pointers to integers; the same fix would likely apply.

[Radar WebKit Bug Importer]

<rdar://problem/79015579>

[EWS]

Committed r278660 (238642@main): <https://commits.webkit.org/238642@main>

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