1234246 - Don't reprotect more than once when linking code

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect, P5)

Description

[Jan de Mooij [:jandem]]

Attachment: 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

[Nicolas B. Pierron [:nbp]]

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 2

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1fb5de3f44c3

Comment 3

[Jan de Mooij [:jandem]]

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

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/1fb5de3f44c3

Comment 5

[Jon Coppeard (:jonco)]

Jan, did the second patch land yet?

Comment 6

[Jan de Mooij [:jandem]]

(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.