Bug 216638

Summary: Add some pointer sanity checks to speculationFromCell().
Product: WebKit Reporter: Mark Lam <mark.lam>
Component: JavaScriptCoreAssignee: 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 Flags
proposed patch.
ews-feeder: commit-queue-
proposed patch.
ysuzuki: review+
patch for landing.
none
proposed patch.
none
proposed patch.
ysuzuki: review+, ews-feeder: commit-queue-
proposed patch.
none
addressing Saam's feedback. saam: review+

Mark Lam
Reported 2020-09-16 23:01:58 PDT
rdar://23226333
Attachments
proposed patch. (2.78 KB, patch)
2020-09-16 23:27 PDT, Mark Lam 		ews-feeder: commit-queue-
DetailsFormatted DiffDiff
proposed patch. (2.78 KB, patch)
2020-09-16 23:33 PDT, Mark Lam 		ysuzuki: review+
DetailsFormatted DiffDiff
patch for landing. (2.85 KB, patch)
2020-09-17 00:08 PDT, Mark Lam 		no flags
DetailsFormatted DiffDiff
proposed patch. (2.85 KB, patch)
2020-09-17 02:01 PDT, Mark Lam 		no flags
DetailsFormatted DiffDiff
proposed patch. (5.12 KB, patch)
2020-09-17 02:05 PDT, Mark Lam 		ysuzuki: review+
ews-feeder: commit-queue-
DetailsFormatted DiffDiff
proposed patch. (5.52 KB, patch)
2020-09-17 02:11 PDT, Mark Lam 		no flags
DetailsFormatted DiffDiff
addressing Saam's feedback. (1.63 KB, patch)
2020-09-17 13:33 PDT, Mark Lam 		saam: review+
DetailsFormatted DiffDiff
Show Obsolete (5) View All Add attachment proposed patch, testcase, etc.
Mark Lam
Comment 1 2020-09-16 23:27:01 PDT
Created attachment 408993 [details] proposed patch.
Yusuke Suzuki
Comment 2 2020-09-16 23:32:25 PDT
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 3 2020-09-16 23:32:35 PDT
Comment on attachment 408993 [details] proposed patch. New patch coming soon to address watch build failure.
Mark Lam
Comment 4 2020-09-16 23:33:36 PDT
Created attachment 408995 [details] proposed patch.
Yusuke Suzuki
Comment 5 2020-09-16 23:36:09 PDT
Comment on attachment 408995 [details] proposed patch. r=me
Yusuke Suzuki
Comment 6 2020-09-16 23:58:40 PDT
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
Comment 7 2020-09-17 00:08:23 PDT
(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
Comment 8 2020-09-17 00:08:56 PDT
Created attachment 408998 [details] patch for landing.
Mark Lam
Comment 9 2020-09-17 02:01:04 PDT
Created attachment 409010 [details] proposed patch. Add 1 more mitigation, and moved the ASSERT into speculationFromCell() after discussing with Yusuke offline.
Mark Lam
Comment 10 2020-09-17 02:04:04 PDT
Comment on attachment 409010 [details] proposed patch. Uploaded the wrong patch.
Mark Lam
Comment 11 2020-09-17 02:05:16 PDT
Created attachment 409011 [details] proposed patch.
Yusuke Suzuki
Comment 12 2020-09-17 02:10:56 PDT
Comment on attachment 409011 [details] proposed patch. r=me if EWS gets green.
Mark Lam
Comment 13 2020-09-17 02:11:04 PDT
Created attachment 409012 [details] proposed patch.
Yusuke Suzuki
Comment 14 2020-09-17 02:17:37 PDT
Comment on attachment 409012 [details] proposed patch. r=me
Mark Lam
Comment 15 2020-09-17 08:25:01 PDT
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 16 2020-09-17 08:25:25 PDT
Comment on attachment 409012 [details] proposed patch. Landing now.
EWS
Comment 17 2020-09-17 08:43:23 PDT
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 18 2020-09-17 12:11:08 PDT
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
Comment 19 2020-09-17 12:35:16 PDT
Reopening to address Saam's feedback.
Mark Lam
Comment 20 2020-09-17 13:33:45 PDT
Created attachment 409061 [details] addressing Saam's feedback.
Mark Lam
Comment 21 2020-09-17 19:07:22 PDT
Thanks for the reviews. Landed in 267221: <http://trac.webkit.org/r267221>.
Note You need to log in before you can comment on or make changes to this bug.