IonMonkey: Assertion failure: (ptrBits & 0x7) == 0, at ./dist/include/js/Value.h:733 or Use of uninitialized value [@ js::gc::MarkValueRange]

VERIFIED FIXED in Firefox 23

Status

()

--
critical
VERIFIED FIXED
6 years ago
5 years ago

People

(Reporter: decoder, Assigned: djvj)

Tracking

(Blocks: 1 bug, 6 keywords)

Trunk
mozilla23
x86_64
Linux
assertion, crash, regression, sec-critical, testcase, valgrind
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox22 unaffected, firefox23 verified, firefox-esr17 unaffected, b2g18 unaffected)

Details

(Whiteboard: [fuzzblocker] [jsbugmon:update,ignore][adv-main23-])

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

6 years ago
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();
(Reporter)

Comment 1

6 years ago
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).
Blocks: 724444
Keywords: crash, valgrind
Summary: Assertion failure: (ptrBits & 0x7) == 0, at ./dist/include/js/Value.h:733 or Use of uninitialized value [@ js::gc::MarkValueRange] → IonMonkey: Assertion failure: (ptrBits & 0x7) == 0, at ./dist/include/js/Value.h:733 or Use of uninitialized value [@ js::gc::MarkValueRange]
Whiteboard: [jsbugmon:update,bisect]
(Reporter)

Updated

6 years ago
Whiteboard: [jsbugmon:update,bisect] → [jsbugmon:update]
(Reporter)

Comment 2

6 years ago
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.
(Reporter)

Comment 3

6 years ago
Needinfo from djvj based on comment 2 :)
(Reporter)

Updated

6 years ago
Flags: needinfo?(kvijayan)
(Reporter)

Comment 4

6 years ago
Marking as fuzzblocker, this produces a quite large set of difficult to reduce test cases.
Whiteboard: [jsbugmon:update] → [jsbugmon:update][fuzzblocker]
(Assignee)

Updated

6 years ago
Assignee: general → kvijayan
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.
Blocks: 864033, 860145
status-firefox22: --- → unaffected
status-firefox23: --- → affected
Keywords: regression, sec-critical
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.
Attachment #740337 - Flags: review?(kvijayan)
(Reporter)

Updated

6 years ago
Whiteboard: [jsbugmon:update][fuzzblocker] → [fuzzblocker] [jsbugmon:update,ignore]
(Reporter)

Comment 7

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

Comment 8

6 years ago
(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.
Flags: needinfo?(kvijayan)
(Assignee)

Comment 9

6 years ago
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.
Attachment #740337 - Flags: review?(kvijayan) → review+
(Assignee)

Comment 10

6 years ago
(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!
(Assignee)

Comment 11

6 years ago
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.
Attachment #740337 - Flags: review+
Created attachment 740771 [details] [diff] [review]
Proposed fix v2

Cheers for the comments, here's the fix with these addressed and test case added.
Attachment #740337 - Attachment is obsolete: true
Attachment #740771 - Flags: review?(kvijayan)

Updated

6 years ago
Duplicate of this bug: 864276
(Assignee)

Comment 14

6 years ago
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.
Attachment #740771 - Flags: review?(kvijayan) → review+
(Assignee)

Comment 15

6 years ago
Created attachment 741124 [details] [diff] [review]
Rebased Ion ArgsObj Patch

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

Comment 16

6 years ago
Created attachment 741129 [details] [diff] [review]
Rebased Ion ArgsObj Patch (with bustage fix)

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.
Attachment #741124 - Attachment is obsolete: true
(Assignee)

Comment 17

6 years ago
Created attachment 741130 [details] [diff] [review]
Rebased fix

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.
Attachment #740771 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/781a47680e34
Status: NEW → RESOLVED
Last Resolved: 6 years ago
status-firefox23: affected → fixed
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
(Reporter)

Updated

6 years ago
Status: RESOLVED → VERIFIED
(Reporter)

Comment 20

6 years ago
JSBugMon: This bug has been automatically verified fixed.
(Assignee)

Updated

6 years ago
Duplicate of this bug: 864033
status-b2g18: --- → unaffected
status-firefox-esr17: --- → unaffected
Marking status-firefox23:verified based on comment 20.
status-firefox23: fixed → verified
Whiteboard: [fuzzblocker] [jsbugmon:update,ignore] → [fuzzblocker] [jsbugmon:update,ignore][adv-main23-]
Group: core-security
You need to log in before you can comment on or make changes to this bug.