From 3e4c076ef204c4b572d02bd1c8dbf8c599e0014d Mon Sep 17 00:00:00 2001 From: Hans Boehm Date: Wed, 18 May 2016 10:09:24 -0700 Subject: [PATCH] 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 --- include/cutils/aref.h | 58 --------------------------------------- libutils/SharedBuffer.cpp | 17 ++++++------ libutils/SharedBuffer.h | 21 ++++++++++---- 3 files changed, 24 insertions(+), 72 deletions(-) delete mode 100644 include/cutils/aref.h diff --git a/include/cutils/aref.h b/include/cutils/aref.h deleted file mode 100644 index 3bd36ea04..000000000 --- a/include/cutils/aref.h +++ /dev/null @@ -1,58 +0,0 @@ -/* - * Copyright (C) 2013 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. - */ - -#ifndef _CUTILS_AREF_H_ -#define _CUTILS_AREF_H_ - -#include -#include - -#include - -__BEGIN_DECLS - -#define AREF_TO_ITEM(aref, container, member) \ - (container *) (((char*) (aref)) - offsetof(container, member)) - -struct aref -{ - volatile int32_t count; -}; - -static inline void aref_init(struct aref *r) -{ - r->count = 1; -} - -static inline int32_t aref_count(struct aref *r) -{ - return r->count; -} - -static inline void aref_get(struct aref *r) -{ - android_atomic_inc(&r->count); -} - -static inline void aref_put(struct aref *r, void (*release)(struct aref *)) -{ - if (android_atomic_dec(&r->count) == 1) - release(r); -} - -__END_DECLS - -#endif // _CUTILS_AREF_H_ diff --git a/libutils/SharedBuffer.cpp b/libutils/SharedBuffer.cpp index c7dd1ab34..f3d6d8f6b 100644 --- a/libutils/SharedBuffer.cpp +++ b/libutils/SharedBuffer.cpp @@ -20,7 +20,6 @@ #include #include -#include #include "SharedBuffer.h" @@ -37,18 +36,19 @@ SharedBuffer* SharedBuffer::alloc(size_t size) SharedBuffer* sb = static_cast(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(released)); - return 0; } SharedBuffer* SharedBuffer::edit() const @@ -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(this)); } diff --git a/libutils/SharedBuffer.h b/libutils/SharedBuffer.h index b6709537e..48358cddc 100644 --- a/libutils/SharedBuffer.h +++ b/libutils/SharedBuffer.h @@ -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 #include #include @@ -43,7 +48,7 @@ public: * 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; @@ -94,12 +99,16 @@ private: 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 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 { @@ -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