From 7f0b2601d37e758e007f53f776889ae362fc246e Mon Sep 17 00:00:00 2001
From: Hans Boehm <hboehm@google.com>
Date: Wed, 15 Mar 2017 11:01:03 -0700
Subject: [PATCH] Add heuristic data race detection to sp<>

Force assignment to read the old pointer value twice, and check
that it didn't change in the interim. Previous experience with
Skia suggests that this has a high probability of correctly detecting
a data race when it occurs, instead of potentially letting the
count associated with the old pointer value get decremented twice,
and corrupting the heap.

This does increase the size of sp assignments, which seem to
commonly get inlined. For the general case, we add a third
comparison and function call.

Some code reformatting to make this consistent with modern conventions
and pass automated checks.

Test: Booted aosp build. Ran libutils tests. Looked at generated code.

Bug: 31227650

Change-Id: Id93a05c6bf10f01ee15ff1bb409611f2058f988f
---
 libutils/Android.bp                    |  1 +
 libutils/StrongPointer.cpp             | 24 ++++++++++++++
 libutils/include/utils/StrongPointer.h | 46 +++++++++++++++-----------
 3 files changed, 51 insertions(+), 20 deletions(-)
 create mode 100644 libutils/StrongPointer.cpp

diff --git a/libutils/Android.bp b/libutils/Android.bp
index 2b98fef98..2b7412b4c 100644
--- a/libutils/Android.bp
+++ b/libutils/Android.bp
@@ -45,6 +45,7 @@ cc_library {
         "StopWatch.cpp",
         "String8.cpp",
         "String16.cpp",
+        "StrongPointer.cpp",
         "SystemClock.cpp",
         "Threads.cpp",
         "Timers.cpp",
diff --git a/libutils/StrongPointer.cpp b/libutils/StrongPointer.cpp
new file mode 100644
index 000000000..ba52502ec
--- /dev/null
+++ b/libutils/StrongPointer.cpp
@@ -0,0 +1,24 @@
+/*
+ * Copyright (C) 2017 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.
+ */
+
+#define LOG_TAG "sp"
+
+#include <log/log.h>
+
+namespace android {
+
+void sp_report_race() { LOG_ALWAYS_FATAL("sp<> assignment detected data race"); }
+}
diff --git a/libutils/include/utils/StrongPointer.h b/libutils/include/utils/StrongPointer.h
index 294e6b6f4..c2f772222 100644
--- a/libutils/include/utils/StrongPointer.h
+++ b/libutils/include/utils/StrongPointer.h
@@ -108,6 +108,9 @@ private:
     T* m_ptr;
 };
 
+// For code size reasons, we do not want this inlined or templated.
+void sp_report_race();
+
 #undef COMPARE
 
 // ---------------------------------------------------------------------------
@@ -161,19 +164,21 @@ sp<T>::~sp() {
 
 template<typename T>
 sp<T>& sp<T>::operator =(const sp<T>& other) {
+    // Force m_ptr to be read twice, to heuristically check for data races.
+    T* oldPtr(*const_cast<T* volatile*>(&m_ptr));
     T* otherPtr(other.m_ptr);
-    if (otherPtr)
-        otherPtr->incStrong(this);
-    if (m_ptr)
-        m_ptr->decStrong(this);
+    if (otherPtr) otherPtr->incStrong(this);
+    if (oldPtr) oldPtr->decStrong(this);
+    if (oldPtr != *const_cast<T* volatile*>(&m_ptr)) sp_report_race();
     m_ptr = otherPtr;
     return *this;
 }
 
 template<typename T>
 sp<T>& sp<T>::operator =(sp<T>&& other) {
-    if (m_ptr)
-        m_ptr->decStrong(this);
+    T* oldPtr(*const_cast<T* volatile*>(&m_ptr));
+    if (oldPtr) oldPtr->decStrong(this);
+    if (oldPtr != *const_cast<T* volatile*>(&m_ptr)) sp_report_race();
     m_ptr = other.m_ptr;
     other.m_ptr = nullptr;
     return *this;
@@ -181,29 +186,30 @@ sp<T>& sp<T>::operator =(sp<T>&& other) {
 
 template<typename T>
 sp<T>& sp<T>::operator =(T* other) {
-    if (other)
-        other->incStrong(this);
-    if (m_ptr)
-        m_ptr->decStrong(this);
+    T* oldPtr(*const_cast<T* volatile*>(&m_ptr));
+    if (other) other->incStrong(this);
+    if (oldPtr) oldPtr->decStrong(this);
+    if (oldPtr != *const_cast<T* volatile*>(&m_ptr)) sp_report_race();
     m_ptr = other;
     return *this;
 }
 
 template<typename T> template<typename U>
 sp<T>& sp<T>::operator =(const sp<U>& other) {
+    T* oldPtr(*const_cast<T* volatile*>(&m_ptr));
     T* otherPtr(other.m_ptr);
-    if (otherPtr)
-        otherPtr->incStrong(this);
-    if (m_ptr)
-        m_ptr->decStrong(this);
+    if (otherPtr) otherPtr->incStrong(this);
+    if (oldPtr) oldPtr->decStrong(this);
+    if (oldPtr != *const_cast<T* volatile*>(&m_ptr)) sp_report_race();
     m_ptr = otherPtr;
     return *this;
 }
 
 template<typename T> template<typename U>
 sp<T>& sp<T>::operator =(sp<U>&& other) {
-    if (m_ptr)
-        m_ptr->decStrong(this);
+    T* oldPtr(*const_cast<T* volatile*>(&m_ptr));
+    if (m_ptr) m_ptr->decStrong(this);
+    if (oldPtr != *const_cast<T* volatile*>(&m_ptr)) sp_report_race();
     m_ptr = other.m_ptr;
     other.m_ptr = nullptr;
     return *this;
@@ -211,10 +217,10 @@ sp<T>& sp<T>::operator =(sp<U>&& other) {
 
 template<typename T> template<typename U>
 sp<T>& sp<T>::operator =(U* other) {
-    if (other)
-        (static_cast<T*>(other))->incStrong(this);
-    if (m_ptr)
-        m_ptr->decStrong(this);
+    T* oldPtr(*const_cast<T* volatile*>(&m_ptr));
+    if (other) (static_cast<T*>(other))->incStrong(this);
+    if (oldPtr) oldPtr->decStrong(this);
+    if (oldPtr != *const_cast<T* volatile*>(&m_ptr)) sp_report_race();
     m_ptr = other;
     return *this;
 }