Closed Bug 767234 Opened 9 years ago Closed 9 years ago

Write barrier on generator slots can be invoked when they contain garbage

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla16
Tracking Status
firefox15 - ---
firefox16 + fixed
firefox17 + fixed
firefox-esr10 - unaffected

People

(Reporter: billm, Assigned: billm)

References

Details

(Keywords: sec-high, Whiteboard: [js:p1][advisory-tracking-])

Attachments

(1 file)

The following sequence of events is possible:

1. Enter a generator. It stores a pointer to an object O in a local slot.
2. Yield. The slot is saved to the generator.
3. Return to the generator code. Null out the slot pointing to O.
4. GC such that O is collected.
5. Start an incremental GC.
6. Yield again. When we copy back the current stack slots to the generator frame, we invoke an incremental write barrier on the generator slot containing O. This pushes it onto the mark stack, later causing a crash when we try to mark it.

Test case:

function gen()
{
    var local = new Date();
    yield 1;
    local = null;
    gc();
    gcslice(0);
    yield 2;
}

var g = gen();
g.next();
g.next();
gcslice();
Shoot, forgot to hide.
Group: core-security
Attached patch patchSplinter Review
We don't actually ever need to invoke an incremental barrier from copyFrameAndValues. It only needs to do post barriers. The important incremental barrier is done explicitly in GeneratorWriteBarrierPre whenever we change the generator state from one that does not satisfy GeneratorHasMarkableFrame to one that does.

I figured it would be easier to just make this all explicit. That removes the need to parametrize over the types to copyFrameAndValues.
Attachment #635559 - Flags: review?(luke)
So, just to check my understanding:
 - we never need a pre-write barrier (for igc) since nothing is changes reachability
 - we do need a post-barrier since (for ggc) since this changes reachability from old generation?

What about this scenario?
 1. generator is suspended and a slot points to O
 2. igc starts, does not mark O
 3. generator resumes, local copied to stack (no pre-write barrier here, so O still unmarked)
 4. generator returns, frame pops
 5. igc finishes, the generator is marked, but, because it isn't active, O is not marked

At the end, O isn't marked although it is reachable at point 2 which means some other object could hold a dangling pointer to it.

Have I missed a place where we barrier in this case?  Is this solvable by having popGeneratorFrame apply a write barrier in the !isYielding case?
In step 3, I think that we will call SendToGenerator, which calls GeneratorWriteBarrierPre, so O would be marked there. Correct me if I'm wrong, though.

More generally, when the generator resumes, the value of GeneratorHasMarkableFrame(gen) will change from true to false, and that's when we're supposed to do the write barrier, because that's when the saved slots are "destroyed", in some theoretical sense. That's what the code is supposed to do.
Comment on attachment 635559 [details] [diff] [review]
patch

Ah, of course, thanks.
Attachment #635559 - Flags: review?(luke) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d8cf9fc3775
Summary: Write barrier on slots can be invoked when they contain garbage → Write barrier on generator slots can be invoked when they contain garbage
Target Milestone: --- → mozilla16
Whiteboard: [js:p1]
https://hg.mozilla.org/mozilla-central/rev/9d8cf9fc3775
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Is this a regression from something recent or a problem that affects older branches? Is it worth getting into Beta (Fx15) or ESR-10?
Incremental GC is only enabled on 16, and this requires incremental GC to be enabled or else nothing bad happens.
Duplicate of this bug: 757795
Duplicate of this bug: 763450
Whiteboard: [js:p1] → [js:p1][advisory-tracking-]
Keywords: verifyme
Group: core-security
mass remove verifyme requests greater than 4 months old
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.