Last Comment Bug 1234246 - Don't reprotect more than once when linking code
: Don't reprotect more than once when linking code
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine: JIT (show other bugs)
: unspecified
: All All
P5 normal (vote)
: ---
Assigned To: Jan de Mooij [:jandem]
:
: Hannes Verschore [:h4writer]
Mentors:
Depends on:
Blocks: 1215479
  Show dependency treegraph
 
Reported: 2015-12-21 08:41 PST by Jan de Mooij [:jandem]
Modified: 2016-10-06 04:54 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (27.67 KB, patch)
2015-12-21 08:41 PST, Jan de Mooij [:jandem]
nicolas.b.pierron: review+
Details | Diff | Splinter Review

Description User image Jan de Mooij [:jandem] 2015-12-21 08:41:56 PST
Created attachment 8700655 [details] [diff] [review]
Patch

We have an AutoWritableJitCode in Linker::newCode. Most callers have to update the JIT code after that, so they have to use another AutoWritableJitCode.

This patch adds a Maybe<AutoWritableJitCode> to the Linker class, so the code is writable as long as the Linker is on the stack. This allows us to remove some AutoWritableJitCode instances.

Ion IC stubs are even worse, we used AutoWritableJitCode for the new stub like 3 or 4 times. This patch refactors the code a bit so we finish the stub completely in linkCode, while it's still writable, and then we attach it.
Comment 1 User image Nicolas B. Pierron [:nbp] 2015-12-21 09:42:39 PST
Comment on attachment 8700655 [details] [diff] [review]
Patch

Review of attachment 8700655 [details] [diff] [review]:
-----------------------------------------------------------------

Nice work!

::: js/src/jit/Ion.cpp
@@ +747,5 @@
>  JitCompartment::toggleBarriers(bool enabled)
>  {
>      // Toggle barriers in compartment wide stubs that have patchable pre barriers.
>      if (regExpExecStub_)
> +        regExpExecStub_->togglePreBarriers(enabled, Reprotect);

Wouldn't it be better to use

  jrt->execAlloc().makeAllWritable();
  jrt->execAlloc().makeAllExecutable();

in this function?
Comment 3 User image Jan de Mooij [:jandem] 2015-12-22 02:19:53 PST
Keeping this bug open because makeAllWritable/makeExecutable are added in a patch I didn't land yet, so I still have to land that part. I pushed this patch to see how it affects AWFY.

(In reply to Nicolas B. Pierron [:nbp] from comment #1)
> Wouldn't it be better to use
> 
>   jrt->execAlloc().makeAllWritable();
>   jrt->execAlloc().makeAllExecutable();
> 
> in this function?

Good suggestion. I'm not sure, I think it depends on the number of Zones. It may indeed be more efficient to toggle all pages in the runtime, or we could use a separate ExecutableAllocator for each Zone... I'll try to do some measurements.

Also, makeAllWritable/makeAllExecutable are less secure, because an attacker could use web workers to write to JIT code while it's writable on the main thread. Still a lot safer than RWX.
Comment 4 User image Carsten Book [:Tomcat] 2015-12-23 03:05:22 PST
https://hg.mozilla.org/mozilla-central/rev/1fb5de3f44c3
Comment 5 User image Jon Coppeard (:jonco) 2016-10-05 02:19:45 PDT
Jan, did the second patch land yet?
Comment 6 User image Jan de Mooij [:jandem] 2016-10-06 04:54:03 PDT
(In reply to Jon Coppeard (:jonco) from comment #5)
> Jan, did the second patch land yet?

I think we can mark this FIXED and don't need the second patch.

Note You need to log in before you can comment on or make changes to this bug.