855442 - OdinMonkey: Assertion failure: size_t(dst - src) >= nelem, at jsutil.h:229 and invalid read

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Christian Holler (:decoder)]

The following testcase asserts on mozilla-central revision 178a4a770bb1 (run with --ion-eager):


  version(170);
gczeal(2,1);
(function () {
})();
function asmModule(g, foreign, heap) {
    "use asm";
    let HEAP8 = new g.Int8Array(heap);
    function f() { return 99; } 
    return {f: f};
}
let m2 = asmModule(this, 2, new ArrayBuffer(2048));

Comment 1

[Christian Holler (:decoder)]

The assertion doesn't reproduce all the time, but there is something going wrong in memory:

==31681== Invalid read of size 2
==31681==    at 0x473210: JSAtom* js::AtomizeChars<(js::AllowGC)1>(JSContext*, unsigned short const*, unsigned long, js::InternBehavior) (String-inl.h:437)
==31681==    by 0x72AEDA: js::frontend::TokenStream::getTokenInternal() (TokenStream.cpp:1129)
==31681==    by 0x70E869: js::frontend::Parser<js::frontend::FullParseHandler>::assignExpr() (TokenStream.h:621)
==31681==    by 0x70D07C: js::frontend::Parser<js::frontend::FullParseHandler>::primaryExpr(js::frontend::TokenKind) (Parser.cpp:6470)
==31681==    by 0x70D7F0: js::frontend::Parser<js::frontend::FullParseHandler>::memberExpr(js::frontend::TokenKind, bool) (Parser.cpp:6029)
==31681==    by 0x70DFE1: js::frontend::Parser<js::frontend::FullParseHandler>::unaryExpr() (Parser.cpp:5143)
==31681==    by 0x70E4D1: js::frontend::Parser<js::frontend::FullParseHandler>::orExpr1() (Parser.cpp:4764)
==31681==    by 0x70E8D7: js::frontend::Parser<js::frontend::FullParseHandler>::assignExpr() (Parser.cpp:4816)
==31681==    by 0x70FD32: js::frontend::Parser<js::frontend::FullParseHandler>::expr() (Parser.cpp:4617)
==31681==    by 0x711F6C: js::frontend::Parser<js::frontend::FullParseHandler>::returnOrYield(bool) (Parser.cpp:2924)
==31681==    by 0x7126F9: js::frontend::Parser<js::frontend::FullParseHandler>::statement() (Parser.cpp:4444)
==31681==    by 0x712C58: js::frontend::Parser<js::frontend::FullParseHandler>::statements(bool*) (Parser.cpp:2272)
==31681==  Address 0x6a21584 is 196 bytes inside a block of size 206 free'd
==31681==    at 0x4C2A82E: free (in /usr/lib/valgrind/vgpreload_memcheck-amd64-linux.so)
==31681==    by 0x4E68B4: bool js::gc::Arena::finalize<JSString>(js::FreeOp*, js::gc::AllocKind, unsigned long) (Utility.h:168)
==31681==    by 0x4ECA1D: _ZL14FinalizeArenasPN2js6FreeOpEPPNS_2gc11ArenaHeaderERNS2_9ArenaListENS2_9AllocKindERNS_11SliceBudgetE.constprop.457 (jsgc.cpp:418)
==31681==    by 0x4F0133: IncrementalCollectSlice(JSRuntime*, long, JS::gcreason::Reason, js::JSGCInvocationKind) (jsgc.cpp:1390)
==31681==    by 0x4F10F8: GCCycle(JSRuntime*, bool, long, js::JSGCInvocationKind, JS::gcreason::Reason) (jsgc.cpp:4422)
==31681==    by 0x4F15FA: _ZL7CollectP9JSRuntimeblN2js18JSGCInvocationKindEN2JS8gcreason6ReasonE.part.376 (jsgc.cpp:4550)
==31681==    by 0x4638A1: JSObject* js::gc::NewGCThing<JSObject, (js::AllowGC)1>(JSContext*, js::gc::AllocKind, unsigned long, js::gc::InitialHeap) (jsgcinlines.h:490)
==31681==    by 0x570934: NewObject(JSContext*, js::Class*, js::types::TypeObject*, JSObject*, js::gc::AllocKind, js::NewObjectKind) (jsgcinlines.h:553)
==31681==    by 0x571BBC: js::NewObjectWithClassProtoCommon(JSContext*, js::Class*, JSObject*, JSObject*, js::gc::AllocKind, js::NewObjectKind) (jsobj.cpp:1311)
==31681==    by 0x4CBE46: js::NewFunction(JSContext*, JS::Handle<JSObject*>, int (*)(JSContext*, unsigned int, JS::Value*), unsigned int, JSFunction::Flags, JS::Handle<JSObject*>, JS::Handle<JSAtom*>, js::gc::AllocKind, js::NewObjectKind) (jsobjinlines.h:1603)
==31681==    by 0x9BB842: js::LinkAsmJS(JSContext*, unsigned int, JS::Value*) (AsmJSLink.cpp:378)
==31681==    by 0x53E3BD: js::InvokeKernel(JSContext*, JS::CallArgs, js::MaybeConstruct) (jscntxtinlines.h:338)


Marking s-s and sec-high, assuming that this is some form of use-after-free (gczeal involved).

Comment 2

[Nicholas Nethercote [inactive]]

(FWIW, if you put "> " at the start of a line in bugzilla, it won't wrap it.  It's useful for long-lined things like Valgrind output.)

Comment 3

[Luke Wagner [:luke]]

Hah, get this, Rooted<JSFlatString>... isn't really rooting.  Atm, Rooteds rely on the conservative stack scanner and apparently they aren't forcing the pointer to be written to the stack in the case (as we have here) where the variable is dead.  Moving the "src->chars()" to right before CompileFunctionBody makes 'src' live across the GC call and thus fixes the assertion.  Terrence is looking into the fix for the underlying problem of Rooteds not rooting.

Comment 4

[Luke Wagner [:luke]]

Attachment: fix

The fix is to use a JSStableString.  Stable strings are what you are supposed to use when pointing into a string's chars anyway (which we are doing) and Rooted<JSStableString*> is specifically specialized for this case.

Comment 5

[Terrence Cole [:terrence]]

Comment on attachment 731944 [details] [diff] [review]
fix

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

::: js/src/jsscript.cpp
@@ +1226,5 @@
>  #else
>      chars = data.source;
>  #endif
> +    Rooted<JSFlatString*> flatStr(cx, js_NewStringCopyN<CanGC>(cx, chars + start, stop - start));
> +    return flatStr->ensureStable(cx);

JSString::ensureStable cannot cause a GC, so we should avoid creating the extra root here.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/4887c05931e9
https://hg.mozilla.org/mozilla-central/rev/eb7984550c2f

Comment 7

[Luke Wagner [:luke]]

Comment on attachment 731944 [details] [diff] [review]
fix

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 840282
User impact if declined: potential crash
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alternatives if risky): very low

Comment 8

[Alex Keybl [:akeybl]]

Comment on attachment 731944 [details] [diff] [review]
fix

sec-crit regression that will be fixed long before release - woot!

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/76485eb7a40c