The following testcase asserts on mozilla-central revision dd03d42b01b1 (run with --ion-eager): gczeal(8); function args(a) { return arguments; } for (var i = 0; i < (1969); i++) a1 = args();

In Valgrind, an uninitialized value shows up: ==60711== Conditional jump or move depends on uninitialised value(s) ==60711== at 0x77C5E4: js::gc::MarkValueRange(JSTracer*, unsigned long, js::EncapsulatedValue*, char const*) (Value.h:740) ==60711== by 0x657609: js::ArgumentsObject::trace(JSTracer*, JSObject*) (ArgumentsObject.cpp:566) ==60711== by 0x77DDCC: js::GCMarker::processMarkStackTop(js::SliceBudget&) (Marking.cpp:1412) ==60711== by 0x77E3DC: js::GCMarker::drainMarkStack(js::SliceBudget&) (Marking.cpp:1465) ==60711== by 0x4F41D0: IncrementalCollectSlice(JSRuntime*, long, JS::gcreason::Reason, js::JSGCInvocationKind) (jsgc.cpp:3805) ==60711== by 0x4F67B8: GCCycle(JSRuntime*, bool, long, js::JSGCInvocationKind, JS::gcreason::Reason) (jsgc.cpp:4447) ==60711== by 0x4F6CB2: _ZL7CollectP9JSRuntimeblN2js18JSGCInvocationKindEN2JS8gcreason6ReasonE.part.382 (jsgc.cpp:4606) ==60711== by 0x4F74F8: js::gc::RunDebugGC(JSContext*) (jsgc.cpp:4516) ==60711== by 0x467FB9: JSObject* js::gc::NewGCThing<JSObject, (js::AllowGC)1>(JSContext*, js::gc::AllocKind, unsigned long, js::gc::InitialHeap) (jsgcinlines.h:519) ==60711== by 0x54E973: JSObject::create(JSContext*, js::gc::AllocKind, js::gc::InitialHeap, JS::Handle<js::Shape*>, JS::Handle<js::types::TypeObject*>, js::HeapSlot*) (jsgcinlines.h:561) ==60711== by 0x65A1C7: js::ArgumentsObject* js::ArgumentsObject::create<CopyIonJSFrameArgs>(JSContext*, JS::Handle<JSScript*>, JS::Handle<JSFunction*>, unsigned int, CopyIonJSFrameArgs&) (ArgumentsObject.cpp:210) ==60711== by 0x65A4BA: js::ArgumentsObject::createForIon(JSContext*, js::ion::IonJSFrameLayout*, JS::Handle<JSObject*>) (ArgumentsObject.cpp:270) Backtrace of assertion: Program received signal SIGSEGV, Segmentation fault. STRING_TO_JSVAL_IMPL (str=<optimized out>) at ./dist/include/js/Value.h:640 640 MOZ_ASSERT((strBits >> JSVAL_TAG_SHIFT) == 0); (gdb) bt #0 STRING_TO_JSVAL_IMPL (str=<optimized out>) at ./dist/include/js/Value.h:640 #1 setString (str=<optimized out>, this=<optimized out>) at ./dist/include/js/Value.h:894 #2 MarkValueInternal (v=0xf57bf0, trc=<optimized out>) at js/src/gc/Marking.cpp:505 #3 js::gc::MarkValueRange (trc=<optimized out>, len=1, vec=<optimized out>, name=<optimized out>) at js/src/gc/Marking.cpp:568 #4 0x000000000065760a in js::ArgumentsObject::trace (trc=0xf3bf50, obj=<optimized out>) at js/src/vm/ArgumentsObject.cpp:566 #5 0x000000000077ddcd in js::GCMarker::processMarkStackTop (this=0xf3bf50, budget=...) at js/src/gc/Marking.cpp:1412 #6 0x000000000077e3dd in js::GCMarker::drainMarkStack (this=0xf3bf50, budget=...) at js/src/gc/Marking.cpp:1465 #7 0x00000000004f41d1 in DrainMarkStack (phase=js::gcstats::PHASE_MARK, sliceBudget=..., rt=0xf3bc90) at js/src/jsgc.cpp:3805 #8 IncrementalCollectSlice (rt=0xf3bc90, budget=<optimized out>, reason=JS::gcreason::DEBUG_GC, gckind=js::GC_NORMAL) at js/src/jsgc.cpp:4289 #9 0x00000000004f67b9 in GCCycle (rt=0xf3bc90, incremental=<optimized out>, budget=-2, gckind=js::GC_NORMAL, reason=JS::gcreason::DEBUG_GC) at js/src/jsgc.cpp:4447 #10 0x00000000004f6cb3 in Collect (rt=0xf3bc90, incremental=true, budget=-2, gckind=js::GC_NORMAL, reason=JS::gcreason::DEBUG_GC) at js/src/jsgc.cpp:4606 #11 0x00000000004f74f9 in Collect (reason=JS::gcreason::DEBUG_GC, gckind=js::GC_NORMAL, budget=-2, incremental=true, rt=0xf3bc90) at js/src/jsgc.cpp:4516 #12 js::gc::RunDebugGC (cx=<optimized out>) at js/src/jsgc.cpp:4806 #13 0x0000000000467fba in js::gc::NewGCThing<JSObject, (js::AllowGC)1> (cx=0xf597f0, kind=js::gc::FINALIZE_OBJECT4_BACKGROUND, thingSize=<optimized out>, heap=<optimized out>) at ../jsgcinlines.h:519 #14 0x000000000054e974 in js_NewGCObject<(js::AllowGC)1> (heap=js::gc::TenuredHeap, kind=js::gc::FINALIZE_OBJECT4_BACKGROUND, cx=<optimized out>) at ../jsgcinlines.h:561 #15 JSObject::create (cx=<optimized out>, kind=js::gc::FINALIZE_OBJECT4_BACKGROUND, heap=js::gc::TenuredHeap, shape=0x7ffff6047948, type=..., extantSlots=<optimized out>) at ../jsobjinlines.h:947 #16 0x000000000065a1c8 in js::ArgumentsObject::create<CopyIonJSFrameArgs> (cx=<optimized out>, script=0x7ffff6038230, callee=(JSFunction * const) 0x7ffff6045240 [object Function "args"], numActuals=<optimized out>, copy=...) at js/src/vm/ArgumentsObject.cpp:210 #17 0x000000000065a4bb in js::ArgumentsObject::createForIon (cx=<optimized out>, frame=<optimized out>, scopeChain=...) at js/src/vm/ArgumentsObject.cpp:270 If I just run the test without GDB or Valgrind, then it crashes (probably due to different uninitialized value).

