1243793 - Assertion failure: v.isUndefined(), at js/src/jsstr.cpp:4469 or Assertion failure: stub->monitorsThis() || *GetNextPc(pc) == JSOP_CHECKTHIS || *GetNextPc(pc) == JSOP_CHECKRETURN, at SharedIC.cpp:4756

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase crashes on mozilla-central revision 211a4c710fb6 (build with --enable-optimize --enable-posix-nspr-emulation --enable-valgrind --enable-gczeal --target=i686-pc-linux-gnu --disable-tests --enable-debug, run with --fuzzing-safe --no-threads --disable-oom-functions --baseline-eager):

{
  each: function f() {};
  unescape(f, 0);
  function f() {};
}



Backtrace:

Program received signal SIGSEGV, Segmentation fault.
0x085eedf7 in js::ToStringSlow<(js::AllowGC)1> (cx=cx@entry=0xf7a73020, arg=...) at js/src/jsstr.cpp:4469
#0  0x085eedf7 in js::ToStringSlow<(js::AllowGC)1> (cx=cx@entry=0xf7a73020, arg=...) at js/src/jsstr.cpp:4469
#1  0x085c8594 in ToString<(js::AllowGC)1> (v=..., cx=0xf7a73020) at js/src/jsstr.h:161
#2  ArgToRootedString (cx=cx@entry=0xf7a73020, args=..., argno=0) at js/src/jsstr.cpp:81
#3  0x085d6f6f in str_unescape (cx=0xf7a73020, argc=2, vp=0xffffbdf0) at js/src/jsstr.cpp:319
#4  0x0871d29a in js::CallJSNative (cx=0xf7a73020, native=0x85d6f10 <str_unescape(JSContext*, unsigned int, JS::Value*)>, args=...) at js/src/jscntxtinlines.h:235
[...]
#22 main (argc=6, argv=0xffffcc34, envp=0xffffcc50) at js/src/shell/js.cpp:7000
eax	0x0	0
ebx	0x984c6f0	159696624
ecx	0xf7e3b88c	-136071028
edx	0x0	0
esi	0xf7a73020	-140038112
edi	0xffffbb50	-17584
ebp	0xffffbad8	4294949592
esp	0xffffbaa0	4294949536
eip	0x85eedf7 <js::ToStringSlow<(js::AllowGC)1>(js::ExclusiveContext*, js::MaybeRooted<JS::Value, (js::AllowGC)1>::HandleType)+503>
=> 0x85eedf7 <js::ToStringSlow<(js::AllowGC)1>(js::ExclusiveContext*, js::MaybeRooted<JS::Value, (js::AllowGC)1>::HandleType)+503>:	movl   $0x1175,0x0
   0x85eee01 <js::ToStringSlow<(js::AllowGC)1>(js::ExclusiveContext*, js::MaybeRooted<JS::Value, (js::AllowGC)1>::HandleType)+513>:	call   0x80ff0d0 <abort()>


Marking this one s-s because the asserts seem strange, especially since they appear to be unrelated to each other. Also the v.isUndefined assert is known to be associated with security problems.

Comment 1

[Fuzzing Team]

JSBugMon: Bisection requested, result:
autoBisect shows this is probably related to the following changeset:

The first bad revision is:
changeset:   https://hg.mozilla.org/mozilla-central/rev/0e1c61bab502
user:        Shu-yu Guo
date:        Sat Jan 23 13:28:45 2016 -0800
summary:     Bug 1235590 - Allow redeclaring block-scoped functions and warn about deprecation for now. (r=jorendorff)

This iteration took 441.193 seconds to run.

Comment 2

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

Shu-yu, is bug 1235590 a likely regressor?

Comment 3

[Shu-yu Guo [:shu]]

Labels!!

Comment 4

[Shu-yu Guo [:shu]]

Attachment: Fix handling of labels when emitting hoisted function definitions.

Comment 5

[Jason Orendorff [:jorendorff]]

Comment on attachment 8713300 [details] [diff] [review]
Fix handling of labels when emitting hoisted function definitions.

Review of attachment 8713300 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/frontend/BytecodeEmitter.cpp
@@ -5377,3 @@
>          if (!sc->strict()) {
> -            while (pn->isKind(PNK_LABEL))
> -                pn = pn->as<LabeledStatement>().statement();

OMG.

Would it be overreacting to write a `class ParseNodeRange` and massively search-and-replace the above loop header with a range-based one? Because that's what this makes me feel like doing...

::: js/src/tests/ecma_6/LexicalEnvironment/block-scoped-functions-deprecated-redecl.js
@@ +11,5 @@
>  
> +// Tests labels.
> +{
> +    l: function f() { }
> +    assertEq(f, f);

Please instead make a copy of the immediately preceding test (lines 1-10 of this file) and add a label on the second function.

Comment 6

[Shu-yu Guo [:shu]]

(In reply to Jason Orendorff [:jorendorff] from comment #5)
> Comment on attachment 8713300 [details] [diff] [review]
> Fix handling of labels when emitting hoisted function definitions.
> 
> Review of attachment 8713300 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/frontend/BytecodeEmitter.cpp
> @@ -5377,3 @@
> >          if (!sc->strict()) {
> > -            while (pn->isKind(PNK_LABEL))
> > -                pn = pn->as<LabeledStatement>().statement();
> 
> OMG.
> 
> Would it be overreacting to write a `class ParseNodeRange` and massively
> search-and-replace the above loop header with a range-based one? Because
> that's what this makes me feel like doing...
> 

A good suggestion. Be my guest. :)

Comment 7

[Shu-yu Guo [:shu]]

Comment on attachment 8713300 [details] [diff] [review]
Fix handling of labels when emitting hoisted function definitions.

[Security approval request comment]
How easily could an exploit be constructed based on the patch?

> Easy to hit the crashing path. No clue on ease of exploit.

Do comments in the patch, the check-in comment, or tests included in the patch paint a bulls-eye on the security problem?

> Yes.

Which older supported branches are affected by this flaw?

> Aurora.

If not all supported branches, which bug introduced the flaw?

> Bug 1235590

Do you have backports for the affected branches? If not, how different, hard to create, and risky will they be?

> Should apply cleanly.

How likely is this patch to cause regressions; how much testing does it need?

> Will not cause regressions.

Comment 8

[Al Billings [:abillings - ex-MoCo]]

Comment on attachment 8713300 [details] [diff] [review]
Fix handling of labels when emitting hoisted function definitions.

sec-approval+.

Please nominate for Aurora as well since that seems to be the oldest affected version.

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c62e8f636155

Comment 10

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed.

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

Would you like to request uplift since 46 is affected?

Comment 12

[Shu-yu Guo [:shu]]

Attachment: Patch for backport with comments applied

Comment 13

[Shu-yu Guo [:shu]]

Comment on attachment 8716053 [details] [diff] [review]
Patch for backport with comments applied

Approval Request Comment
[Feature/regressing bug #]: Bug 1235590
[User impact if declined]: Crashes
[Describe test coverage new/current, TreeHerder]: on m-c
[Risks and why]: Low, bug fix only
[String/UUID change made/needed]: None

Comment 14

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8716053 [details] [diff] [review]
Patch for backport with comments applied

Crash fix for recent regression, please uplift to aurora.

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/fc392979e840

Comment 16

[Fuzzing Team]

JSBugMon: This bug has been automatically verified fixed on Fx46