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

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

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


g("module 'foo' {}");
function g(x) {
  eval(x, g.escaped);
}

Comment 1

[Christian Holler (:decoder)]

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 2

[Christian Holler (:decoder)]

Needinfo based on comment 1 :)

Comment 3

[Eddy Bruel [:ejpbruel]]

(In reply to Christian Holler (:decoder) from comment #2)
> Needinfo based on comment 1 :)

Needstacktrace based on needinfo :-)

Comment 4

[Eddy Bruel [: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.

Comment 5

[Eddy Bruel [:ejpbruel]]

Attachment: Quick fix

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.

Comment 6

[Christian Holler (:decoder)]

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

Comment 7

[Andrew McCreight [:mccr8]]

Does comment 6 have the same regression window as comment 1?  Marking flags based on comment 1.

Comment 8

[Alex Keybl [:akeybl]]

Pinged Eddy to find out latest status.

Comment 9

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Perhaps this can land? (Feel free to set checkin-needed if this is ready)

Comment 10

[Eddy Bruel [:ejpbruel]]

Attachment: Updated fix

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.

Comment 11

[Christian Holler (:decoder)]

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?

Comment 12

[Eddy Bruel [:ejpbruel]]

(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.

Comment 13

[Christian Holler (:decoder)]

(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

[Eddy Bruel [:ejpbruel]]

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.

Comment 15

[Christian Holler (:decoder)]

Attachment: [crash-signature] Machine-readable crash signature

Comment 16

[Eddy Bruel [:ejpbruel]]

Attachment: Updated fix

Comment 17

[Eddy Bruel [:ejpbruel]]

Attachment: Updated fix

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.

Comment 18

[Jason Orendorff [: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.

Comment 19

[Eddy Bruel [:ejpbruel]]

Attachment: Updated fix

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.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/2bd3d9bbd722

Comment 21

[Christian Holler (:decoder)]

JSBugMon: The testcase found in this bug no longer reproduces (tried revision 6f32011a27ef).

Comment 22

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 23

[Dave Townsend [:mossop]]

Jorendorff was goiong to look into the branch uplifts here.

Comment 24

[Alex Keybl [:akeybl]]

This is a wontfix for FF22 at this point - sorry, but this didn't make it into our final beta.

Comment 25

[Lukas Blakk [:lsblakk] use ?needinfo]

Emailed jorendorff to ask about an uplift nom for FF23

Comment 26

[Jason Orendorff [: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

Comment 27

[Ryan VanderMeulen [:RyanVM]]

This landed on m-c during the Fx24 cycle.
https://hg.mozilla.org/releases/mozilla-beta/rev/64823c2294bf