Closed Bug 1002754 Opened 6 years ago Closed 6 years ago

APZC destructor has a use-after-free hazard when progressive painting is enabled

Categories

(Core :: Panning and Zooming, defect)

All
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32
feature-b2g 2.0

People

(Reporter: kats, Assigned: kats)

References

Details

(Keywords: csectype-uaf)

Attachments

(2 files, 8 obsolete files)

9.72 KB, patch
botond
: review+
Details | Diff | Splinter Review
10.41 KB, patch
kats
: review+
Details | Diff | Splinter Review
While debugging progressive painting on B2G I ran into a use-after-free problem in the APZC destructor. It attempts to call SendReleaseSharedCompositorFrameMetrics on the mCrossProcessCompositorParent after that CrossProcessCompositorParent instance has been destroyed. I can repro this intermittently using my local patch queue and starting the contacts app on b2g.

I verified this was a use-after-free by printf'ing when the object gets destroyed and when it is dereferenced, since gdb was misreporting things when it caught the segfault.

Randall, do you recall what the expected behaviour in this case is? If I change the mCrossProcessCompositorParent to be some sort of RefPtr and just skip the call to SendReleaseSharedCompositorFrameMetrics, is that ok? Or will it leave stuff in a bad state?
Flags: needinfo?(rbarker)
The mCrossProcessCompositorParent was expected to be valid. If it is being deleted before the APZC then it needs to be removed from the APZC at time of deletion.

From: https://bugzilla.mozilla.org/show_bug.cgi?id=895358#c29

> > @@ +516,4 @@
> > >  
> > >    uint64_t mLayersId;
> > >    nsRefPtr<CompositorParent> mCompositorParent;
> > > +  PCompositorParent* mCrossProcessCompositorParent;
> > 
> > Does this need to be refcounted? When is the PCompositorParent deleted? Is
> > it ever possible for it to get deleted while an APZC instance still has a
> > pointer to it? 
> 
> Can an IPDL generated interface be refcounted? I thought it couldn't. It should live as long as
> the content process it is associated. I did look into at the time and is seem okay but that doesn't
> mean things can't change.
Flags: needinfo?(rbarker)
Attached patch Patch (obsolete) — Splinter Review
Assignee: nobody → bugmail.mozilla
Attachment #8414159 - Flags: review?(rbarker)
Attachment #8414159 - Flags: review?(botond)
Comment on attachment 8414159 [details] [diff] [review]
Patch

Review of attachment 8414159 [details] [diff] [review]:
-----------------------------------------------------------------

