Skip to content
This repository has been archived by the owner on Nov 8, 2023. It is now read-only.

Commit

Permalink
Fix SharedBuffer. Remove aref.
Browse files Browse the repository at this point in the history
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
  • Loading branch information
hboehm committed May 23, 2016
1 parent 6221295 commit 3e4c076
Show file tree
Hide file tree
Showing 3 changed files with 24 additions and 72 deletions.
58 changes: 0 additions & 58 deletions include/cutils/aref.h

This file was deleted.

17 changes: 9 additions & 8 deletions libutils/SharedBuffer.cpp
Expand Up @@ -20,7 +20,6 @@
#include <string.h>

#include <log/log.h>
#include <utils/Atomic.h>

#include "SharedBuffer.h"

Expand All @@ -37,18 +36,19 @@ SharedBuffer* SharedBuffer::alloc(size_t size)

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
Expand Down Expand Up @@ -108,14 +108,15 @@ SharedBuffer* SharedBuffer::reset(size_t new_size) const
}

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));
}
Expand Down
21 changes: 15 additions & 6 deletions libutils/SharedBuffer.h
Expand Up @@ -14,9 +14,14 @@
* limitations under the License.
*/

/*
* DEPRECATED. DO NOT USE FOR NEW CODE.
*/

#ifndef ANDROID_SHARED_BUFFER_H
#define ANDROID_SHARED_BUFFER_H

#include <atomic>
#include <stdint.h>
#include <sys/types.h>

Expand All @@ -43,7 +48,7 @@ class SharedBuffer
* In other words, the buffer must have been release by all its
* users.
*/
static ssize_t dealloc(const SharedBuffer* released);
static void dealloc(const SharedBuffer* released);

//! access the data for read
inline const void* data() const;
Expand Down Expand Up @@ -94,12 +99,16 @@ class SharedBuffer
SharedBuffer(const SharedBuffer&);
SharedBuffer& operator = (const SharedBuffer&);

// 16 bytes. must be sized to preserve correct alignment.
mutable int32_t mRefs;
size_t mSize;
uint32_t mReserved[2];
// Must be sized to preserve correct alignment.
mutable std::atomic<int32_t> mRefs;
size_t mSize;
uint32_t mReserved[2];
};

static_assert(sizeof(SharedBuffer) % 8 == 0
&& (sizeof(size_t) > 4 || sizeof(SharedBuffer) == 16),
"SharedBuffer has unexpected size");

// ---------------------------------------------------------------------------

const void* SharedBuffer::data() const {
Expand Down Expand Up @@ -127,7 +136,7 @@ size_t SharedBuffer::sizeFromData(const void* data) {
}

bool SharedBuffer::onlyOwner() const {
return (mRefs == 1);
return (mRefs.load(std::memory_order_acquire) == 1);
}

}; // namespace android
Expand Down

0 comments on commit 3e4c076

Please sign in to comment.