Closed
Bug 1267557
Opened 8 years ago
Closed 8 years ago
Some code poisoning changes
Categories
(Core :: JavaScript Engine: JIT, defect)
Core
JavaScript Engine: JIT
Tracking
()
RESOLVED
FIXED
mozilla49
People
(Reporter: jandem, Assigned: jandem)
References
Details
Attachments
(4 files)
1.17 KB,
patch
|
nbp
:
review+
ritu
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
2.49 KB,
patch
|
nbp
:
review+
ritu
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
725 bytes,
patch
|
nbp
:
review+
ritu
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
3.38 KB,
patch
|
jonco
:
review+
ritu
:
approval-mozilla-aurora+
ritu
:
approval-mozilla-beta+
ritu
:
approval-mozilla-esr45+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•8 years ago
|
||
When we allocate executable code from a pool, there may be a few alignment bytes before the actual code. This patch poisons those "headerSize" bytes too. I noticed this in a few crash reports.
Attachment #8745260 -
Flags: review?(nicolas.b.pierron)
Assignee | ||
Comment 2•8 years ago
|
||
This patch also removes the unused JS_SWEPT_FRAME_PATTERN constant. Not sure which value to use on MIPS or MIPS64. Most odd values I tried are valid MIPS instructions, so I'll let the MIPS gurus find one.
Attachment #8745262 -
Flags: review?(nicolas.b.pierron)
Assignee | ||
Comment 3•8 years ago
|
||
Comment on attachment 8745262 [details] [diff] [review] Part 2 - Change poison values Review of attachment 8745262 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/public/Utility.h @@ +49,5 @@ > #define JS_ALLOCATED_TENURED_PATTERN 0x4D > #define JS_EMPTY_STOREBUFFER_PATTERN 0x1B > + > +#if defined(JS_CODEGEN_X86) || defined(JS_CODEGEN_X64) || defined(JS_CODEGEN_NONE) > +# define JS_SWEPT_CODE_PATTERN 0xED // IN (%dx),%eax LLDB disassembles this as: IN %dx, %eax I'll change this to something like: // IN instruction, will crash.
Updated•8 years ago
|
Attachment #8745260 -
Flags: review?(nicolas.b.pierron) → review+
Comment 4•8 years ago
|
||
Comment on attachment 8745262 [details] [diff] [review] Part 2 - Change poison values Review of attachment 8745262 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/public/Utility.h @@ +53,5 @@ > +# define JS_SWEPT_CODE_PATTERN 0xED // IN (%dx),%eax > +#elif defined(JS_CODEGEN_ARM) || defined(JS_CODEGEN_ARM64) > +# define JS_SWEPT_CODE_PATTERN 0xA3 // undefined instruction > +#else > +# error "JS_SWEPT_CODE_PATTERN not defined for this platform" nit: Add a comment above this to mention that we are looking for a pattern which, when repeated to fill an instruction is either an illegal instruction (in user mode), or not the code of any valid instruction.
Attachment #8745262 -
Flags: review?(nicolas.b.pierron) → review+
Comment 5•8 years ago
|
||
See part 2, which is adding a poison value in order to avoid instruction slides.
Flags: needinfo?(r)
Flags: needinfo?(branislav.rankov)
Comment 6•8 years ago
|
||
(In reply to Nicolas B. Pierron [:nbp] from comment #5) > See part 2, which is adding a poison value in order to avoid instruction > slides. I suggest define JS_SWEPT_CODE_PATTERN = 0x01 for mips, this instruction encoding '0x01010101' is invalid on all mips ISAs I know (mips1/mips2/mips3/mips4/mips5/mips32r1/mips32r2/mips32r3/mips32r5/mips32r6/mips64r1/mips64r2/mips64r3/mips64r5 and mips64r6).
Flags: needinfo?(r)
Comment 7•8 years ago
|
||
Attachment #8745846 -
Flags: review?(nicolas.b.pierron)
Comment 8•8 years ago
|
||
Comment on attachment 8745846 [details] [diff] [review] Part 3 - Change poison values for MIPS Review of attachment 8745846 [details] [diff] [review]: ----------------------------------------------------------------- Thanks :)
Attachment #8745846 -
Flags: review?(nicolas.b.pierron) → review+
Assignee | ||
Comment 9•8 years ago
|
||
These patches fail to compile the browser, because the JS_CODEGEN_* constants are not defined when we #include js/public/Utility.h in code outside SpiderMonkey. This patch moves the poison values defines to jsutil.h. I also removed some unused values in the process, and added a missing value to IsThingPoisoned.
Attachment #8746425 -
Flags: review?(jcoppeard)
Comment 10•8 years ago
|
||
Comment on attachment 8746425 [details] [diff] [review] Part 0 - Move poison values Review of attachment 8746425 [details] [diff] [review]: ----------------------------------------------------------------- They're not used outside the engine so this seems fine. ::: js/src/gc/Marking.cpp @@ +125,5 @@ > JS_FRESH_NURSERY_PATTERN, > JS_SWEPT_NURSERY_PATTERN, > JS_ALLOCATED_NURSERY_PATTERN, > JS_FRESH_TENURED_PATTERN, > + JS_MOVED_TENURED_PATTERN, Thanks for updating this.
Attachment #8746425 -
Flags: review?(jcoppeard) → review+
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(branislav.rankov)
Comment 11•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/bccf6d5c4c05 https://hg.mozilla.org/integration/mozilla-inbound/rev/1e9d2cc7c629 https://hg.mozilla.org/integration/mozilla-inbound/rev/16391def96e4 https://hg.mozilla.org/integration/mozilla-inbound/rev/9f792a06e6ae
Comment 12•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/bccf6d5c4c05 https://hg.mozilla.org/mozilla-central/rev/1e9d2cc7c629 https://hg.mozilla.org/mozilla-central/rev/16391def96e4 https://hg.mozilla.org/mozilla-central/rev/9f792a06e6ae
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox49:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Assignee | ||
Comment 13•8 years ago
|
||
Comment on attachment 8746425 [details] [diff] [review] Part 0 - Move poison values Requesting approval for all 4 patches. These patches change our JIT code poisoning to be safer and to make it easier to diagnose crashes. It ensures we always crash when we attempt to execute poisoned code. Approval Request Comment [Feature/regressing bug #]: - [User impact if declined]: Patches help diagnose crashes and make it harder to exploit potential security issues. [Describe test coverage new/current, TreeHerder]: On m-c for about a week without problems. [Risks and why]: Low risk. [String/UUID change made/needed]: None.
Attachment #8746425 -
Flags: approval-mozilla-esr45?
Attachment #8746425 -
Flags: approval-mozilla-beta?
Attachment #8746425 -
Flags: approval-mozilla-aurora?
status-firefox47:
--- → affected
status-firefox48:
--- → affected
status-firefox-esr45:
--- → affected
Hello Jan, have we looked at data from Nightly (a few sample crash reports where this applies) to validate that this patch is indeed helping improve root causing some class of crashes? I was wondering if we should wait for that kind of validation before uplifting this to Beta47. What do you think?
Flags: needinfo?(jdemooij)
Comment on attachment 8746425 [details] [diff] [review] Part 0 - Move poison values Let's uplift to Aurora48 and stabilize before uplifting to Beta47.
Attachment #8746425 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8745260 -
Flags: approval-mozilla-aurora+
Attachment #8745262 -
Flags: approval-mozilla-aurora+
Attachment #8745846 -
Flags: approval-mozilla-aurora+
Comment 16•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/e122f4f0fa72 https://hg.mozilla.org/releases/mozilla-aurora/rev/29d746b57ed2 https://hg.mozilla.org/releases/mozilla-aurora/rev/2fa3f71011cc https://hg.mozilla.org/releases/mozilla-aurora/rev/fa1e3821e0e4
Assignee | ||
Comment 17•8 years ago
|
||
(In reply to Ritu Kothari (:ritu) from comment #14) > Hello Jan, have we looked at data from Nightly (a few sample crash reports > where this applies) to validate that this patch is indeed helping improve > root causing some class of crashes? I was wondering if we should wait for > that kind of validation before uplifting this to Beta47. What do you think? We are not seeing a lot of those crashes on Nightly so it's hard to say, but here's one where we now crash while executing the new poison pattern: bp-cd18691b-5153-43cb-b9b9-736232160506. Before, we might have continued executing the code after that and the report would have been much more confusing to categorize. I think this is worth uplifting, as it's low risk (on m-c for at least 10 days) and it turns "might crash when something bad happens" into "will definitely crash when something bad happens", that's a nice security improvement.
Flags: needinfo?(jdemooij)
(In reply to Jan de Mooij [:jandem] from comment #17) > (In reply to Ritu Kothari (:ritu) from comment #14) > > Hello Jan, have we looked at data from Nightly (a few sample crash reports > > where this applies) to validate that this patch is indeed helping improve > > root causing some class of crashes? I was wondering if we should wait for > > that kind of validation before uplifting this to Beta47. What do you think? > > We are not seeing a lot of those crashes on Nightly so it's hard to say, but > here's one where we now crash while executing the new poison pattern: > bp-cd18691b-5153-43cb-b9b9-736232160506. Before, we might have continued > executing the code after that and the report would have been much more > confusing to categorize. > > I think this is worth uplifting, as it's low risk (on m-c for at least 10 > days) and it turns "might crash when something bad happens" into "will > definitely crash when something bad happens", that's a nice security > improvement. Thank you for your due diligence. I am glad to hear that there is at least some data that this is helping. I will approve uplift to Beta.
Comment on attachment 8746425 [details] [diff] [review] Part 0 - Move poison values This has baken in Aurora for a few days, Jan has confirmed in comment 18 that this code is helping crash diagnostics, ESR45+, Beta47+
Attachment #8746425 -
Flags: approval-mozilla-esr45?
Attachment #8746425 -
Flags: approval-mozilla-esr45+
Attachment #8746425 -
Flags: approval-mozilla-beta?
Attachment #8746425 -
Flags: approval-mozilla-beta+
Part 1 doesn't appear to apply cleanly to esr45.
Flags: needinfo?(jdemooij)
Comment 21•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/507d849ea8f4 https://hg.mozilla.org/releases/mozilla-beta/rev/6a5cce634618 https://hg.mozilla.org/releases/mozilla-beta/rev/686cf3f6d74a https://hg.mozilla.org/releases/mozilla-beta/rev/cb18fea52de4
Assignee | ||
Comment 22•8 years ago
|
||
https://hg.mozilla.org/releases/mozilla-esr45/rev/d692c9984963 https://hg.mozilla.org/releases/mozilla-esr45/rev/47887256a413 https://hg.mozilla.org/releases/mozilla-esr45/rev/8401c10ae0a5 https://hg.mozilla.org/releases/mozilla-esr45/rev/c48f83805d40
Flags: needinfo?(jdemooij)
You need to log in
before you can comment on or make changes to this bug.
Description
•