Bug 214953 – validate untagArrayPtr

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

[Saam Barati]

....

[Saam Barati]

Created attachment 405627 [details]
patch

[Saam Barati]

Comment on attachment 405627 [details]
patch

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

> Source/JavaScriptCore/assembler/MacroAssemblerARM64E.h:135
> +        TrustedImm32 shiftAmount { 64 - OS_CONSTANT(EFFECTIVE_ADDRESS_WIDTH) };

Will switch to using Keith's new constant once it lands.

> Source/JavaScriptCore/jit/AssemblyHelpers.cpp:1152
> +        skip = branchPtr(Equal, storage, TrustedImmPtr(JSArrayBufferView::nullVectorPtr()));

this is the change.

> Source/JavaScriptCore/jit/AssemblyHelpers.cpp:1188
> +            done.append(branchPtr(Equal, storage, TrustedImmPtr(JSArrayBufferView::nullVectorPtr())));

this is the changed line.

[Saam Barati]

Created attachment 405646 [details]
patch

[Keith Miller]

Comment on attachment 405646 [details]
patch

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

r=me with some comments.

> Source/JavaScriptCore/assembler/MacroAssemblerARM64E.h:135
> +        TrustedImm32 shiftAmount { 64 - OS_CONSTANT(EFFECTIVE_ADDRESS_WIDTH) };

Nit: Can you use numberOfPACBits?

> Source/JavaScriptCore/assembler/MacroAssemblerARM64E.h:137
> +        lshift64(shiftAmount, target);
> +        urshift64(shiftAmount, target);

I think this can probably be a single and instruction for each of the constants we currently use.

[Saam Barati]

Created attachment 405710 [details]
patch for landing

[EWS]

Committed r265151: <https://trac.webkit.org/changeset/265151>

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

[Radar WebKit Bug Importer]

<rdar://problem/66391434>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 215074

[Saam Barati]

Created attachment 424271 [details]
patch

[Mark Lam]

Comment on attachment 424271 [details]
patch

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

> Source/JavaScriptCore/assembler/MacroAssemblerARM64E.h:149
> +        auto scratch = InvalidGPRReg; 
> +        if (validateAuth) {
> +            scratch = getCachedMemoryTempRegisterIDAndInvalidate();
> +            move(target, scratch);
> +        }
> +
>          m_assembler.autdb(target, lengthGPR);
> +
> +        if (validateAuth) {
> +            ASSERT(target != ARM64Registers::sp);
> +            removeArrayPtrTag(scratch);
> +            auto isValidPtr = branch64(Equal, scratch, target);
> +            breakpoint(0xc473);
> +            isValidPtr.link(this);
> +        }

Let's do this instead:

        auto scratch = validateAuth ? getCachedMemoryTempRegisterIDAndInvalidate() : InvalidGPRReg;
        untagArrayPtr(length, target, validateAuth, scratch);

> Source/JavaScriptCore/assembler/MacroAssemblerARM64E.h:157
> +        ASSERT(LogicalImmediate::create64(nonPACBitsMask).isValid());
> +        and64(TrustedImmPtr(nonPACBitsMask), target);

Isn't nonPACBitsMask a member of CagedPtr?  Did this actually build?

> Source/JavaScriptCore/jit/AssemblyHelpers.cpp:1074
> +#if CPU(ARM64E)
> +            done.append(branchPtr(Equal, storage, TrustedImmPtr(JSArrayBufferView::nullVectorPtr())));
> +#endif

Isn't this also needed for x86_64 because null != neutered?

[Saam Barati]

Comment on attachment 424271 [details]
patch

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

>> Source/JavaScriptCore/assembler/MacroAssemblerARM64E.h:149
>> +        }
> 
> Let's do this instead:
> 
>         auto scratch = validateAuth ? getCachedMemoryTempRegisterIDAndInvalidate() : InvalidGPRReg;
>         untagArrayPtr(length, target, validateAuth, scratch);

sounds good

>> Source/JavaScriptCore/assembler/MacroAssemblerARM64E.h:157
>> +        and64(TrustedImmPtr(nonPACBitsMask), target);
> 
> Isn't nonPACBitsMask a member of CagedPtr?  Did this actually build?

It's also defined above in this file:
    static constexpr uintptr_t nonPACBitsMask = (1ull << (64 - numberOfPACBits)) - 1;

>> Source/JavaScriptCore/jit/AssemblyHelpers.cpp:1074
>> +#endif
> 
> Isn't this also needed for x86_64 because null != neutered?

Detached array buffer is zero on x86:
void JSArrayBufferView::detach()
{
    auto locker = holdLock(cellLock());
    RELEASE_ASSERT(hasArrayBuffer());
    RELEASE_ASSERT(!isShared());
    m_length = 0;
    m_vector.clear();
}

[Mark Lam]

Comment on attachment 424271 [details]
patch

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

r=me

>>> Source/JavaScriptCore/assembler/MacroAssemblerARM64E.h:157
>>> +        and64(TrustedImmPtr(nonPACBitsMask), target);
>> 
>> Isn't nonPACBitsMask a member of CagedPtr?  Did this actually build?
> 
> It's also defined above in this file:
>     static constexpr uintptr_t nonPACBitsMask = (1ull << (64 - numberOfPACBits)) - 1;

sorry, I had a bad grep.

>>> Source/JavaScriptCore/jit/AssemblyHelpers.cpp:1074
>>> +#endif
>> 
>> Isn't this also needed for x86_64 because null != neutered?
> 
> Detached array buffer is zero on x86:
> void JSArrayBufferView::detach()
> {
>     auto locker = holdLock(cellLock());
>     RELEASE_ASSERT(hasArrayBuffer());
>     RELEASE_ASSERT(!isShared());
>     m_length = 0;
>     m_vector.clear();
> }

I think it is a bug for x86_64 to use 0 to both mean null and neutered.  But that's unrelated to this patch.  So, moving on.

[Saam Barati]

Created attachment 424284 [details]
patch for landing

[EWS]

Committed r275079: <https://commits.webkit.org/r275079>

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

[Saam Barati]

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