Closed
Bug 1002754
Opened 10 years ago
Closed 10 years ago
APZC destructor has a use-after-free hazard when progressive painting is enabled
Categories
(Core :: Panning and Zooming, defect)
Tracking
()
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?
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(rbarker)
Comment 1•10 years ago
|
||
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)
Assignee | ||
Comment 2•10 years ago
|
||
Assignee: nobody → bugmail.mozilla
Attachment #8414159 -
Flags: review?(rbarker)
Attachment #8414159 -
Flags: review?(botond)
Comment 3•10 years ago
|
||
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+
Assignee | ||
Comment 4•10 years ago
|
||
(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.
Assignee | ||
Comment 5•10 years ago
|
||
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 6•10 years ago
|
||
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?
Assignee | ||
Comment 7•10 years ago
|
||
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.
Comment 8•10 years ago
|
||
(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?
Assignee | ||
Comment 9•10 years ago
|
||
Doh, good point. You're right, we can just get rid of it. /me missed the big picture
Assignee | ||
Comment 10•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8415498 -
Flags: review?(rbarker) → review+
Updated•10 years ago
|
Attachment #8415498 -
Flags: review?(rbarker)
Attachment #8415498 -
Flags: review?(botond)
Attachment #8415498 -
Flags: review+
Comment 11•10 years ago
|
||
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+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 12•10 years ago
|
||
landing |
https://hg.mozilla.org/integration/mozilla-inbound/rev/01dd7c0d8f8d
Keywords: checkin-needed
Comment 13•10 years ago
|
||
backout |
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.
Comment 14•10 years ago
|
||
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.
Comment 15•10 years ago
|
||
Attachment #8415985 -
Flags: review?(bugmail.mozilla)
Comment 16•10 years ago
|
||
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+
Comment 17•10 years ago
|
||
try |
(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
Comment 18•10 years ago
|
||
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)
Comment 19•10 years ago
|
||
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)
Assignee | ||
Comment 20•10 years ago
|
||
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+
Comment 21•10 years ago
|
||
(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 :)
Comment 22•10 years ago
|
||
failure try |
https://tbpl.mozilla.org/?tree=Try&rev=e72b12b45d73
Comment 23•10 years ago
|
||
failure try |
(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+
Comment 24•10 years ago
|
||
(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.
Assignee | ||
Comment 25•10 years ago
|
||
green try |
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)
Assignee | ||
Comment 26•10 years ago
|
||
Rebased, carrying r+
Attachment #8415988 -
Attachment is obsolete: true
Attachment #8416500 -
Flags: review+
Updated•10 years ago
|
Attachment #8416498 -
Flags: review?(botond) → review+
Comment 27•10 years ago
|
||
(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 :)
Assignee | ||
Comment 28•10 years ago
|
||
remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/a0424bca48cc remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/43b2f692bb54
Comment 29•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/a0424bca48cc https://hg.mozilla.org/mozilla-central/rev/43b2f692bb54
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
Updated•10 years ago
|
feature-b2g: --- → 2.0
Updated•7 years ago
|
Keywords: csectype-uaf
You need to log in
before you can comment on or make changes to this bug.
Description
•