| Summary: | Add some pointer sanity checks to speculationFromCell(). | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Mark Lam <mark.lam> | ||||||||||||||||
| Component: | JavaScriptCore | Assignee: | Mark Lam <mark.lam> | ||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||
| Severity: | Normal | CC: | ews-watchlist, keith_miller, msaboff, saam, tzagallo, webkit-bug-importer, ysuzuki | ||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||
| Attachments: |
|
||||||||||||||||||
|
Description
Mark Lam
2020-09-16 23:01:58 PDT
Created attachment 408993 [details]
proposed patch.
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). Comment on attachment 408993 [details]
proposed patch.
New patch coming soon to address watch build failure.
Created attachment 408995 [details]
proposed patch.
Comment on attachment 408995 [details]
proposed patch.
r=me
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. (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. Created attachment 408998 [details]
patch for landing.
Created attachment 409010 [details]
proposed patch.
Add 1 more mitigation, and moved the ASSERT into speculationFromCell() after discussing with Yusuke offline.
Comment on attachment 409010 [details]
proposed patch.
Uploaded the wrong patch.
Created attachment 409011 [details]
proposed patch.
Comment on attachment 409011 [details]
proposed patch.
r=me if EWS gets green.
Created attachment 409012 [details]
proposed patch.
Comment on attachment 409012 [details]
proposed patch.
r=me
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. Comment on attachment 409012 [details]
proposed patch.
Landing now.
Committed r267192: <https://trac.webkit.org/changeset/267192> All reviewed patches have been landed. Closing bug and clearing flags on attachment 409012 [details]. 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 Reopening to address Saam's feedback. Created attachment 409061 [details]
addressing Saam's feedback.
Thanks for the reviews. Landed in 267221: <http://trac.webkit.org/r267221>. |