It is unfortunate that there doesn't appear to be a better way to track the life cycle of mCrossProcessParent. If it can be destroyed before the dtor of the APZC is called what is to insure it won't happen even earlier at some point in the future.
Attachment #8414159 - Flags: review?(rbarker) → review+
(In reply to Randall Barker [:rbarker] from comment #3)
> It is unfortunate that there doesn't appear to be a better way to track the
> life cycle of mCrossProcessParent. If it can be destroyed before the dtor of
> the APZC is called what is to insure it won't happen even earlier at some
> point in the future.

It is kinda sucky, I agree. I couldn't find a good way to detect this better without significant changes to the codebase so maybe we can defer a full solution to when we run into the problem.
Attached patch Patch (obsolete) — Splinter Review
Actually I was able to do a little better and guard the other use site of mCrossProcessCompositorParent as well. It's still ugly but at least it should provide a more graceful failure if things change in the future.
Attachment #8414159 - Attachment is obsolete: true
Attachment #8414159 - Flags: review?(botond)
Attachment #8415452 - Flags: review?(rbarker)
Attachment #8415452 - Flags: review?(botond)
Comment on attachment 8415452 [details] [diff] [review]
Patch

Review of attachment 8415452 [details] [diff] [review]:
-----------------------------------------------------------------

::: gfx/layers/apz/src/AsyncPanZoomController.cpp
@@ +454,5 @@
> +{
> +  Compositor::AssertOnCompositorThread();
> +
> +  PCompositorParent* compositor = nullptr;
> +  if (mCrossProcessCompositorParent) {

Is there any point in keeping around mCrossProcessCompositorParent if we do this?
Yeah, in the case where the APZC is destroyed before the CrossProcessCompositorParent we still need to send the message to release the shared frame metrics.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #7)
> Yeah, in the case where the APZC is destroyed before the
> CrossProcessCompositorParent we still need to send the message to release
> the shared frame metrics.

Right, but now we always look in the LayerTreeState to get it, so why store it?
Doh, good point. You're right, we can just get rid of it. /me missed the big picture
Attached patch Patch (v3) (obsolete) — Splinter Review
Now with 100% fewer stale pointers
Attachment #8415452 - Attachment is obsolete: true
Attachment #8415452 - Flags: review?(rbarker)
Attachment #8415452 - Flags: review?(botond)
Attachment #8415498 - Flags: review?(rbarker)
Attachment #8415498 - Flags: review?(botond)
Attachment #8415498 - Flags: review?(rbarker) → review+
Attachment #8415498 - Flags: review?(rbarker)
Attachment #8415498 - Flags: review?(botond)
Attachment #8415498 - Flags: review+
Comment on attachment 8415498 [details] [diff] [review]
Patch (v3)

Review of attachment 8415498 [details] [diff] [review]:
-----------------------------------------------------------------

Not sure what happened with the flags there... Anyways, the patch has r+ from me and rbarker.
Attachment #8415498 - Flags: review?(rbarker) → review+
Backed out for gtest failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/8390ac871e97

https://tbpl.mozilla.org/php/getParsedLog.php?id=38877894&tree=Mozilla-Inbound

Please verify this is green on Try before the next checkin request.
Looks like we need to do something like introduce AsyncPanZoomController::AssertOnCompositorThread() (forwarding to Compositor::AssertOnCompositorThread() much like APZCTreeManager::AssertOnCompositorThread()), and override it to be a no-op in TestAsyncPanZOomController.
This is the original patch with the trivial modification of changing Compositor::AssertOnCompositorThread() to AsyncPanZoomController::AssertOnCompositorThread(). Carrying r+'s.

These patches solve the gtest failure in the way outlined in comment 14. I'd push to Try but Try pushes seem to be hanging...
Attachment #8415498 - Attachment is obsolete: true
Attachment #8415988 - Flags: review+
(In reply to Botond Ballo [:botond] from comment #16)
> I'd push to Try but Try pushes seem to be hanging...

Turns out I'm just impatient :)

https://tbpl.mozilla.org/?tree=Try&rev=679ad4d76acc
Whoops, was missing a 'const' on the overriding method.

New Try push: https://tbpl.mozilla.org/?tree=Try&rev=d230b21e7826
Attachment #8415985 - Attachment is obsolete: true
Attachment #8415985 - Flags: review?(bugmail.mozilla)
Attachment #8415999 - Flags: review?(bugmail.mozilla)
And now I put the const on the wrong method... going to build locally before Trying again :)
Attachment #8415999 - Attachment is obsolete: true
Attachment #8415999 - Flags: review?(bugmail.mozilla)
Attachment #8416003 - Flags: review?(bugmail.mozilla)
Comment on attachment 8416003 [details] [diff] [review]
Part 1 - Introduce AsyncPanZoomController::AssertOnCompositorThread() which can be mocked out in gtests

Review of attachment 8416003 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for taking care of this. Guess I don't need the build I kicked off over lunch :/
Attachment #8416003 - Flags: review?(bugmail.mozilla) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #20)
> Comment on attachment 8416003 [details] [diff] [review]
> Part 1 - Introduce AsyncPanZoomController::AssertOnCompositorThread() which
> can be mocked out in gtests
> 
> Review of attachment 8416003 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for taking care of this. Guess I don't need the build I kicked off
> over lunch :/

Having a build around never hurts :)
(In reply to Botond Ballo [:botond] from comment #22)
> https://tbpl.mozilla.org/?tree=Try&rev=e72b12b45d73

And failed again. Turns out AssertOnCompositorThread() is AsyncPanZoomController's first virtual functions, and compilers Werror on having a virtual function but not virtual destructor (except the ones we build with locally, naturally).

Carrying r+.

https://tbpl.mozilla.org/?tree=Try&rev=7d228e4f0eee
Attachment #8416003 - Attachment is obsolete: true
Attachment #8416035 - Flags: review+
(In reply to Botond Ballo [:botond] from comment #23)
> https://tbpl.mozilla.org/?tree=Try&rev=7d228e4f0eee

Still no cigar. The call site of AsyncPanZoomController::AssertOnCompositorThread() which we need to be polymorphic for the mocking to work is in the _destructor_ (of AsyncPanZoomController, the base class), so no virtual dispatch actually happens.
For the record the reason I put this in AsyncPanZoomController than than APZCTreeManager is (1) because there's already other test stuff in AsyncPanZoomController and (2) AsyncPanZoomController is less exposed to the rest of the world so there's less chance of this getting abused for some nefarious purpose.

Try is looking pretty green: https://tbpl.mozilla.org/?tree=Try&rev=399671b09819
Attachment #8416035 - Attachment is obsolete: true
Attachment #8416498 - Flags: review?(botond)
Rebased, carrying r+
Attachment #8415988 - Attachment is obsolete: true
Attachment #8416500 - Flags: review+
Attachment #8416498 - Flags: review?(botond) → review+
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #25)
> Created attachment 8416498 [details] [diff] [review]
> Part 1 - Allow gtests to disable threading checks
> 
> For the record the reason I put this in AsyncPanZoomController than than
> APZCTreeManager is (1) because there's already other test stuff in
> AsyncPanZoomController and (2) AsyncPanZoomController is less exposed to the
> rest of the world so there's less chance of this getting abused for some
> nefarious purpose.
> 
> Try is looking pretty green:
> https://tbpl.mozilla.org/?tree=Try&rev=399671b09819

Thanks! This solution is definitely less clunky than the ICompositorUtils thing I proposed on IRC :)
https://hg.mozilla.org/mozilla-central/rev/a0424bca48cc
https://hg.mozilla.org/mozilla-central/rev/43b2f692bb54
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
feature-b2g: --- → 2.0
You need to log in before you can comment on or make changes to this bug.