Bug 216638 – Add some pointer sanity checks to speculationFromCell().

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

[Mark Lam]

rdar://23226333

[Mark Lam]

Created attachment 408993 [details]
proposed patch.

[Yusuke Suzuki]

Comment on attachment 408993 [details]
proposed patch.

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

> Source/JavaScriptCore/bytecode/SpeculatedType.cpp:559
> +#if USE(JSVALUE64)
> +    uintptr_t pointerAsInt = bitwise_cast<uintptr_t>(pointer);
> +    uintptr_t canonicalPointerBits = pointerAsInt << 16;
> +    uintptr_t nonCanonicalPointerBits = pointerAsInt >> 48;
> +    return !nonCanonicalPointerBits && canonicalPointerBits;
> +#else

I think this is not correct for ARM64_32 since pointer is 32bit.
Let's use CPU(ADDRESS64).

[Mark Lam]

Comment on attachment 408993 [details]
proposed patch.

New patch coming soon to address watch build failure.

[Mark Lam]

Created attachment 408995 [details]
proposed patch.

[Yusuke Suzuki]

Comment on attachment 408995 [details]
proposed patch.

r=me

[Yusuke Suzuki]

Comment on attachment 408995 [details]
proposed patch.

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

> Source/JavaScriptCore/bytecode/SpeculatedType.cpp:568
> +        return SpecNone;

Let's crash if it is debug build.

> Source/JavaScriptCore/bytecode/SpeculatedType.cpp:573
> +                return SpecNone;

Ditto.

> Source/JavaScriptCore/bytecode/SpeculatedType.cpp:581
> +        return SpecNone;

Ditto.

[Mark Lam]

(In reply to Yusuke Suzuki from comment #6)
> Comment on attachment 408995 [details]
> proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=408995&action=review
> 
> > Source/JavaScriptCore/bytecode/SpeculatedType.cpp:568
> > +        return SpecNone;
> 
> Let's crash if it is debug build.

I'm going to add the ASSERT in the isSanePointer() function instead.

[Mark Lam]

Created attachment 408998 [details]
patch for landing.

[Mark Lam]

Created attachment 409010 [details]
proposed patch.

Add 1 more mitigation, and moved the ASSERT into speculationFromCell() after discussing with Yusuke offline.

[Mark Lam]

Comment on attachment 409010 [details]
proposed patch.

Uploaded the wrong patch.

[Mark Lam]

Created attachment 409011 [details]
proposed patch.

[Yusuke Suzuki]

Comment on attachment 409011 [details]
proposed patch.

r=me if EWS gets green.

[Mark Lam]

Created attachment 409012 [details]
proposed patch.

[Yusuke Suzuki]

Comment on attachment 409012 [details]
proposed patch.

r=me

[Mark Lam]

Every test on the jsc-armv7-tests bot is failing with this message: “jsc: error while loading shared libraries: libicudata.so.63: cannot open shared object file: No such file or directory”.  These are not due to this patch.

[Mark Lam]

Comment on attachment 409012 [details]
proposed patch.

Landing now.

[EWS]

Committed r267192: <https://trac.webkit.org/changeset/267192>

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

[Saam Barati]

Comment on attachment 409012 [details]
proposed patch.

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

> Source/JavaScriptCore/bytecode/SpeculatedType.cpp:557
> +    uintptr_t nonCanonicalPointerBits = pointerAsInt >> 48;

we should use the effective address width constant instead of 48

[Mark Lam]

Reopening to address Saam's feedback.

[Mark Lam]

Created attachment 409061 [details]
addressing Saam's feedback.

[Mark Lam]

Thanks for the reviews.  Landed in 267221: <http://trac.webkit.org/r267221>.