Closed Bug 1067473 Opened 5 years ago Closed 5 years ago

BarrieredCell<JSObject> is calling the wrong zone() implementation

Categories

(Core :: JavaScript: GC, defect, P1, critical)

defect

Tracking

()

RESOLVED FIXED
Tracking Status
firefox33 --- wontfix
firefox34 --- wontfix
firefox35 --- fixed
firefox36 --- fixed
firefox-esr31 --- wontfix

People

(Reporter: terrence, Assigned: terrence)

References

(Blocks 1 open bug)

Details

(Keywords: sec-high, Whiteboard: [fixed by bug 1068223][adv-main35+])

While investigating bug 1029549, I noticed a rather serious problem with BarrieredCell. Cell::zone() is implemented on BarrieredCell<T>::zone, such that the default calls T::tenuredZone(). For ObjectImpl, T::zone is overridden as shape_->tenuredZone() so that it works on Nursery things. However, this is a template and the only override is for ObjectImpl. Even though JSObject, JSFunction, etc derive from ObjectImpl, templates don't work at that level, so when doing MarkInternal<JSObject> we call BarrieredCell<JSObject>::zone, which is not overridden. This gets us the wrong behavior and will crash on or after arenaHeader() if used on a nursery object. This is probably causing real instability in the field.

When reviewing Nick's patch adding BarrieredCell, I convinced myself that this would work somehow, but the stacks in bug 1029549 prove that we were both wrong. I think we should just undo BarrieredCell and bite the bullet with the duplicated code. It's not substantially more code and the extra complexity was hiding at least one real bug.

The security implications here are not really clear, but are certainly bad. In theory, an attacker could mock up an ArenaHeader in the nursery and cause the GC to go off the rails. I'm marking this as sec-high for now.
Wow. That's a rather important find!

It also seems like a worrisome gap in our tests, since tenuredZone() and tenuredZoneFromAnyThread() both assert isTenured().
Keywords: sec-high
Whiteboard: sec-high
The patch to completely fix this is enormous and not easily backportable. We could push a bandage for this one instance, but then it will be obvious what the error is and make it possible to find other instances. On the other hand, if we do the rewrite, it will completely hide the fact that there was a problem in the prior code. Given how subtle the error is (we haven't hit it on TBPL), I am going to do the rewrite in a separate cleanup/cover bug and let the fix ride the trains.
I've landed the cover bug, bug 1068223.
Thanks, terrence. Apologies for the screw-up.
No worries! I didn't spot it either. Just another good reminder of how C++ is a giant foot-cannon waiting to happen.
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [fixed by bug 1068223]
Whiteboard: [fixed by bug 1068223] → [fixed by bug 1068223][adv-main35+]
Group: core-security → core-security-release
Group: core-security-release
You need to log in before you can comment on or make changes to this bug.