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

Status: RESOLVED FIXED

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

Description

[Terrence Cole [:terrence]]

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.

Comment 1

[Steve Fink [:sfink] [:s:]]

Wow. That's a rather important find!

It also seems like a worrisome gap in our tests, since tenuredZone() and tenuredZoneFromAnyThread() both assert isTenured().

Comment 2

[Terrence Cole [:terrence]]

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.

Comment 3

[Terrence Cole [:terrence]]

I've landed the cover bug, bug 1068223.

Comment 4

[Nicholas Nethercote [inactive]]

Thanks, terrence. Apologies for the screw-up.

Comment 5

[Terrence Cole [:terrence]]

No worries! I didn't spot it either. Just another good reminder of how C++ is a giant foot-cannon waiting to happen.