New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 0 users
Status: Fixed
Owner:
Email to this user bounced
Closed: Sep 2014
Cc:
EstimatedDays: ----
NextAction: ----
OS: Android
Pri: 1
Type: Bug-Security
Nag



Sign in to add a comment
Heap-use-after-free in cc::LayerTreeHost::RecreateUIResources
Project Member Reported by clusterf...@chromium.org, Aug 24 2014 Back to list
Detailed report: https://cluster-fuzz.appspot.com/testcase?key=6445887766134784

Fuzzer: Marty_html_twiddler
Job Type: Android_asan_chrome

Crash Type: Heap-use-after-free READ 4
Crash Address: 0x98c68660
Crash State:
  cc::LayerTreeHost::RecreateUIResources
  cc::SingleThreadProxy::DoCommit
  cc::SingleThreadProxy::CompositeImmediately
  

Unminimized Testcase: https://cluster-fuzz.appspot.com/download/AMIfv97vQ5A2v4Pj6lOokS0ubUTTU1zcwpwFw7uzzhoxE4O54uj9uwPeZrZC-YX-LOju1poyCGb7U4WREv6YSZ0mjPY1p2WqiZHCsbqFPujwHRgIHYfdwCuhVarBJabf1Y3GpCTESLuugaQH4ED6eOjO13BcerDSEmrpDqv5C9RoiVukwUdyZdk


Additional requirements: Requires HTTP

Filer: inferno
 
Cc: feng@chromium.org
Owner: danakj@chromium.org
Status: Assigned
Project Member Comment 2 by clusterf...@chromium.org, Aug 24 2014
Labels: Pri-1
Comment 3 by danakj@chromium.org, Aug 24 2014
Cc: aelias@chromium.org danakj@chromium.org enne@chromium.org
Owner: boliu@chromium.org
Can you take a look Bo?
Comment 4 by aelias@chromium.org, Aug 24 2014
Owner: aelias@chromium.org
This isn't WebView-related, so reassigning.
Comment 5 by danakj@chromium.org, Aug 25 2014
Cc: boliu@chromium.org
Comment 6 by aelias@chromium.org, Aug 26 2014
Owner: siev...@chromium.org
Seems similar to https://code.google.com/p/chromium/issues/detail?id=367867 in that the log reports three failed context creation attempts.  Maybe we should hard checkfail in this case to avoid the security issue of reading invalid memory?
Labels: Security_Impact-Stable
Project Member Comment 8 by clusterf...@chromium.org, Aug 26 2014
Labels: M-37
Project Member Comment 9 by clusterf...@chromium.org, Sep 2 2014
Labels: Nag
sievers@: Uh oh! This issue is still open and hasn't been updated in the last 7 days. Since this is a serious security vulnerability, we want to make sure progress is happening. Can you update the bug with current status, and what, if anything, is blocking?

If you are not the right Owner for this bug, please find someone else to own it as soon as possible and remove yourself as Owner.

If the issue is already fixed or you are to unable to reproduce it, please close the bug. (And thanks for fixing the bug!).

These nags can be disabled by adding a 'WIP' label and an optional codereview link.

- Your friendly ClusterFuzz
Cc: dtrainor@chromium.org jdduke@chromium.org
Labels: -OS-All OS-Android
I think I see the bug:

In Chrome for Android ResourceManager::OnResourceReady() will replace any existing UIResourceClient that it owns (for a given id) with a new one. The crash is from a LayerTreeHost referencing the old one after it was replaced.

Looks like there is a misunderstanding between the two different call sites for UIResourcesAreInvalid(). If the LTH changed (old one was torn down because the app was hidden), then we don't have to call LTH to delete the invalid resource. But if the context was lost, the UI resource stays around and UIResourceClient::GetBitmap() is called again.

There is probably a simple bandaid for this (by letting the android compositor/ui resource manager delete all resources when the context is lost). But it could also probably be simplified and made more robust, since we have all these abstractions layers for Android which would allow us to make resources persistent, but it then ends up exposing the externals (cc::UIResourceId and cc::UIResourceClient).

