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

4 files changed, 108 insertions, 37 deletions

diff --git a/include/androidfw/Asset.h b/include/androidfw/Asset.h
index ee77e971011b..52c863774efb 100644
--- a/include/androidfw/Asset.h
+++ b/include/androidfw/Asset.h
@@ -44,7 +44,7 @@ namespace android {
  */
 class Asset {
 public:
-    virtual ~Asset(void);
+    virtual ~Asset(void) = default;
 
     static int32_t getGlobalCount();
     static String8 getAssetAllocations();
@@ -119,6 +119,19 @@ public:
     const char* getAssetSource(void) const { return mAssetSource.string(); }
 
 protected:
+    /*
+     * Adds this Asset to the global Asset list for debugging and
+     * accounting.
+     * Concrete subclasses must call this in their constructor.
+     */
+    static void registerAsset(Asset* asset);
+
+    /*
+     * Removes this Asset from the global Asset list.
+     * Concrete subclasses must call this in their destructor.
+     */
+    static void unregisterAsset(Asset* asset);
+
     Asset(void);        // constructor; only invoked indirectly
 
     /* handle common seek() housekeeping */
diff --git a/libs/androidfw/Asset.cpp b/libs/androidfw/Asset.cpp
index 2cfa6666e9ab..8e8c6a2e25a2 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 2bc026b79ca2..1fe1773578fa 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 000000000000..45c8cef92918
--- /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());
+}