1322445 - Switch PageProtectingVector from protecting fully used pages to fully unused pages.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Emanuel Hoogeveen [:ehoogeveen]]

Analysis of recent crashes from bug 1124397 indicates that whatever is corrupting our buffers, it's always writing to the last page of the buffer. PageProtectingVector works by protecting pages that we're done writing to, and that's obviously never going to apply to the last page in a buffer.

To catch this behavior, instead we can try to protect *unused* pages. That should be cheaper as well, since we won't try to patch jumps in pages we aren't using.

Comment 1

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: Extend PageProtectingVector to protect unused pages as well.

This extends PageProtectingVector with the ability to protect unused pages as well, makes both kinds of protection optional, and removes all the AutoUnprotect gunk since it's no longer needed. Passes jit tests locally.

Comment 2

[Emanuel Hoogeveen [:ehoogeveen]]

Oh, and I also noticed several places where I was using capacity() and/or length() incorrectly. This doesn't affect our current use case since sizeof(T) == 1, but it was still wrong.

Comment 3

[Emanuel Hoogeveen [:ehoogeveen]]

I set the initial lower bound to 0 for testing, forgot to change that back. Fixed locally.

Comment 4

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: v1.1 - Extend PageProtectingVector to protect unused pages as well.

Fixed an issue that showed up in ASAN and Valgrind. Still waiting on try results, but I expect this to be green now (and I need to head off to bed).

Comment 5

[Emanuel Hoogeveen [:ehoogeveen]]

Yep:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=de0ade0f155e189df40c113943a6753f689222fb
https://treeherder.mozilla.org/#/jobs?repo=try&revision=55f3a47d1475235c9a04d1e2eb61a8fe76219c44

Comment 6

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: v1.2 - Extend PageProtectingVector to protect unused pages as well.

Managed to simplify things a bit more after waking up hideously early. I'm pretty happy with this now.

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=591a3d43e78a32560f28e9fc3f5eb3c4d26f9722

Comment 7

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: v1.3 - Extend PageProtectingVector to protect unused pages as well.

That crashed on ASAN again, and I spotted another bug while looking into that. I think this is correct now.

Comment 8

[Emanuel Hoogeveen [:ehoogeveen]]

Okay, all green now (ASAN Debug was the only build that showed the problem):
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a38802a7a56b94d004f09e4cddab2c05244a2b3e

Comment 9

[Jan de Mooij [:jandem]]

Wasn't the plan to switch it so we only protect the unused part?

Comment 10

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #9)
> Wasn't the plan to switch it so we only protect the unused part?

Nevermind, I missed comment 1.

Comment 11

[Emanuel Hoogeveen [:ehoogeveen]]

Sorry, yeah the patch switches to protecting only unused pages, but I didn't want to just completely change what PageProtectingVector can do. Instead I extended it to do both, and switched AssemblerBuffer to only use the unused page protection instead.

Comment 12

[Jan de Mooij [:jandem]]

Comment on attachment 8817446 [details] [diff] [review]
v1.3 - Extend PageProtectingVector to protect unused pages as well.

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

::: js/src/jit/x86-shared/AssemblerBuffer-x86-shared.h
@@ +157,5 @@
>          }
>  
> +        PageProtectingVector<unsigned char, 256, SystemAllocPolicy,
> +                             /* ProtectUsed = */ false, /* ProtectUnused = */ true,
> +                             /* InitialLowerBound = */ 32 * 1024> m_buffer;

Any thoughts on using a vanilla Vector here #ifdef RELEASE_OR_BETA, to minimize potential risk? I know we have some #ifdefs in PageProtectionVector to disable the protection, but there's still a lot of non-trivial code there that isn't inside #ifdefs and may add runtime overhead.

We may have to #ifdef enableProtection/disableProtection calls too but that seems fine.

Comment 13

[Emanuel Hoogeveen [:ehoogeveen]]

Thanks for the review!

(In reply to Jan de Mooij [:jandem] from comment #12) 
> Any thoughts on using a vanilla Vector here #ifdef RELEASE_OR_BETA, to
> minimize potential risk? I know we have some #ifdefs in PageProtectionVector
> to disable the protection, but there's still a lot of non-trivial code there
> that isn't inside #ifdefs and may add runtime overhead.
> 
> We may have to #ifdef enableProtection/disableProtection calls too but that
> seems fine.

I can do that - should be especially easy now that the AutoUnprotect stuff is gone.

Comment 14

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: v1.4 - Extend PageProtectingVector to protect unused pages as well.

Carrying forward r+ with that change. In exchange I removed the ifdefs from PageProtectingVector itself. I think that makes it less surprising if you want the protection it offers on beta and release as well (e.g. if performance isn't a concern). I did leave the ifdef in MemoryProtectionExceptionHandler - with any luck if someone needs PageProtectingVector on beta and release, the crash volume will stand out enough that the annotations aren't needed. If it turns out that they are, it's a one-line change to activate them.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6cec2e289658
Extend PageProtectingVector to protect unused pages as well. r=jandem

Comment 16

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/b38289113d37 for https://treeherder.mozilla.org/logviewer.html#?job_id=40612937&repo=mozilla-inbound

Comment 17

[Emanuel Hoogeveen [:ehoogeveen]]

Hmm, sorry about that. Odd, I thought I fixed those - let's see if Try reproduces.

Comment 18

[Emanuel Hoogeveen [:ehoogeveen]]

Attachment: v1.5 - Extend PageProtectingVector to protect unused pages as well.

Alright, I spotted the problem, and sm-asan is green now:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1eb5ba958543db0d4967a99903ea7beaa9fe48e8

(that build isn't listed as an option on Trychooser, annoyingly)

Comment 19

[Emanuel Hoogeveen [:ehoogeveen]]

Let's try this again. I'm not really sure how ASAN managed to trigger that particular edge case, but I'm glad it did.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/67f930fb5da8
Extend PageProtectingVector to protect unused pages as well. r=jandem

Comment 21

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/67f930fb5da8

Comment 22

[Hannes Verschore [:h4writer]]

Seemed to have caused a decrease in our slowdown due to less protections:

== Change summary for alert #4526 (as of December 15 2016 13:54 UTC) ==

Improvements:

  5%  ss 1.0.1 format-xparb linux64 opt shell                     15.56 -> 14.83
  4%  octane 2.0.1 Typescript linux64 opt shell                   22502.12 -> 23510.5
  4%  misc 0.8 bugs-652377-jslint-on-jslint linux64 opt shell     498.97 -> 477.87
  3%  kraken 1.1 beat-detection linux64 opt shell                 90.2 -> 87.23
  3%  octane 2.0.1 Gameboy linux64 opt shell                      49723.71 -> 51282.42
  3%  speedometer 1.0 summary osx-10-10 opt                       45.88 -> 47.12
  3%  kraken 1.1 crypto-sha256-iterative linux64 opt shell        45.26 -> 44.12
  2%  kraken 1.1 crypto-pbkdf2 linux64 opt shell                  113.05 -> 110.43
  2%  kraken 1.1 crypto-aes linux64 opt shell                     55.78 -> 54.53

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=4526

Comment 23

[Benjamin Bouvier [:bbouvier] (inactive)]

This also helped wasm baseline compilation of AngryBots a lot (34% faster), nice!