Closed Bug 855442 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: JavaScript Engine, defect, critical)

x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla23
Tracking Status
firefox20 --- unaffected
firefox21 --- unaffected
firefox22 + fixed
firefox23 --- fixed
firefox-esr17 --- unaffected
b2g18 --- unaffected
b2g18-v1.0.1 --- unaffected

People

(Reporter: decoder, Assigned: luke)

References

(Blocks 1 open bug)

Details

(5 keywords, Whiteboard: [jsbugmon:ignore][adv-main22-])

Attachments

(1 file)

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));
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).
Blocks: odinfuzz
Whiteboard: [jsbugmon:ignore]
(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.)
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.
Attached patch fixSplinter Review
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.
Attachment #731944 - Flags: review?(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.
Attachment #731944 - Flags: review?(terrence) → review+
https://hg.mozilla.org/mozilla-central/rev/4887c05931e9
https://hg.mozilla.org/mozilla-central/rev/eb7984550c2f
Assignee: general → luke
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
Flags: in-testsuite+
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
Attachment #731944 - Flags: approval-mozilla-aurora?
Comment on attachment 731944 [details] [diff] [review]
fix

sec-crit regression that will be fixed long before release - woot!
Attachment #731944 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Whiteboard: [jsbugmon:ignore] → [jsbugmon:ignore][adv-main22-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.