summaryrefslogtreecommitdiff
path: root/libs
diff options
context:
space:
mode:
authorAdam Lesinski <adamlesinski@google.com>2016-10-17 13:50:56 -0700
committerAdam Lesinski <adamlesinski@google.com>2016-10-18 01:27:39 +0000
commit2582465bb437d3700b846fe66416f1c650e9345c (patch)
treedb0b6e84410b7aef271f72d5b03aa66b61f89cf9 /libs
parentc81e1f15d28711d69f576d34741ec06faea0c4de (diff)
downloadandroid-frameworks-base-2582465bb437d3700b846fe66416f1c650e9345c.tar.gz
android-frameworks-base-2582465bb437d3700b846fe66416f1c650e9345c.tar.xz
Fix race with Asset destruction and printing allocation stats
A race could occur when printing the list of Asset allocations for debugging purposes. Each Asset object would insert themselves into a global linked list on construction and remove themselves on destruction. Iterating the list and the insertion/remove operations all acquire a global lock. The race occurs after the Asset subclass destructor runs but before the Asset base class destructor runs, which performs the actual removal from the list. The vtable of the object being destroyed ends up pointing at the base Asset class' vtable, and during the iteration of the global list, a pure virtual method is called leading to an abort, since the wrong vtable is dereferenced. This change moves the insertion/removal of the Asset object into the global list to the concrete class, which adds some maintenance overhead but solves the problem. Bug:31113965 Test: make libandroidfw_tests Change-Id: I1a620897e5e04a8519ee247883bba0719b1fa6f3 (cherry picked from commit 0358efe4f76f42d9eea91600202a5ab0831d9cef)
Diffstat (limited to 'libs')
-rw-r--r--libs/androidfw/Asset.cpp92
-rw-r--r--libs/androidfw/tests/Android.mk1
-rw-r--r--libs/androidfw/tests/Asset_test.cpp37
3 files changed, 94 insertions, 36 deletions
diff --git a/libs/androidfw/Asset.cpp b/libs/androidfw/Asset.cpp
index 4e14b13..6ae39d5 100644
--- a/libs/androidfw/Asset.cpp
+++ b/libs/androidfw/Asset.cpp
@@ -52,6 +52,47 @@ static int32_t gCount = 0;
static Asset* gHead = NULL;
static Asset* gTail = NULL;
+void Asset::registerAsset(Asset* asset)
+{
+ AutoMutex _l(gAssetLock);
+ gCount++;
+ asset->mNext = asset->mPrev = NULL;
+ if (gTail == NULL) {
+ gHead = gTail = asset;
+ } else {
+ asset->mPrev = gTail;
+ gTail->mNext = asset;
+ gTail = asset;
+ }
+
+ if (kIsDebug) {
+ ALOGI("Creating Asset %p #%d\n", asset, gCount);
+ }
+}
+
+void Asset::unregisterAsset(Asset* asset)
+{
+ AutoMutex _l(gAssetLock);
+ gCount--;
+ if (gHead == asset) {
+ gHead = asset->mNext;
+ }
+ if (gTail == asset) {
+ gTail = asset->mPrev;
+ }
+ if (asset->mNext != NULL) {
+ asset->mNext->mPrev = asset->mPrev;
+ }
+ if (asset->mPrev != NULL) {
+ asset->mPrev->mNext = asset->mNext;
+ }
+ asset->mNext = asset->mPrev = NULL;
+
+ if (kIsDebug) {
+ ALOGI("Destroying Asset in %p #%d\n", asset, gCount);
+ }
+}
+
int32_t Asset::getGlobalCount()
{
AutoMutex _l(gAssetLock);
@@ -79,43 +120,8 @@ String8 Asset::getAssetAllocations()
}
Asset::Asset(void)
- : mAccessMode(ACCESS_UNKNOWN)
+ : mAccessMode(ACCESS_UNKNOWN), mNext(NULL), mPrev(NULL)
{
- AutoMutex _l(gAssetLock);
- gCount++;
- mNext = mPrev = NULL;
- if (gTail == NULL) {
- gHead = gTail = this;
- } else {
- mPrev = gTail;
- gTail->mNext = this;
- gTail = this;
- }
- if (kIsDebug) {
- ALOGI("Creating Asset %p #%d\n", this, gCount);
- }
-}
-
-Asset::~Asset(void)
-{
- AutoMutex _l(gAssetLock);
- gCount--;
- if (gHead == this) {
- gHead = mNext;
- }
- if (gTail == this) {
- gTail = mPrev;
- }
- if (mNext != NULL) {
- mNext->mPrev = mPrev;
- }
- if (mPrev != NULL) {
- mPrev->mNext = mNext;
- }
- mNext = mPrev = NULL;
- if (kIsDebug) {
- ALOGI("Destroying Asset in %p #%d\n", this, gCount);
- }
}
/*
@@ -361,6 +367,9 @@ off64_t Asset::handleSeek(off64_t offset, int whence, off64_t curPosn, off64_t m
_FileAsset::_FileAsset(void)
: mStart(0), mLength(0), mOffset(0), mFp(NULL), mFileName(NULL), mMap(NULL), mBuf(NULL)
{
+ // Register the Asset with the global list here after it is fully constructed and its
+ // vtable pointer points to this concrete type. b/31113965
+ registerAsset(this);
}
/*
@@ -369,6 +378,10 @@ _FileAsset::_FileAsset(void)
_FileAsset::~_FileAsset(void)
{
close();
+
+ // Unregister the Asset from the global list here before it is destructed and while its vtable
+ // pointer still points to this concrete type. b/31113965
+ unregisterAsset(this);
}
/*
@@ -685,6 +698,9 @@ _CompressedAsset::_CompressedAsset(void)
: mStart(0), mCompressedLen(0), mUncompressedLen(0), mOffset(0),
mMap(NULL), mFd(-1), mZipInflater(NULL), mBuf(NULL)
{
+ // Register the Asset with the global list here after it is fully constructed and its
+ // vtable pointer points to this concrete type. b/31113965
+ registerAsset(this);
}
/*
@@ -693,6 +709,10 @@ _CompressedAsset::_CompressedAsset(void)
_CompressedAsset::~_CompressedAsset(void)
{
close();
+
+ // Unregister the Asset from the global list here before it is destructed and while its vtable
+ // pointer still points to this concrete type. b/31113965
+ unregisterAsset(this);
}
/*
diff --git a/libs/androidfw/tests/Android.mk b/libs/androidfw/tests/Android.mk
index 2bc026b..1fe1773 100644
--- a/libs/androidfw/tests/Android.mk
+++ b/libs/androidfw/tests/Android.mk
@@ -22,6 +22,7 @@ LOCAL_PATH:= $(call my-dir)
testFiles := \
AppAsLib_test.cpp \
+ Asset_test.cpp \
AttributeFinder_test.cpp \
ByteBucketArray_test.cpp \
Config_test.cpp \
diff --git a/libs/androidfw/tests/Asset_test.cpp b/libs/androidfw/tests/Asset_test.cpp
new file mode 100644
index 0000000..45c8cef
--- /dev/null
+++ b/libs/androidfw/tests/Asset_test.cpp
@@ -0,0 +1,37 @@
+/*
+ * Copyright (C) 2016 The Android Open Source Project
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+#include <androidfw/Asset.h>
+
+#include <gtest/gtest.h>
+
+using namespace android;
+
+TEST(AssetTest, FileAssetRegistersItself) {
+ const int32_t count = Asset::getGlobalCount();
+ Asset* asset = new _FileAsset();
+ EXPECT_EQ(count + 1, Asset::getGlobalCount());
+ delete asset;
+ EXPECT_EQ(count, Asset::getGlobalCount());
+}
+
+TEST(AssetTest, CompressedAssetRegistersItself) {
+ const int32_t count = Asset::getGlobalCount();
+ Asset* asset = new _CompressedAsset();
+ EXPECT_EQ(count + 1, Asset::getGlobalCount());
+ delete asset;
+ EXPECT_EQ(count, Asset::getGlobalCount());
+}