Don't reprotect more than once when linking code




3 years ago
3 years ago


(Reporter: jandem, Assigned: jandem)


Firefox Tracking Flags

(Not tracked)



(1 attachment)



3 years ago
Posted patch PatchSplinter Review
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.
Attachment #8700655 - Flags: review?(nicolas.b.pierron)
Comment on attachment 8700655 [details] [diff] [review]

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


in this function?
Attachment #8700655 - Flags: review?(nicolas.b.pierron) → review+

Comment 3

3 years ago
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.
Keywords: leave-open
Jan, did the second patch land yet?
Flags: needinfo?(jdemooij)
Priority: -- → P5

Comment 6

3 years ago
(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.
Last Resolved: 3 years ago
Flags: needinfo?(jdemooij)
Resolution: --- → FIXED


3 years ago
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.