Closed Bug 1243793 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: JavaScript Engine, defect, critical)

x86
Linux
defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla47
Tracking Status
firefox45 --- unaffected
firefox46 + verified
firefox47 + verified
firefox-esr38 --- unaffected

People

(Reporter: decoder, Unassigned)

References

(Blocks 1 open bug)

Details

(4 keywords, Whiteboard: [jsbugmon:update])

Attachments

(2 files)

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.
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:   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.
Shu-yu, is bug 1235590 a likely regressor?
Blocks: 1235590
Flags: needinfo?(shu)
Labels!!
Flags: needinfo?(shu)
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.
Attachment #8713300 - Flags: review?(jorendorff) → review+
(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 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.
Attachment #8713300 - Flags: sec-approval?
Keywords: sec-high
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.
Attachment #8713300 - Flags: sec-approval? → sec-approval+
https://hg.mozilla.org/mozilla-central/rev/c62e8f636155
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Status: RESOLVED → VERIFIED
JSBugMon: This bug has been automatically verified fixed.
Would you like to request uplift since 46 is affected?
Flags: needinfo?(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
Flags: needinfo?(shu)
Attachment #8716053 - Flags: approval-mozilla-aurora?
Comment on attachment 8716053 [details] [diff] [review]
Patch for backport with comments applied

Crash fix for recent regression, please uplift to aurora.
Attachment #8716053 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
JSBugMon: This bug has been automatically verified fixed on Fx46
Group: javascript-core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.