Android-specific so changing from OS-All to OS-Android.
Ok after discussion with dtrainor will try a minimal fix and just not call UIResourcesAreInvalid (which causes all the badness) when the context was lost.

LTH will call GetBitmap() for the existing resource client, which should already work for 2 of the 3 resource client types (the ones with persistent bitmaps). But thumbnail.cc needs a minor fix in GetBitmap() to cause it to be reloaded, since it throws away the bitmap after first use to save memory.

Ok I couldn't get the Thumbnail invalidation fix to work, because the interaction between and lifetime of the pieces is a bit whacky. So will fix the crash only for now, which is a trivial change.
Project Member Comment 13 by bugdroid1@chromium.org, Sep 4 2014
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f5186515e0607b24d77a0bca13634f0158865b8e

commit f5186515e0607b24d77a0bca13634f0158865b8e
Author: sievers <sievers@chromium.org>
Date: Thu Sep 04 02:51:45 2014

android: Don't invalidate UI resources if context is lost

This callback implies that the resources are gone (because LTH
was deleted). What ends up happening is that the resources get
deleted while LTH still tries to call GetBitmap() on them.

This still needs some working invalidation mechanism for Thumbnails,
which don't have a persistent Bitmap they return from GetBitmap().

BUG= 406879 
NOTRY=True
TBR=dtrainor@chromium.org

Review URL: https://codereview.chromium.org/543543002

Cr-Commit-Position: refs/heads/master@{#293252}

[modify] https://chromium.googlesource.com/chromium/src.git/+/f5186515e0607b24d77a0bca13634f0158865b8e/content/browser/renderer_host/compositor_impl_android.cc

Labels: Merge-Requested
Labels: -M-37 M-38
Status: Fixed
No more M37 patches, requesting for M38.
Project Member Comment 16 by clusterf...@chromium.org, Sep 4 2014
Labels: -Restrict-View-SecurityTeam Restrict-View-SecurityNotify
Labels: -Merge-Requested Merge-Approved
Project Member Comment 18 by bugdroid1@chromium.org, Sep 5 2014
Labels: -Merge-Approved merge-merged-2125
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/201c5eb067d67b2aa05436ce5ef616e42abec908

commit 201c5eb067d67b2aa05436ce5ef616e42abec908
Author: Daniel Sievers <sievers@chromium.org>
Date: Fri Sep 05 17:49:10 2014

android: Don't invalidate UI resources if context is lost

This callback implies that the resources are gone (because LTH
was deleted). What ends up happening is that the resources get
deleted while LTH still tries to call GetBitmap() on them.

This still needs some working invalidation mechanism for Thumbnails,
which don't have a persistent Bitmap they return from GetBitmap().

BUG= 406879 
NOTRY=True
TBR=dtrainor@chromium.org

Review URL: https://codereview.chromium.org/543543002

Cr-Commit-Position: refs/heads/master@{#293252}
(cherry picked from commit f5186515e0607b24d77a0bca13634f0158865b8e)

Review URL: https://codereview.chromium.org/542163003

Cr-Commit-Position: refs/branch-heads/2125@{#242}
Cr-Branched-From: b68026d94bda36dd106a3d91a098719f952a9477-refs/heads/master@{#290040}

[modify] https://chromium.googlesource.com/chromium/src.git/+/201c5eb067d67b2aa05436ce5ef616e42abec908/content/browser/renderer_host/compositor_impl_android.cc

Labels: Release-0-M38
Project Member Comment 20 by clusterf...@chromium.org, Dec 11 2014
Labels: -Restrict-View-SecurityNotify
Bulk update: removing view restriction from closed bugs.
Project Member Comment 21 by sheriffbot@chromium.org, Oct 1 2016
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Project Member Comment 22 by sheriffbot@chromium.org, Oct 2 2016
This bug has been closed for more than 14 weeks. Removing security view restrictions.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: allpublic
Sign in to add a comment