Closed
Bug 1446811
Opened 6 years ago
Closed 6 years ago
Crash in js::gc::StoreBuffer::putSlot
Categories
(Core :: JavaScript Engine, defect, P1)
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox59 | --- | unaffected |
firefox60 | + | fixed |
firefox61 | + | fixed |
People
(Reporter: axel, Assigned: jonco)
References
Details
(Keywords: crash, sec-high)
Crash Data
Attachments
(2 files)
18.05 KB,
patch
|
jorendorff
:
review+
abillings
:
sec-approval+
|
Details | Diff | Splinter Review |
18.18 KB,
patch
|
jcristau
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
This bug was filed from the Socorro interface and is report bp-940927c6-1fc7-43e1-a7a3-0a5fe0180318. ============================================================= Top 10 frames of crashing thread: 0 XUL js::gc::StoreBuffer::putSlot js/src/gc/StoreBuffer.h:308 1 XUL js::ExportEntryObject::create js/src/gc/Barrier.h:723 2 XUL js::ModuleBuilder::processExport js/src/builtin/ModuleObject.cpp:1473 3 XUL js::frontend::GeneralParser<js::frontend::FullParseHandler, char16_t>::exportLexicalDeclaration js/src/frontend/Parser.cpp:5453 4 XUL js::frontend::GeneralParser<js::frontend::FullParseHandler, char16_t>::exportDeclaration js/src/frontend/Parser.cpp 5 XUL js::frontend::GeneralParser<js::frontend::FullParseHandler, char16_t>::statementListItem js/src/frontend/Parser.cpp:7759 6 XUL js::frontend::GeneralParser<js::frontend::FullParseHandler, char16_t>::statementList js/src/frontend/Parser.cpp:4228 7 XUL js::frontend::Parser<js::frontend::FullParseHandler, char16_t>::moduleBody js/src/frontend/Parser.cpp:2269 8 XUL BytecodeCompiler::compileModule js/src/frontend/BytecodeCompiler.cpp:408 9 XUL js::frontend::CompileModule js/src/frontend/BytecodeCompiler.cpp:606 ============================================================= The line that triggers this crash seems to be export const [THREAD_NEW, THREAD_MUTED, THREAD_FAVORITE, THREAD_DELETED] = [0, 1, 2, 4]; in a web extension, with <script type="module">
Comment 1•6 years ago
|
||
OK wow, it looks like ExportEntryObject::create can allocate a nursery object on a helper thread and then trigger a StoreBuffer add from that thread?
Group: javascript-core-security
Updated•6 years ago
|
Flags: needinfo?(jcoppeard)
Comment 2•6 years ago
|
||
This could explain some of the StoreBuffer crashes we've seen lately.
Reporter | ||
Comment 3•6 years ago
|
||
FYI, `let` vs `const` didn't make a difference. When I did the `const` without the `export`, and put an `export {..}` in a separate statement, things works.
Comment 4•6 years ago
|
||
So if I run this in the shell: offThreadCompileModule("export const [a, b] = [1, 2];"); I get: Assertion failure: binding->isKind(ParseNodeKind::Name), at Parser.cpp:5358 So I wonder if there's a separate issue going on.
Comment 5•6 years ago
|
||
One thing I noticed yesterday is that JSContext::nurserySuppressions_ is not initialized by the JSContext constructor. Not sure if the ProtectedData template is initializing it.
Assignee | ||
Comment 6•6 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #1) > it looks like ExportEntryObject::create can allocate a nursery object on a helper thread Ouch, that's not supposed to happen. We check for helper threads in allocation... I'm not sure I trust this stack. > Not sure if the ProtectedData template is initializing it. ProtectedData initialises |value| with forwarded arguments. In this case there are no arguments so I think this this falls under 'value initialisation' and the member will be set to zero. JScontext::setHelperThread() will then increment it. Probably destructuring assignment isn't properly supported by export.
Assignee: nobody → jcoppeard
Flags: needinfo?(jcoppeard)
Comment 7•6 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #4) > Assertion failure: binding->isKind(ParseNodeKind::Name), at Parser.cpp:5358 > > So I wonder if there's a separate issue going on. This major bug has to be fixed first. In release mode we go straight into a union and access the wrong fork. UB. jonco, if you don't have time for a parser bug, just assign this to me.
Updated•6 years ago
|
Priority: -- → P1
Assignee | ||
Comment 8•6 years ago
|
||
Thanks. I already started looking at this though. Here's a patch to recursively handle array and object destructuring patterns in exported declarations. I guess I missed this case originally. I cargo culted the code to traverse the parser data structures so please check I covered everything.
Attachment #8960685 -
Flags: review?(jorendorff)
Comment 9•6 years ago
|
||
Comment on attachment 8960685 [details] [diff] [review] bug1446811-export-binding-patterns Review of attachment 8960685 [details] [diff] [review]: ----------------------------------------------------------------- This is great. There is probably something obviously broken with this idea in practice, but it seems like all of the checkExportedNames machinery could be moved down to PerParserHandler... Not saying you should do it. The only benefit is code size, and that kind of change carries a high risk of being wasted time...
Attachment #8960685 -
Flags: review?(jorendorff) → review+
Assignee | ||
Comment 10•6 years ago
|
||
(In reply to Jason Orendorff [:jorendorff] from comment #9) > it seems like all of the checkExportedNames machinery could be moved down to > PerParserHandler... That would be nice. I had a go at this but couldn't see how to access parse nodes since these are a different type between full parsing / syntax parsing.
Assignee | ||
Comment 11•6 years ago
|
||
Comment on attachment 8960685 [details] [diff] [review] bug1446811-export-binding-patterns [Security approval request comment] How easily could an exploit be constructed based on the patch? Difficult. Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem? No. Which older supported branches are affected by this flaw? FF 60 is affected. If not all supported branches, which bug introduced the flaw? This was exposed by bug 1438139. Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be? Yes, the same patch should apply cleanly. How likely is this patch to cause regressions; how much testing does it need? Unlikely.
Attachment #8960685 -
Flags: sec-approval?
Comment 12•6 years ago
|
||
(In reply to Jon Coppeard (:jonco) from comment #10) > (In reply to Jason Orendorff [:jorendorff] from comment #9) > > it seems like all of the checkExportedNames machinery could be moved down to > > PerParserHandler... > > I had a go at this but couldn't see how to access parse > nodes since these are a different type between full parsing / syntax parsing. This wouldn't be a problem -- you could use PerHandlerParser::Node. Other PHP functions use Node this way. The real problem is all this checkExportedGoo stuff bottoms out in checkExportedName, and that function calls GeneralParser::error which demands CharT knowledge (in order to get a line of context for the error). So in the end this couldn't be moved that far up the hierarchy. Last I looked at the hierarchy stuff, all the checkExported stuff could go into Parser<FullParseHandler, CharT> (with none in GeneralParser) -- as long as GeneralParser<ParseHandler, CharT>::exportDeclaration() was an abort-if-syntax-parser, then asFinalParser()->exportDeclaration() so that export-decl parsing were wholly in Parser<FullParseHandler, CharT>. But it wasn't worth the time *to me* to digress to fix it.
Comment 13•6 years ago
|
||
Comment on attachment 8960685 [details] [diff] [review] bug1446811-export-binding-patterns sec-approval+ for trunk. We'll want a branch patch for 60 as well. 59 is unaffected, right?
Attachment #8960685 -
Flags: sec-approval? → sec-approval+
Updated•6 years ago
|
status-firefox59:
--- → unaffected
status-firefox60:
--- → affected
status-firefox61:
--- → affected
status-firefox-esr52:
--- → unaffected
tracking-firefox60:
--- → +
tracking-firefox61:
--- → +
Comment 14•6 years ago
|
||
This is possibly the same as bug 1444495, can someone confirm?
Assignee | ||
Comment 15•6 years ago
|
||
(In reply to Alex Gaynor [:Alex_Gaynor] from comment #14) > This is possibly the same as bug 1444495, can someone confirm? No, it's not the same.
Assignee | ||
Comment 16•6 years ago
|
||
(In reply to Al Billings [:abillings] from comment #13) > 59 is unaffected, right? This functionality is preffed off by default in 59 but can be enabled.
Comment 17•6 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/54ae81245f46
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Assignee | ||
Comment 18•6 years ago
|
||
Approval Request Comment [Feature/Bug causing the regression]: Bug 1438139. [User impact if declined]: Possible crash / security vulnerability. [Is this code covered by automated tests?]: Yes. [Has the fix been verified in Nightly?]: Yes. [Needs manual test from QE? If yes, steps to reproduce]: No. [List of other uplifts needed for the feature/fix]: None. [Is the change risky?]: No. [Why is the change risky/not risky?]: The patch adds code to cover parser AST cases that were not implemented originally. The code is straightforward. The code is covered by tests. [String changes made/needed]: None
Attachment #8962285 -
Flags: approval-mozilla-beta?
Comment 19•6 years ago
|
||
Comment on attachment 8962285 [details] [diff] [review] bug1446811-beta sec-high js fix for beta60
Attachment #8962285 -
Flags: approval-mozilla-beta? → approval-mozilla-beta+
Assignee | ||
Comment 20•6 years ago
|
||
uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/ef1614cd592b831c0f1116270a7ad4934017d645
Updated•6 years ago
|
Updated•6 years ago
|
Group: javascript-core-security → core-security-release
Updated•6 years ago
|
Group: core-security-release
You need to log in
before you can comment on or make changes to this bug.
Description
•