Closed Bug 1329499 Opened 5 years ago Closed 5 years ago

Extend PageProtectingVector to detect poison patterns

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: ehoogeveen, Assigned: ehoogeveen)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 3 obsolete files)

Bug 1329473 seems to show that the current protections aren't helping either. This is simpler than my other idea, so let's try this first.
The number of template arguments is getting a little out of control, but I'd rather not remove any of the other functionality. This addition is pretty simple though.

Performance is surprisingly good: on the WASM baseline benchmark I've been using, I see 3-4% slowdown with WASM's runtime killswitch and 8-9% slowdown with the killswitch disabled.
Attachment #8824786 - Flags: review?(jdemooij)
Oh, and I left the reentrancy guard enabled because.. well, it has only been a few days, so even if it's a long shot I'd like to give it a little more time.
Here's a patch to protect against shenanigans during realloc. It's fairly simple, but I don't know if it's worth doing (given that we already tried instrumenting jemalloc itself).
Attachment #8825012 - Flags: feedback?(jdemooij)
Comment on attachment 8824786 [details] [diff] [review]
Detect the jemalloc freed memory poison pattern during append calls.

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

I'd be really surprised if the append() calls were doing the poisoning, but can't hurt to check this for a while.

::: js/src/ds/PageProtectingVector.h
@@ +36,5 @@
>           bool ProtectUnused = true,
>           bool GuardAgainstReentrancy = true,
> +         bool DetectPoison = false,
> +         size_t InitialLowerBound = 0,
> +         uint8_t PoisonPattern = 0x5e>

s/0x5e/0xe5/ no?
Attachment #8824786 - Flags: review?(jdemooij) → review+
Thanks!

(In reply to Jan de Mooij [:jandem] from comment #4)
> > +         uint8_t PoisonPattern = 0x5e>
> 
> s/0x5e/0xe5/ no?

Err, yes. Good catch!
Attachment #8824786 - Attachment is obsolete: true
Attachment #8825038 - Flags: review+
Comment on attachment 8825012 [details] [diff] [review]
Part 2: Keep outside actors from messing with our old buffer during realloc.

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

Same here, we can try this for a few weeks and back out if it doesn't change anything.
Attachment #8825012 - Flags: feedback?(jdemooij) → review+
Great, let's do it. Performance with this patch was a bit worse than I expected, so I added a test to avoid the mprotects for oldSize < 32k. Also added a missing include that showed up on Try.

New try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0230e9e326669f6378fef3b03b1f018f1b1894f7
Attachment #8825012 - Attachment is obsolete: true
Attachment #8825078 - Flags: review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9e9e07a5afb
Part 1: Detect the jemalloc freed memory poison pattern during append calls. r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/c89eb360f419
Part 2: Keep outside actors from messing with our old buffer during realloc. r=jandem
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/e9e9e07a5afb
https://hg.mozilla.org/mozilla-central/rev/c89eb360f419
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.