commit 54828dc739e361a795000e8de79d954db3dcbef8
author Hans Boehm <hboehm@google.com> Wed May 18 10:09:24 2016 -0700
committer Hans Boehm <hboehm@google.com> Mon May 23 15:19:53 2016 -0700
tree 68c6f63b335e61614e64d9c327935a8b296b583b
parent 70a46d674a04e95da633a5914abd7a55a46e2b3e

Fix SharedBuffer. Remove aref.

Add comment that SharedBuffer is deprecated.

Both aref and SharedBuffer had memory ordering bugs.  Aref has no
clients.

SharedBuffer had several bugs, which are fixed here:

mRefs was declared neither volatile, not atomic, allowing the
compiler to, for example, reuse a stale previously loaded value.

It used the default android_atomic release memory ordering, which
is insufficient for reference count decrements.

It used an ordinary memory read in onlyOwner() to check whether
an object is safe to deallocate, without any attempt to ensure
memory ordering.

Comments claimed that SharedBuffer was exactly 16 bytes, but
this was neither checked, nor correct on 64-bit platforms.

This turns mRef into a std::atomic and removes the android_atomic
dependency.

Bug: 28826227
Change-Id: I39fa0b4f70ac0471b14ad274806fc4e0c0802e78
(cherry picked from commit 3e4c076ef204c4b572d02bd1c8dbf8c599e0014d)

libutils/SharedBuffer.cpp

diff --git a/libutils/SharedBuffer.cpp b/libutils/SharedBuffer.cpp
index 34d75ee..a8a9fb4 100644
--- a/libutils/SharedBuffer.cpp
+++ b/libutils/SharedBuffer.cpp
@@ -18,7 +18,6 @@
 #include <string.h>
 
 #include <log/log.h>
-#include <utils/Atomic.h>
 
 #include "SharedBuffer.h"
 
@@ -35,18 +34,19 @@
 
     SharedBuffer* sb = static_cast<SharedBuffer *>(malloc(sizeof(SharedBuffer) + size));
     if (sb) {
-        sb->mRefs = 1;
+        // Should be std::atomic_init(&sb->mRefs, 1);
+        // But that generates a warning with some compilers.
+        // The following is OK on Android-supported platforms.
+        sb->mRefs.store(1, std::memory_order_relaxed);
         sb->mSize = size;
     }
     return sb;
 }
 
 
-ssize_t SharedBuffer::dealloc(const SharedBuffer* released)
+void SharedBuffer::dealloc(const SharedBuffer* released)
 {
-    if (released->mRefs != 0) return -1; // XXX: invalid operation
     free(const_cast<SharedBuffer*>(released));
-    return 0;
 }
 
 SharedBuffer* SharedBuffer::edit() const
@@ -106,14 +106,15 @@
 }
 
 void SharedBuffer::acquire() const {
-    android_atomic_inc(&mRefs);
+    mRefs.fetch_add(1, std::memory_order_relaxed);
 }
 
 int32_t SharedBuffer::release(uint32_t flags) const
 {
     int32_t prev = 1;
-    if (onlyOwner() || ((prev = android_atomic_dec(&mRefs)) == 1)) {
-        mRefs = 0;
+    if (onlyOwner() || ((prev = mRefs.fetch_sub(1, std::memory_order_release) == 1)
+            && (atomic_thread_fence(std::memory_order_acquire), true))) {
+        mRefs.store(0, std::memory_order_relaxed);
         if ((flags & eKeepStorage) == 0) {
             free(const_cast<SharedBuffer*>(this));
         }