Closed Bug 858060 Opened 7 years ago Closed 7 years ago

Assertion failure: cur->isKind(PNK_FUNCTION), at frontend/NameFunctions.cpp:318

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla24
Tracking Status
firefox21 --- unaffected
firefox22 + wontfix
firefox23 + fixed
firefox24 + fixed
firefox-esr17 --- unaffected
b2g18 --- unaffected

People

(Reporter: decoder, Assigned: jorendorff)

References

(Blocks 1 open bug)

Details

(5 keywords, Whiteboard: [jsbugmon:update,ignore][adv-main23+])

Attachments

(2 files, 4 obsolete files)

The following testcase asserts on mozilla-central revision c232bec6974d (no options required):


g("module 'foo' {}");
function g(x) {
  eval(x, g.escaped);
}
Whiteboard: [jsbugmon:update,bisect]
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
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.
Needinfo based on comment 1 :)
Flags: needinfo?(ejpbruel)
(In reply to Christian Holler (:decoder) from comment #2)
> Needinfo based on comment 1 :)

Needstacktrace based on needinfo :-)
Flags: needinfo?(ejpbruel)
(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.
Attached patch Quick fix (obsolete) — Splinter Review
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)
Attachment #733412 - Flags: review?(jorendorff) → review+
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
Does comment 6 have the same regression window as comment 1?  Marking flags based on comment 1.
Assignee: general → ejpbruel
Keywords: regression
Pinged Eddy to find out latest status.
Perhaps this can land? (Feel free to set checkin-needed if this is ready)
Flags: needinfo?(ejpbruel)
Attached patch Updated fix (obsolete) — Splinter Review
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)
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)
Attachment #753568 - Flags: feedback?(choller) → feedback-
(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)
(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.
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.
Flags: needinfo?(ejpbruel)
Attached patch Updated fix (obsolete) — Splinter Review
Attachment #753568 - Attachment is obsolete: true
Attachment #753568 - Flags: review?(jorendorff)
Attachment #756875 - Flags: review?(jorendorff)
Flags: needinfo?(ejpbruel)
Attached patch Updated fix (obsolete) — Splinter Review
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)
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+
Attached patch Updated fixSplinter Review
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)
Attachment #757053 - Flags: review?(jorendorff) → review+
https://hg.mozilla.org/mozilla-central/rev/2bd3d9bbd722
Status: NEW → RESOLVED
Closed: 7 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla24
Whiteboard: [jsbugmon:update] → [jsbugmon:update,ignore]
JSBugMon: The testcase found in this bug no longer reproduces (tried revision 6f32011a27ef).
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Jorendorff was goiong to look into the branch uplifts here.
Assignee: ejpbruel → jorendorff
This is a wontfix for FF22 at this point - sorry, but this didn't make it into our final beta.
Emailed jorendorff to ask about an uplift nom for FF23
Flags: needinfo?(jorendorff)
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?
Flags: needinfo?(jorendorff)
Attachment #757053 - Flags: approval-mozilla-beta?
Attachment #757053 - Flags: approval-mozilla-beta+
Attachment #757053 - Flags: approval-mozilla-aurora?
Attachment #757053 - Flags: approval-mozilla-aurora+
Whiteboard: [jsbugmon:update,ignore] → [jsbugmon:update,ignore][adv-main23+]
Group: core-security
You need to log in before you can comment on or make changes to this bug.