754725 - Assertion failure: (ptrBits & 0x7) == 0, at jsval.h:760 or Crash [@ compartment]

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following test asserts/crashes on mozilla-central revision c758cc9b60e5 (options -m -n):


try {
function fun() {
    (new fun (("10"))) ('function pf' + (((true) )[ fun ]) + '() {}');
}
fun();
} catch(exc1) {}
function eval() { eval(  ) ; }
function DoWhile_3()
  eval();
try {
  DoWhile_3(  ) ;
} catch(e) { }
function f()
  test();
function test() {
  try {
    for (var i in f());
  } catch ( foopy  ) {  }  
  gc();
}
test();


Backtrace in Opt build:

==7571== Invalid read of size 8
==7571==    at 0x5781C1: void js::gc::MarkInternal<JSString>(JSTracer*, JSString**) (Heap.h:969)
==7571==    by 0x5799BD: js::gc::MarkValueInternal(JSTracer*, JS::Value*) (Marking.cpp:329)
==7571==    by 0x579BBE: js::gc::MarkValueRootRange(JSTracer*, unsigned long, JS::Value*, char const*) (Marking.cpp:369)
==7571==    by 0x5384B1: js::StackSpace::mark(JSTracer*) (Stack.cpp:527)
==7571==    by 0x4567DD: _ZN2jsL11MarkRuntimeEP8JSTracerb.clone.246 (jsgc.cpp:2325)
==7571==    by 0x456D37: BeginMarkPhase(JSRuntime*) (jsgc.cpp:2975)
==7571==    by 0x45A210: GCCycle(JSRuntime*, bool, long, js::JSGCInvocationKind) (jsgc.cpp:3271)
==7571==    by 0x45B38F: Collect(JSRuntime*, bool, long, js::JSGCInvocationKind, js::gcreason::Reason) (jsgc.cpp:3719)
==7571==    by 0x56C80C: GC(JSContext*, unsigned int, JS::Value*) (TestingFunctions.cpp:52)
==7571==    by 0x47E337: js::InvokeKernel(JSContext*, js::CallArgs, js::MaybeConstruct) (jscntxtinlines.h:426)
==7571==    by 0x47A234: js::Interpret(JSContext*, js::StackFrame*, js::InterpMode) (jsinterp.cpp:2744)
==7571==    by 0x5F9DDD: UncachedInlineCall(js::VMFrame&, js::InitialFrameFlags, void**, bool*, unsigned int) (InvokeHelpers.cpp:376)
==7571==  Address 0xe3000 is not stack'd, malloc'd or (recently) free'd


Assuming s-s and sec-critical due to GC related crash. Furthermore, this crash affects mozilla-aurora and mozilla-beta as well.

Comment 1

[Christian Holler (:decoder)]

Just testing some new JSBugMon functionality here.

Comment 2

[Christian Holler (:decoder)]

Should probably learn my own syntax...

Comment 3

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically confirmed to be still valid on branch mozilla-aurora  (reproduced on revision 56d137349efc).
JSBugMon: This bug has been automatically confirmed to be still valid on branch mozilla-beta  (reproduced on revision 0540130b5f2f).
JSBugMon: The testcase found in this bug does not reproduce on branch mozilla-release (tried revision 5bcfa0da3be9).

Comment 4

[David Mandelin [:dmandelin]]

Moving this back a cycle--IGC takes priority.

Comment 5

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: patch

In this test, we have a dead value on the VM stack. It contains garbage--in this case, the tag makes it look like a NullValue, but the low bits are nonzero so that isNull() is false. We do one GC and avoid marking the value because it's dead. However, we don't overwrite it with anything because it's not an object or a string. Later, we do a second GC and try to mark it. isGCThing() returns true because something with a NULL tag counts as a GC thing. But then when we convert it to a Cell and try to mark it, we crash.

The fix is to normalize the low bits of the value while still leaving the tag unaffected.

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/84af366e007f

Comment 7

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/84af366e007f

Comment 8

[Daniel Veditz [:dveditz]]

Am I interpreting comment 3 correctly that this is a regression in Firefox 13, and therefore doesn't affect the ESR branch? Although if we don't know the cause of the regression we might still need it if it was due to a security fix taken on that branch.

qawanted: please find when this bug crept in.

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 632965 [details] [diff] [review]
patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 723313
User impact if declined: Low likelihood of GC crashes, exploits.
Testing completed (on m-c, etc.): On m-c.
Risk to taking this patch (and alternatives if risky): Low. Semantics should be unchanged except when the bug happens.
String or UUID changes made by this patch: None

Regarding ESR, there's no need to take this. It regressed in FF13.

Comment 10

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

(removing keywords because bug 723313 has been identified as the regressor)

Comment 11

[Lukas Blakk [:lsblakk] use ?needinfo]

Comment on attachment 632965 [details] [diff] [review]
patch

low risk, sec critical - approving.

Comment 12

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1f4ac5f52188
https://hg.mozilla.org/releases/mozilla-beta/rev/6a43080525ab

Comment 13

[Christian Holler (:decoder)]

JSBugMon: This bug has been automatically verified fixed.

Comment 14

[Christian Holler (:decoder)]

Test is too slow to be added and I did not succeed in rewriting, marking in-testsuite-.