Closed
Bug 858060
Opened 12 years ago
Closed 11 years ago
Assertion failure: cur->isKind(PNK_FUNCTION), at frontend/NameFunctions.cpp:318
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
VERIFIED
FIXED
mozilla24
Tracking | Status | |
---|---|---|
firefox21 | --- | unaffected |
firefox22 | + | wontfix |
firefox23 | + | fixed |
firefox24 | + | fixed |
firefox-esr17 | --- | unaffected |
b2g18 | --- | unaffected |
People
(Reporter: decoder, Assigned: jorendorff)
Details
(5 keywords, Whiteboard: [jsbugmon:update,ignore][adv-main23+])
Attachments
(2 files, 4 obsolete files)
533 bytes,
text/plain
|
Details | |
2.96 KB,
patch
|
jorendorff
:
review+
lsblakk
:
approval-mozilla-aurora+
lsblakk
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
The following testcase asserts on mozilla-central revision c232bec6974d (no options required): g("module 'foo' {}"); function g(x) { eval(x, g.escaped); }
Reporter | ||
Updated•12 years ago
|
Whiteboard: [jsbugmon:update,bisect]
Reporter | ||
Updated•12 years ago
|
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
Reporter | ||
Comment 1•12 years ago
|
||
JSBugMon: Bisection requested, result: autoBisect shows this is probably related to the following changeset: The first bad revision is: changeset: 122483:ec8547a266b7 user: Eddy Bruel date: Wed Feb 20 20:49:41 2013 +0100 summary: Bug 568953 - Parser support for module declarations; r=jorendorff This iteration took 123.577 seconds to run.
Comment 3•12 years ago
|
||
(In reply to Christian Holler (:decoder) from comment #2) > Needinfo based on comment 1 :) Needstacktrace based on needinfo :-)
Flags: needinfo?(ejpbruel)
Comment 4•12 years ago
|
||
(In reply to Christian Holler (:decoder) from comment #2) > Needinfo based on comment 1 :) Looking at that test, my hunch is that the assertion is wrong. I'll look into it in a sec.
Comment 5•12 years ago
|
||
That assertion is wrong. PN_CODE used to be PN_FUNCTION. It has been changed because it can now refer to modules as well as functions, but we still have places in the code where we explicitly assert that the parse node kind is PNK_FUNCTION. I've attached a one-liner fix to remedy the problem.
Attachment #733412 -
Flags: review?(jorendorff)
Assignee | ||
Updated•12 years ago
|
Attachment #733412 -
Flags: review?(jorendorff) → review+
Reporter | ||
Comment 6•12 years ago
|
||
Marking s-s and sec-critical because the following test triggers the same assertion but trips a "use-after-poison" in AddressSanitizer builds (indicating a use-after-free involving one of the internal JS allocators): Math=1; module 'foo' {} The trace is this: ==2921== ERROR: AddressSanitizer: use-after-poison on address 0x60720000da8e at pc 0x8c75c7 bp 0x7fffccee5e70 sp 0x7fffccee5e68 READ of size 1 at 0x60720000da8e thread T0 #0 0x8c75c6 in js::frontend::FunctionBox::useAsmOrInsideUseAsm() const /srv/repos/mozilla-central/js/src/frontend/SharedContext.h:235 #1 0x887dda in js::frontend::CompileScript(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSScript*>, JS::CompileOptions const&, unsigned short const*, unsigned long, JSString*, unsigned int, js::SourceCompressionToken*) /srv/repos/mozilla-central/js/src/frontend/BytecodeCompiler.cpp:203 #2 0x476ae2 in JS::Compile(JSContext*, JS::Handle<JSObject*>, JS::CompileOptions, unsigned short const*, unsigned long) /srv/repos/mozilla-central/js/src/jsapi.cpp:5327 #3 0x476ee9 in JS::Compile(JSContext*, JS::Handle<JSObject*>, JS::CompileOptions, _IO_FILE*) /srv/repos/mozilla-central/js/src/jsapi.cpp:5354 #4 0x43843f in Process(JSContext*, JSObject*, char const*, bool) /srv/repos/mozilla-central/js/src/shell/js.cpp:464 #5 0x435f5c in ProcessArgs(JSContext*, JSObject*, js::cli::OptionParser*) /srv/repos/mozilla-central/js/src/shell/js.cpp:5111 #6 0x4371d1 in main /srv/repos/mozilla-central/js/src/shell/js.cpp:5385 #7 0x7f690fc2176c in __libc_start_main /build/buildd/eglibc-2.15/csu/libc-start.c:226 #8 0x434c84 in _start ??:0 0x60720000da8e is located 398 bytes inside of 4096-byte region [0x60720000d900,0x60720000e900) allocated by thread T0 here: #0 0x42a0b2 in malloc ??:0 #1 0x961ae2 in js_malloc(unsigned long) /srv/repos/mozilla-central/js/src/opt64asan/./dist/include/js/Utility.h:150
Group: core-security
Keywords: csec-uaf,
sec-critical
Comment 7•12 years ago
|
||
Does comment 6 have the same regression window as comment 1? Marking flags based on comment 1.
Assignee: general → ejpbruel
status-b2g18:
--- → unaffected
status-firefox21:
--- → unaffected
status-firefox22:
--- → affected
status-firefox23:
--- → affected
status-firefox-esr17:
--- → unaffected
tracking-firefox22:
--- → ?
tracking-firefox23:
--- → ?
Keywords: regression
Updated•12 years ago
|
Comment 8•11 years ago
|
||
Pinged Eddy to find out latest status.
![]() |
||
Comment 9•11 years ago
|
||
Perhaps this can land? (Feel free to set checkin-needed if this is ready)
Flags: needinfo?(ejpbruel)
Comment 10•11 years ago
|
||
I've updated the fix. Note that the bytecode emitter cannot handle parse nodes of kind PNK_MODULE yet, so we need to avoid an assertion there as well. This should fix the assertion errors, but I have no idea if it will also fix the memory issue from comment #6 (it shouldn't, afaict). I tried to confirm that for myself, but I need a Clang build with ASAN support to test it. The Clang version that ships with OS X doesn't have ASAN support yet, so I had to make a custom build. Unfortunately, that custom build causes build errors when trying to build SpiderMonkey with it. Decoder said he would look into my fix and see if he could confirm whether or not this fixes the problem. If not, we need to find some way for me to build SpiderMonkey with ASAN support.
Attachment #733412 -
Attachment is obsolete: true
Attachment #753568 -
Flags: review?(jorendorff)
Attachment #753568 -
Flags: feedback?(choller)
Flags: needinfo?(ejpbruel)
Reporter | ||
Comment 11•11 years ago
|
||
I just confirmed that the use-after-poison is *not* gone with the patch. I also tried a regular opt-build with Valgrind and I see this: ==24262== Invalid read of size 1 ==24262== at 0x82103CB: bool js::frontend::FoldConstants<js::frontend::FullParseHandler>(JSContext*, js::frontend::FullParseHandler::Node*, js::frontend::Parser<js::frontend::FullParseHandler>*, bool, bool) (SharedContext.h:235) ==24262== by 0x81FD985: js::frontend::CompileScript(JSContext*, JS::Handle<JSObject*>, JS::Handle<JSScript*>, JS::CompileOptions const&, unsigned short const*, unsigned int, JSString*, unsigned int, js::SourceCompressionToken*) (BytecodeCompiler.cpp:207) ==24262== by 0xFE93334B: ??? ==24262== Address 0x9a6442a is 266 bytes inside a block of size 4,096 alloc'd ==24262== at 0x4ADEE68: malloc (in /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so) ==24262== by 0x8247E09: js::LifoAlloc::getOrCreateChunk(unsigned int) (Utility.h:152) ==24262== by 0xFE9333FB: ??? Haven't tried a debug build yet with the fix but it's likely that you can reproduce the issue there with Valgrind.. Maybe that makes it more actionable for you right now?
Flags: needinfo?(ejpbruel)
Reporter | ||
Updated•11 years ago
|
Attachment #753568 -
Flags: feedback?(choller) → feedback-
Comment 12•11 years ago
|
||
(In reply to Christian Holler (:decoder) from comment #11) > I just confirmed that the use-after-poison is *not* gone with the patch. I > also tried a regular opt-build with Valgrind and I see this: > > > ==24262== Invalid read of size 1 > ==24262== at 0x82103CB: bool > js::frontend::FoldConstants<js::frontend::FullParseHandler>(JSContext*, > js::frontend::FullParseHandler::Node*, > js::frontend::Parser<js::frontend::FullParseHandler>*, bool, bool) > (SharedContext.h:235) > ==24262== by 0x81FD985: js::frontend::CompileScript(JSContext*, > JS::Handle<JSObject*>, JS::Handle<JSScript*>, JS::CompileOptions const&, > unsigned short const*, unsigned int, JSString*, unsigned int, > js::SourceCompressionToken*) (BytecodeCompiler.cpp:207) > ==24262== by 0xFE93334B: ??? > ==24262== Address 0x9a6442a is 266 bytes inside a block of size 4,096 > alloc'd > ==24262== at 0x4ADEE68: malloc (in > /usr/lib/valgrind/vgpreload_memcheck-x86-linux.so) > ==24262== by 0x8247E09: js::LifoAlloc::getOrCreateChunk(unsigned int) > (Utility.h:152) > ==24262== by 0xFE9333FB: ??? > > > Haven't tried a debug build yet with the fix but it's likely that you can > reproduce the issue there with Valgrind.. Maybe that makes it more > actionable for you right now? Are you sure this particular regression was introduced by the same changeset as the one in comment #1? I have a hunch that the problem was introduced by a later changeset, extending one subclass of SharedContext with a field, but not the other.
Flags: needinfo?(ejpbruel)
Reporter | ||
Comment 13•11 years ago
|
||
(In reply to Eddy Bruel [:ejpbruel] from comment #12) > Are you sure this particular regression was introduced by the same changeset > as the one in comment #1? No, I am not. The only reason why I listed this here is that the second test triggered this assertion in a debug build and in an opt build the invalid read/use-after-poison so I assumed they are connected, especially because the test is so small. If this issue shouldn't be fixed in this bug, then we should clone the bug and fix the problem there.
Comment 14•11 years ago
|
||
Oh, I think I found the issue! There is another place in frontend/FoldConstants.cpp:267 where we assume that if a parse node has kind PN_CODE, it must be a function, which is no longer true. I don't have time to write up a patch right now, but I can do so later today. I'm still operating blind however, so I'd need decoder to double check if this fixes the issue.
Reporter | ||
Comment 15•11 years ago
|
||
![]() |
||
Updated•11 years ago
|
status-firefox24:
--- → affected
Flags: needinfo?(ejpbruel)
![]() |
||
Updated•11 years ago
|
tracking-firefox24:
--- → ?
Updated•11 years ago
|
Comment 16•11 years ago
|
||
Attachment #753568 -
Attachment is obsolete: true
Attachment #753568 -
Flags: review?(jorendorff)
Attachment #756875 -
Flags: review?(jorendorff)
Flags: needinfo?(ejpbruel)
Comment 17•11 years ago
|
||
Slightly nicer fix. Hopefully, this catches all the places where we need to check if a code node is a function or a module now.
Attachment #756875 -
Attachment is obsolete: true
Attachment #756875 -
Flags: review?(jorendorff)
Attachment #756876 -
Flags: review?(jorendorff)
Assignee | ||
Comment 18•11 years ago
|
||
Comment on attachment 756876 [details] [diff] [review] Updated fix Review of attachment 756876 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/frontend/BytecodeEmitter.cpp @@ +6024,5 @@ > JS_ASSERT(pn->getArity() == PN_NULLARY); > break; > > + case PNK_MODULE: > + return true; // TODO: Add emitter support for modules This has to report a SyntaxError or something, right? It can't just return true; we would allow module syntax but ignore it.
Attachment #756876 -
Flags: review?(jorendorff) → review+
Comment 19•11 years ago
|
||
Fair enough on the syntax error. Here is a new patch with that comment addressed. Decoder, could you please confirm that this patch fixes the problem? It looks fine on my machine, but I'd like to be absolutely sure. Jorendorff, I'll be on PTO for the next 3 weeks, so could you make sure that this gets landed, or hand it off to someone else if you are too busy? I've also informed my manager about this bug, just in case.
Attachment #756876 -
Attachment is obsolete: true
Attachment #757053 -
Flags: review?(jorendorff)
Assignee | ||
Updated•11 years ago
|
Attachment #757053 -
Flags: review?(jorendorff) → review+
Comment 20•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2bd3d9bbd722
Status: NEW → RESOLVED
Closed: 11 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Reporter | ||
Updated•11 years ago
|
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]
Reporter | ||
Comment 21•11 years ago
|
||
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 6f32011a27ef).
Reporter | ||
Updated•11 years ago
|
Status: RESOLVED → VERIFIED
Reporter | ||
Comment 22•11 years ago
|
||
JSBugMon: This bug has been automatically verified fixed.
Comment 23•11 years ago
|
||
Jorendorff was goiong to look into the branch uplifts here.
Assignee: ejpbruel → jorendorff
Comment 24•11 years ago
|
||
This is a wontfix for FF22 at this point - sorry, but this didn't make it into our final beta.
Comment 25•11 years ago
|
||
Emailed jorendorff to ask about an uplift nom for FF23
Flags: needinfo?(jorendorff)
Assignee | ||
Comment 26•11 years ago
|
||
Comment on attachment 757053 [details] [diff] [review] Updated fix [Approval Request Comment] Bug caused by (feature/regressing bug #): bug 568953 User impact if declined: likely remote execution vulnerability Testing completed (on m-c, etc.): has baked on m-c for a few weeks now Risk to taking this patch (and alternatives if risky): minimal String or IDL/UUID changes made by this patch: none
Attachment #757053 -
Flags: approval-mozilla-beta?
Attachment #757053 -
Flags: approval-mozilla-aurora?
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(jorendorff)
Updated•11 years ago
|
Attachment #757053 -
Flags: approval-mozilla-beta?
Attachment #757053 -
Flags: approval-mozilla-beta+
Attachment #757053 -
Flags: approval-mozilla-aurora?
Attachment #757053 -
Flags: approval-mozilla-aurora+
Comment 27•11 years ago
|
||
This landed on m-c during the Fx24 cycle. https://hg.mozilla.org/releases/mozilla-beta/rev/64823c2294bf
Updated•11 years ago
|
Whiteboard: [jsbugmon:update,ignore] → [jsbugmon:update,ignore][adv-main23+]
Updated•10 years ago
|
Group: core-security
You need to log in
before you can comment on or make changes to this bug.
Description
•