JSBugMon: Bisection requested, result: autoBisect shows this is probably related to the following changeset: The first bad revision is: changeset: 129263:d12788533ab7 user: Kannan Vijayan date: Thu Apr 18 16:47:25 2013 -0400 summary: Bug 860145 - Allow Ion to compile functions which require heavyweight arguments-object construction. r=jandem r=nbp This iteration took 151.809 seconds to run.

Needinfo from djvj based on comment 2 :)

Marking as fuzzblocker, this produces a quite large set of difficult to reduce test cases.

The fuzzing and browsing crashes from this seem to be all over the place in GC-sensitive code, so I'm marking this sec-critical.

It looks like CopyIonJSFrameArgs::copyArgs() doesn't initialize all of the argument data if there are less arguments supplied than the function takes. When GC happens, the system attempts to mark the uninitialized value crashes. I've verified this fixes the crash at the revision given in the defect, however I can't get this to reproduce at the current tip for some reason.

JSBugMon: The testcase found in this bug no longer reproduces (tried revision 1150403342b2).

(In reply to Jon Coppeard (:jonco) from comment #6) > Created attachment 740337 [details] [diff] [review] > Potential fix > > It looks like CopyIonJSFrameArgs::copyArgs() doesn't initialize all of the > argument data if there are less arguments supplied than the function takes. > When GC happens, the system attempts to mark the uninitialized value crashes. > > I've verified this fixes the crash at the revision given in the defect, > however I can't get this to reproduce at the current tip for some reason. RyanVM backed out the arguments-object patches, so tip probably won't reproduce. You want to update to just before his backouts. I was just implementing this when I saw that you had taken care of it. Nice, and thank you :) Comments coming up.

Comment on attachment 740337 [details] [diff] [review] Potential fix Review of attachment 740337 [details] [diff] [review]: ----------------------------------------------------------------- I was going to say that the |totalArgs| could be computed within each function (each of the stack frame variants can obtain the callee function or NULL if it doesn't exist and it's a script-only frame). However, I think your changes to make the |totalArguments| an explicit parameter is a good thing. r=me with comments addressed. ::: js/src/vm/ArgumentsObject.cpp @@ +113,2 @@ > unsigned numActuals = frame_->numActualArgs(); > + JS_ASSERT(numActuals <= totalArgs); You can extract numFormals from the IonJSFrameLayout here. unsigned numFormals = CalleeTokenToFunction(frame_->calleeToken())->nargs; JS_ASSERT(numFormals <= totalArgs); @@ +120,4 @@ > while (src != end) > (dst++)->init(*src++); > + > + if (numActuals < totalArgs) { This can become (numActuals < numFormals), after above change. @@ +123,5 @@ > + if (numActuals < totalArgs) { > + HeapValue *dstEnd = dstBase + totalArgs; > + while (dst != dstEnd) > + (dst++)->init(UndefinedValue()); > + } Additionally (as with CopyStackFrameArguments): JS_ASSERT(Max(numActuals, numFormals) == totalArgs); @@ +156,5 @@ > > /* Define formals which are not part of the actuals. */ > unsigned numActuals = iter_.numActualArgs(); > + JS_ASSERT(numActuals <= totalArgs); > + if (numActuals < totalArgs) { Do the same asserts as above here. Keep the numFormals, and ASSERT that it's <= totalArgs. @@ +162,3 @@ > while (dst != dstEnd) > (dst++)->init(UndefinedValue()); > } Again, as with the other variants of the copyArgs(), assert that Max(numActuals, numFormals) == totalArgs here too.

(In reply to Jon Coppeard (:jonco) from comment #6) > Created attachment 740337 [details] [diff] [review] > Potential fix > > It looks like CopyIonJSFrameArgs::copyArgs() doesn't initialize all of the > argument data if there are less arguments supplied than the function takes. > When GC happens, the system attempts to mark the uninitialized value crashes. > > I've verified this fixes the crash at the revision given in the defect, > however I can't get this to reproduce at the current tip for some reason. Also: Add a test case containing the fuzzbug code in comment 1!

Comment on attachment 740337 [details] [diff] [review] Potential fix Review of attachment 740337 [details] [diff] [review]: ----------------------------------------------------------------- Actually, I'd like to re-review the fixed up patch with the testcase added and comments addressed before signing off on it.

Cheers for the comments, here's the fix with these addressed and test case added.

Comment on attachment 740771 [details] [diff] [review] Proposed fix v2 Review of attachment 740771 [details] [diff] [review]: ----------------------------------------------------------------- Looks great! I'll need to re-land the arguments object patch before you can push this on top of it, so we should coordinate on that. Get in touch on IRC and we'll coordinate that.

Jon: This is the rebased ion argsobj patch, against latest tip (as of 10:00pm eastern time)

Forgot to merge the bustage fix into the previous rebased patch. That's fixed in this one. When pushing, just this one and your fix patch should be good enough.

Your fix to this bug needed a trivial but non-autoamtic merge. I needed to do it anyway to test on try, so I'm putting it up. Pushing the above patch plus this one should do the job.

JSBugMon: This bug has been automatically verified fixed.

Marking status-firefox23:verified based on comment 20.