Closed Bug 977810 Opened 6 years ago Closed 6 years ago

Poison Baseline/Ion code in release builds

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: cpeterson, Assigned: cpeterson)

References

Details

(Keywords: sec-want, Whiteboard: [js:p1])

Attachments

(1 file, 1 obsolete file)

Ion code is only poisoned in debug builds. Consider poisoning in release builds or #ifdef JS_CRASH_DIAGNOSTICS (Nightly and Aurora).

https://hg.mozilla.org/mozilla-central/file/4cfb6c61b137/js/src/jit/Ion.cpp#l676
Note that JS_POISON is already #ifdef JS_CRASH_DIAGNOSTICS-only, so we shouldn't need any special checking for nightly/aurora.
Attached patch poison-ion-code.patch (obsolete) — Splinter Review
Poison freed Ion code in nightly builds.

As of bug 860254, jemalloc now poisons freed memory in both debug and release builds. bsmedberg suggested [1] poisoning freed JIT code, too. I replaced with #ifdef DEBUG with #ifdef JS_CRASH_DIAGNOSTICS, so it only affects Nightly and Aurora release builds. How much would this poison affect JS performance if it was enabled in release builds on all channels?

[1] https://groups.google.com/forum/#!topic/mozilla.dev.tech.js-engine.internals/dUlUxYsFz5Q
Assignee: nobody → cpeterson
Status: NEW → ASSIGNED
Attachment #8388161 - Flags: review?(jdemooij)
Also null out JitCode's code_ and pool_ pointers.
Attachment #8388161 - Attachment is obsolete: true
Attachment #8388161 - Flags: review?(jdemooij)
Attachment #8388162 - Flags: review?(jdemooij)
Comment on attachment 8388162 [details] [diff] [review]
poison-ion-code.patch

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

Thanks for doing this!

::: js/src/jit/Ion.cpp
@@ +690,5 @@
> +        // Horrible hack: if we are using perf integration, we don't
> +        // want to reuse code addresses, so we just leak the memory instead.
> +        if (!PerfEnabled()) {
> +            pool_->release();
> +        }

Nit: no {}
Attachment #8388162 - Flags: review?(jdemooij) → review+
(In reply to Chris Peterson (:cpeterson) from comment #2)
> How much would this poison affect
> JS performance if it was enabled in release builds on all channels?

I just tested this on Octane in the shell, and I don't see a slowdown if I enable the poisoning in an opt build. So I'm fine with removing "#ifdef JS_CRASH_DIAGNOSTICS", as long as you keep an eye on AWFY/Talos after it lands.
(In reply to Jan de Mooij [:jandem] from comment #5)
> I just tested this on Octane in the shell, and I don't see a slowdown if I
> enable the poisoning in an opt build. So I'm fine with removing "#ifdef
> JS_CRASH_DIAGNOSTICS", as long as you keep an eye on AWFY/Talos after it
> lands.

JS_POISON itself is #ifdef JS_CRASH_DIAGNOSTICS so that JS_POISON call would need to be replaced with a memset().
(In reply to Chris Peterson (:cpeterson) from comment #6)
> JS_POISON itself is #ifdef JS_CRASH_DIAGNOSTICS so that JS_POISON call would
> need to be replaced with a memset().

Ah, indeed. I have to run, but I just tried again with a memset there and perf still looks fine in the shell.
Or we could (should!) define the real JS_POISON in all builds... can we try that?
Blocks: 981991
I filed a new bug 981991 to continue the investigation of defining JS_POISON in release builds.
https://hg.mozilla.org/mozilla-central/rev/9984a46fc938
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Summary: Poison Ion code in release builds → Poison Baseline/Ion code in release builds
You need to log in before you can comment on or make changes to this bug.