Bug 227170 – Prevent sign-extended casts for 32 bits arch

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

[Mikhail R. Gadelha]

In a number of places, addresses are reinterpreted as uint64, which can lead to wrong addresses in 32 bits arch. 

For example, in https://bugs.webkit.org/show_bug.cgi?id=226503, asCell() returns a pointer to the payload and using reinterpret_cast will sign extend asNumber to FFFF FFFF + , which will fail the isImpureNaN check in jsNumber.

[Mikhail R. Gadelha]

Created attachment 431777 [details]
Patch

[Mikhail R. Gadelha]

I've also added a bitwise_cast there, which is not memory-related but it makes the code slightly more readable.

[Mikhail R. Gadelha]

Created attachment 431781 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 431781 [details]
Patch

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

r=me with nits

> Source/JavaScriptCore/disassembler/ARM64/A64DOpcode.h:190
> +        bufferPrintf("0x%" PRIxPTR, reinterpret_cast<uintptr_t>(pc + immediate));

Using bitwise_cast would be better since it checks whether the start() is the same size to uintptr_t.

> Source/JavaScriptCore/runtime/JSCell.cpp:295
> +        ptrdiff_t cellOffset = cellAddress - reinterpret_cast<uintptr_t>(foundBlockHandle->start());

Using bitwise_cast would be better since it checks whether the start() is the same size to uintptr_t.

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:2799
> +    append(Move, Arg::bigImm(reinterpret_cast<uintptr_t>(&m_tierUp->m_counter)), countdownPtr);

Ditto.

> Source/JavaScriptCore/wasm/WasmAirIRGenerator.cpp:2851
> +    append(Move, Arg::bigImm(reinterpret_cast<uintptr_t>(&m_tierUp->m_counter)), countdownPtr);

Ditto.

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:2033
> +    Value* countDownLocation = constant(pointerType(), reinterpret_cast<uintptr_t>(&m_tierUp->m_counter), Origin());

Ditto.

> Source/JavaScriptCore/wasm/WasmB3IRGenerator.cpp:2083
> +    Value* countDownLocation = constant(pointerType(), reinterpret_cast<uintptr_t>(&m_tierUp->m_counter), origin);

Ditto.

> Source/WTF/wtf/LoggerHelper.h:79
> +        return reinterpret_cast<const void*>((reinterpret_cast<uintptr_t>(parentIdentifier) & parentMask) | (childIdentifier & maskLowerWord));

Ditto.

[Yusuke Suzuki]

Comment on attachment 431781 [details]
Patch

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

>> Source/WTF/wtf/LoggerHelper.h:79
>> +        return reinterpret_cast<const void*>((reinterpret_cast<uintptr_t>(parentIdentifier) & parentMask) | (childIdentifier & maskLowerWord));
> 
> Ditto.

Is this correct? We are using `static constexpr uint64_t`

[Mikhail R. Gadelha]

Created attachment 431863 [details]
Patch

[Mikhail R. Gadelha]

Comment on attachment 431781 [details]
Patch

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

>>> Source/WTF/wtf/LoggerHelper.h:79
>>> +        return reinterpret_cast<const void*>((reinterpret_cast<uintptr_t>(parentIdentifier) & parentMask) | (childIdentifier & maskLowerWord));
>> 
>> Ditto.
> 
> Is this correct? We are using `static constexpr uint64_t`

The compiler will introduce an implicit cast from uintptr_t to uint64_t in 32 bits mode... Here's the clang ast dump (I actually just copied the function to a separate file but it should generate the same code during compilation):

      | `-BinaryOperator 0x5655a48 <col:47, col:91> 'unsigned long long' '&'
HERE->|   |-ImplicitCastExpr 0x5655a30 <col:47, col:87> 'unsigned long long' <IntegralCast>
      |   | `-CallExpr 0x5655998 <col:47, col:87> 'unsigned int':'unsigned int'
      |   |   |-ImplicitCastExpr 0x5655980 <col:47, col:69> 'unsigned int (*)(const void *)' <FunctionToPointerDecay>
      |   |   | `-DeclRefExpr 0x56558b8 <col:47, col:69> 'unsigned int (const void *)' lvalue Function 0x56557a0 'bitwise_cast' 'unsigned int (const void *)' (FunctionTemplate 0x5653770 'bitwise_cast')
      |   |   `-ImplicitCastExpr 0x56559c0 <col:71> 'const void *' <LValueToRValue>
      |   |     `-DeclRefExpr 0x56554e0 <col:71> 'const void *' lvalue ParmVar 0x5654f98 'parentIdentifier' 'const void *'
      |   `-ImplicitCastExpr 0x5655a18 <col:91> 'uint64_t':'unsigned long long' <LValueToRValue>
      |     `-DeclRefExpr 0x56559f8 <col:91> 'const uint64_t':'const unsigned long long' lvalue Var 0x5655218 'parentMask' 'const uint64_t':'const unsigned long long' non_odr_use_constant

No implicit cast is added in 64 bits mode.

[Mikhail R. Gadelha]

Created attachment 432265 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/79779047>

[EWS]

Committed r279341 (239209@main): <https://commits.webkit.org/239209@main>

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