Bug 76724

Summary: ENABLE(ASSEMBLER_WX_EXCLUSIVE): LinkBuffer can leave pages not marked as executable.
Product: WebKit Reporter: Yong Li <yong.li.webkit>
Component: JavaScriptCoreAssignee: Yong Li <yong.li.webkit>
Status: RESOLVED FIXED    
Severity: Critical CC: barraclough, fpizlo, ggaren, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Other   
OS: Other   
Attachments:
Description Flags
workaround
none
the patch that fixes LinkBuffer
rwlbuis: review-
Updated none

Yong Li
Reported 2012-01-20 12:05:40 PST
MetaAllocator breaks ENABLE(ASSEMBLER_WX_EXCLUSIVE) because the flags can only be applied to whole pages.
Attachments
workaround (2.40 KB, patch)
2012-01-20 12:11 PST, Yong Li 		no flags
DetailsFormatted DiffDiff
the patch that fixes LinkBuffer (3.94 KB, patch)
2012-02-09 09:02 PST, Yong Li 		rwlbuis: review-
DetailsFormatted DiffDiff
Updated (4.15 KB, patch)
2012-02-10 11:07 PST, Yong Li 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Yong Li
Comment 1 2012-01-20 12:11:59 PST
Created attachment 123353 [details] workaround
Geoffrey Garen
Comment 2 2012-01-20 12:34:37 PST
Do any platforms still use WX_EXCLUSIVE?
Yong Li
Comment 3 2012-01-20 13:01:05 PST
(In reply to comment #2) > Do any platforms still use WX_EXCLUSIVE? Yes BlackBerry/QNX uses it
Geoffrey Garen
Comment 4 2012-01-20 13:16:45 PST
Comment on attachment 123353 [details] workaround I guess I'll say r+ to unbreak that platform. But you're going to get really bad memory use if every JS function is 1 page at minimum.
Yong Li
Comment 5 2012-01-20 13:19:05 PST
(In reply to comment #4) > (From update of attachment 123353 [details]) > I guess I'll say r+ to unbreak that platform. But you're going to get really bad memory use if every JS function is 1 page at minimum. Thanks. I'll try to think about some other solution...
Yong Li
Comment 6 2012-02-09 08:57:43 PST
Found a bug in LinkedBuffer. The size used to call makeExecutable can be smaller than the one that was used for makeWritable. So it can leave pages that are not set back to default flags. When an assembly on that page is executed or JIT returns to that page in the case it was already executing that page, the software will crash. Patch is on the way. But I'm not sure MetaAllocator is safe with ENABLE(ASSEMBLER_WX_EXCLUSIVE) even with this fix. Geoffrey, do you see potential problems with MetaAllocator + ENABLE(ASSEMBLER_WX_EXCLUSIVE) + Workers?
Yong Li
Comment 7 2012-02-09 09:02:35 PST
Created attachment 126314 [details] the patch that fixes LinkBuffer
Yong Li
Comment 8 2012-02-09 10:28:23 PST
(In reply to comment #6) > Found a bug in LinkedBuffer. The size used to call makeExecutable can be smaller than the one that was used for makeWritable. So it can leave pages that are not set back to default flags. When an assembly on that page is executed or JIT returns to that page in the case it was already executing that page, the software will crash. > > Patch is on the way. > > But I'm not sure MetaAllocator is safe with ENABLE(ASSEMBLER_WX_EXCLUSIVE) even with this fix. Geoffrey, do you see potential problems with MetaAllocator + ENABLE(ASSEMBLER_WX_EXCLUSIVE) + Workers? never mind. worker should OK because worker uses its own global object.
Eric Seidel (no email)
Comment 9 2012-02-10 10:19:07 PST
Comment on attachment 123353 [details] workaround Cleared Geoffrey Garen's review+ from obsolete attachment 123353 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Rob Buis
Comment 10 2012-02-10 10:59:14 PST
Comment on attachment 126314 [details] the patch that fixes LinkBuffer r- because it is fine for ENABLE(BRANCH_COMPACTION), but not the other case.
Yong Li
Comment 11 2012-02-10 11:00:29 PST
(In reply to comment #10) > (From update of attachment 126314 [details]) > r- because it is fine for ENABLE(BRANCH_COMPACTION), but not the other case. Good catch! it would cause problem when !ENABLE(BRANCH_COMPACTION) && ENABLE(ASSEMBLER_WX_EXCLUSIVE)...
Yong Li
Comment 12 2012-02-10 11:07:45 PST
Created attachment 126537 [details] Updated
Rob Buis
Comment 13 2012-02-10 11:11:03 PST
Comment on attachment 126537 [details] Updated Looks good.
WebKit Review Bot
Comment 14 2012-02-10 15:02:15 PST
Comment on attachment 126537 [details] Updated Clearing flags on attachment: 126537 Committed r107447: <http://trac.webkit.org/changeset/107447>
WebKit Review Bot
Comment 15 2012-02-10 15:02:20 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.