Scarfski/filament
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Better handle collisions and use-after-free detection

Browse Source 
- each handle now has a 4-bits "age", meaning that handles are recycled
  only after 16 alloc/free cycles.
  This is used to detect double-free and use-after free.
  This should also allow us to compare handles, because freeing and
  reallocating an object, won't produce the same Handle (at least
  for 16 rounds).

- removed "type safety" checks because it's almost impossible to
  get it wrong thanks to our compile time type safety checks. This
  didn't provide a useful value added.

- This feature is built on top of being able to set/get a 8 bits tag
  associated with the memory block returned by the pool allocator. We
  use the "extra" parameter of the allocator to allocate a "hidden"
  structure containing the age of that memory block.

- Also we don't allow to compare Handle<> of different types
This commit is contained in:
Mathias Agopian
2024-02-06 10:03:07 -08:00
committed by Mathias Agopian
parent f9c8e65ef3
commit ef703bb4be
4 changed files with 157 additions and 117 deletions
Download Patch File Download Diff File Expand all files Collapse all files

16
filament/backend/include/backend/Handle.h
View File

@@ -62,14 +62,6 @@ public:
// clear the handle, this doesn't free associated resources
void clear() noexcept { object = nullid; }
// compare handles
bool operator==(const HandleBase& rhs) const noexcept { return object == rhs.object; }
bool operator!=(const HandleBase& rhs) const noexcept { return object != rhs.object; }
bool operator<(const HandleBase& rhs) const noexcept { return object < rhs.object; }
bool operator<=(const HandleBase& rhs) const noexcept { return object <= rhs.object; }
bool operator>(const HandleBase& rhs) const noexcept { return object > rhs.object; }
bool operator>=(const HandleBase& rhs) const noexcept { return object >= rhs.object; }
// get this handle's handleId
HandleId getId() const noexcept { return object; }
@@ -101,6 +93,14 @@ struct Handle : public HandleBase {
explicit Handle(HandleId id) noexcept : HandleBase(id) { }
// compare handles of the same type
bool operator==(const Handle& rhs) const noexcept { return getId() == rhs.getId(); }
bool operator!=(const Handle& rhs) const noexcept { return getId() != rhs.getId(); }
bool operator<(const Handle& rhs) const noexcept { return getId() < rhs.getId(); }
bool operator<=(const Handle& rhs) const noexcept { return getId() <= rhs.getId(); }
bool operator>(const Handle& rhs) const noexcept { return getId() > rhs.getId(); }
bool operator>=(const Handle& rhs) const noexcept { return getId() >= rhs.getId(); }
// type-safe Handle cast
template<typename B, typename = std::enable_if_t<std::is_base_of<T, B>::value> >
Handle(Handle<B> const& base) noexcept : HandleBase(base) { } // NOLINT(hicpp-explicit-conversions,google-explicit-constructor)

177
filament/backend/include/private/backend/HandleAllocator.h
View File

@@ -24,22 +24,19 @@
#include <utils/compiler.h>
#include <utils/debug.h>
#include <utils/ostream.h>
#include <utils/Panic.h>
#include <tsl/robin_map.h>
#include <cstddef>
#include <exception>
#include <type_traits>
#include <unordered_map>
#include <utility>
#include <stddef.h>
#include <stdint.h>
#if !defined(NDEBUG) && UTILS_HAS_RTTI
# define HANDLE_TYPE_SAFETY 1
#else
# define HANDLE_TYPE_SAFETY 0
#endif
#define HandleAllocatorGL HandleAllocator<16, 64, 208> // ~3640 / pool / MiB
#define HandleAllocatorVK HandleAllocator<80, 176, 320> // ~1820 / pool / MiB
#define HandleAllocatorMTL HandleAllocator<48, 160, 592> // ~1310 / pool / MiB
@@ -52,11 +49,6 @@ namespace filament::backend {
template<size_t P0, size_t P1, size_t P2>
class HandleAllocator {
public:
static_assert(P0 % 16 == 0, "HandleAllocator Pools must be multiple of 16 bytes");
static_assert(P1 % 16 == 0, "HandleAllocator Pools must be multiple of 16 bytes");
static_assert(P2 % 16 == 0, "HandleAllocator Pools must be multiple of 16 bytes");
HandleAllocator(const char* name, size_t size) noexcept;
HandleAllocator(HandleAllocator const& rhs) = delete;
HandleAllocator& operator=(HandleAllocator const& rhs) = delete;
@@ -74,14 +66,9 @@ public:
*/
template<typename D, typename ... ARGS>
Handle<D> allocateAndConstruct(ARGS&& ... args) noexcept {
Handle<D> h{ allocateHandle<sizeof(D)>() };
Handle<D> h{ allocateHandle<D>() };
D* addr = handle_cast<D*>(h);
new(addr) D(std::forward<ARGS>(args)...);
#if HANDLE_TYPE_SAFETY
mLock.lock();
mHandleTypeId[addr] = typeid(D).name();
mLock.unlock();
#endif
return h;
}
@@ -97,13 +84,7 @@ public:
*/
template<typename D>
Handle<D> allocate() noexcept {
Handle<D> h{ allocateHandle<sizeof(D)>() };
#if HANDLE_TYPE_SAFETY
D* addr = handle_cast<D*>(h);
mLock.lock();
mHandleTypeId[addr] = typeid(D).name();
mLock.unlock();
#endif
Handle<D> h{ allocateHandle<D>() };
return h;
}
@@ -120,17 +101,10 @@ public:
assert_invariant(handle);
D* addr = handle_cast<D*>(const_cast<Handle<B>&>(handle));
assert_invariant(addr);
// currently we implement construct<> with dtor+ctor, we could use operator= also
// but all our dtors are trivial, ~D() is actually a noop.
addr->~D();
new(addr) D(std::forward<ARGS>(args)...);
#if HANDLE_TYPE_SAFETY
mLock.lock();
mHandleTypeId[addr] = typeid(D).name();
mLock.unlock();
#endif
return addr;
}
@@ -147,12 +121,6 @@ public:
D* addr = handle_cast<D*>(const_cast<Handle<B>&>(handle));
assert_invariant(addr);
new(addr) D(std::forward<ARGS>(args)...);
#if HANDLE_TYPE_SAFETY
mLock.lock();
mHandleTypeId[addr] = typeid(D).name();
mLock.unlock();
#endif
return addr;
}
@@ -168,19 +136,8 @@ public:
void deallocate(Handle<B>& handle, D const* p) noexcept {
// allow to destroy the nullptr, similarly to operator delete
if (p) {
#if HANDLE_TYPE_SAFETY
mLock.lock();
auto typeId = mHandleTypeId[p];
mHandleTypeId.erase(p);
mLock.unlock();
if (UTILS_UNLIKELY(typeId != typeid(D).name())) {
utils::slog.e << "Destroying handle " << handle.getId() << ", type " << typeid(D).name()
<< ", but handle's actual type is " << typeId << utils::io::endl;
std::terminate();
}
#endif
p->~D();
deallocateHandle<sizeof(D)>(handle.getId());
deallocateHandle<D>(handle.getId());
}
}
@@ -208,7 +165,17 @@ public:
std::is_base_of_v<B, typename std::remove_pointer_t<Dp>>, Dp>
handle_cast(Handle<B>& handle) noexcept {
assert_invariant(handle);
void* const p = handleToPointer(handle.getId());
auto [p, tag] = handleToPointer(handle.getId());
if (isPoolHandle(handle.getId())) {
// check for use after free
uint8_t const age = (tag & HANDLE_AGE_MASK) >> HANDLE_AGE_SHIFT;
auto const pNode = static_cast<typename Allocator::Node*>(p);
uint8_t const expectedAge = pNode[-1].age;
ASSERT_POSTCONDITION(expectedAge == age,
"use-after-free of Handle with id=%d", handle.getId());
}
return static_cast<Dp>(p);
}
@@ -223,29 +190,57 @@ public:
private:
// template <int P0, int P1, int P2>
template<typename D>
static constexpr size_t getBucketSize() noexcept {
if constexpr (sizeof(D) <= P0) { return P0; }
if constexpr (sizeof(D) <= P1) { return P1; }
static_assert(sizeof(D) <= P2);
return P2;
}
class Allocator {
friend class HandleAllocator;
utils::PoolAllocator<P0, 16> mPool0;
utils::PoolAllocator<P1, 16> mPool1;
utils::PoolAllocator<P2, 16> mPool2;
static constexpr size_t MIN_ALIGNMENT = alignof(std::max_align_t);
struct Node { uint8_t age; };
// Note: using the `extra` parameter of PoolAllocator<>, even with a 1-byte structure,
// generally increases all pool allocations by 8-bytes because of alignment restrictions.
template<size_t SIZE>
using Pool = utils::PoolAllocator<SIZE, MIN_ALIGNMENT, sizeof(Node)>;
Pool<P0> mPool0;
Pool<P1> mPool1;
Pool<P2> mPool2;
UTILS_UNUSED_IN_RELEASE const utils::AreaPolicy::HeapArea& mArea;
public:
static constexpr size_t MIN_ALIGNMENT_SHIFT = 4;
explicit Allocator(const utils::AreaPolicy::HeapArea& area);
static constexpr size_t getAlignment() noexcept { return MIN_ALIGNMENT; }
// this is in fact always called with a constexpr size argument
[[nodiscard]] inline void* alloc(size_t size, size_t, size_t extra = 0) noexcept {
[[nodiscard]] inline void* alloc(size_t size, size_t, size_t, uint8_t* outAge) noexcept {
void* p = nullptr;
if (size <= mPool0.getSize()) p = mPool0.alloc(size, 16, extra);
else if (size <= mPool1.getSize()) p = mPool1.alloc(size, 16, extra);
else if (size <= mPool2.getSize()) p = mPool2.alloc(size, 16, extra);
if (size <= mPool0.getSize()) p = mPool0.alloc(size);
else if (size <= mPool1.getSize()) p = mPool1.alloc(size);
else if (size <= mPool2.getSize()) p = mPool2.alloc(size);
if (UTILS_LIKELY(p)) {
Node const* const pNode = static_cast<Node const*>(p);
// we are guaranteed to have at least sizeof<Node> bytes of extra storage before
// the allocation address.
*outAge = pNode[-1].age;
}
return p;
}
// this is in fact always called with a constexpr size argument
inline void free(void* p, size_t size) noexcept {
inline void free(void* p, size_t size, uint8_t age) noexcept {
assert_invariant(p >= mArea.begin() && (char*)p + size <= (char*)mArea.end());
// check for double-free
Node* const pNode = static_cast<Node*>(p);
uint8_t& expectedAge = pNode[-1].age;
ASSERT_POSTCONDITION(expectedAge == age,
"double-free of Handle of size %d at %p", size, p);
expectedAge = (expectedAge + 1) & 0xF; // fixme
if (size <= mPool0.getSize()) { mPool0.free(p); return; }
if (size <= mPool1.getSize()) { mPool1.free(p); return; }
if (size <= mPool2.getSize()) { mPool2.free(p); return; }
@@ -267,24 +262,16 @@ private:
// allocateHandle()/deallocateHandle() selects the pool to use at compile-time based on the
// allocation size this is always inlined, because all these do is to call
// allocateHandleInPool()/deallocateHandleFromPool() with the right pool size.
template<size_t SIZE>
template<typename D>
HandleBase::HandleId allocateHandle() noexcept {
if constexpr (SIZE <= P0) { return allocateHandleInPool<P0>(); }
if constexpr (SIZE <= P1) { return allocateHandleInPool<P1>(); }
static_assert(SIZE <= P2);
return allocateHandleInPool<P2>();
constexpr size_t BUCKET_SIZE = getBucketSize<D>();
return allocateHandleInPool<BUCKET_SIZE>();
}
template<size_t SIZE>
template<typename D>
void deallocateHandle(HandleBase::HandleId id) noexcept {
if constexpr (SIZE <= P0) {
deallocateHandleFromPool<P0>(id);
} else if constexpr (SIZE <= P1) {
deallocateHandleFromPool<P1>(id);
} else {
static_assert(SIZE <= P2);
deallocateHandleFromPool<P2>(id);
}
constexpr size_t BUCKET_SIZE = getBucketSize<D>();
deallocateHandleFromPool<BUCKET_SIZE>(id);
}
// allocateHandleInPool()/deallocateHandleFromPool() is NOT inlined, which will cause three
@@ -293,9 +280,11 @@ private:
template<size_t SIZE>
UTILS_NOINLINE
HandleBase::HandleId allocateHandleInPool() noexcept {
void* p = mHandleArena.alloc(SIZE);
uint8_t age;
void* p = mHandleArena.alloc(SIZE, alignof(std::max_align_t), 0, &age);
if (UTILS_LIKELY(p)) {
return pointerToHandle(p);
uint32_t const tag = (uint32_t(age) << HANDLE_AGE_SHIFT) & HANDLE_AGE_MASK;
return arenaPointerToHandle(p, tag);
} else {
return allocateHandleSlow(SIZE);
}
@@ -305,42 +294,51 @@ private:
UTILS_NOINLINE
void deallocateHandleFromPool(HandleBase::HandleId id) noexcept {
if (UTILS_LIKELY(isPoolHandle(id))) {
void* p = handleToPointer(id);
mHandleArena.free(p, SIZE);
auto [p, tag] = handleToPointer(id);
uint8_t const age = (tag & HANDLE_AGE_MASK) >> HANDLE_AGE_SHIFT;
mHandleArena.free(p, SIZE, age);
} else {
deallocateHandleSlow(id, SIZE);
}
}
static constexpr uint32_t HEAP_HANDLE_FLAG = 0x80000000u;
// we handle a 4 bits age per address
static constexpr uint32_t HANDLE_HEAP_FLAG = 0x80000000u; // pool vs heap handle
static constexpr uint32_t HANDLE_AGE_MASK = 0x78000000u; // handle's age
static constexpr uint32_t HANDLE_INDEX_MASK = 0x07FFFFFFu; // handle index
static constexpr uint32_t HANDLE_TAG_MASK = HANDLE_AGE_MASK;
static constexpr uint32_t HANDLE_AGE_SHIFT = 27;
static bool isPoolHandle(HandleBase::HandleId id) noexcept {
return (id & HEAP_HANDLE_FLAG) == 0u;
return (id & HANDLE_HEAP_FLAG) == 0u;
}
HandleBase::HandleId allocateHandleSlow(size_t size) noexcept;
void deallocateHandleSlow(HandleBase::HandleId id, size_t size) noexcept;
// We inline this because it's just 4 instructions in the fast case
inline void* handleToPointer(HandleBase::HandleId id) const noexcept {
inline std::pair<void*, uint32_t> handleToPointer(HandleBase::HandleId id) const noexcept {
// note: the null handle will end-up returning nullptr b/c it'll be handled as
// a non-pool handle.
if (UTILS_LIKELY(isPoolHandle(id))) {
char* const base = (char*)mHandleArena.getArea().begin();
size_t offset = id << Allocator::MIN_ALIGNMENT_SHIFT;
return static_cast<void*>(base + offset);
uint32_t const tag = id & HANDLE_TAG_MASK;
size_t const offset = (id & HANDLE_INDEX_MASK) * Allocator::getAlignment();
return { static_cast<void*>(base + offset), tag };
}
return handleToPointerSlow(id);
return { handleToPointerSlow(id), 0 };
}
void* handleToPointerSlow(HandleBase::HandleId id) const noexcept;
// We inline this because it's just 3 instructions
inline HandleBase::HandleId pointerToHandle(void* p) const noexcept {
inline HandleBase::HandleId arenaPointerToHandle(void* p, uint32_t tag) const noexcept {
char* const base = (char*)mHandleArena.getArea().begin();
size_t offset = (char*)p - base;
auto id = HandleBase::HandleId(offset >> Allocator::MIN_ALIGNMENT_SHIFT);
assert_invariant((id & HEAP_HANDLE_FLAG) == 0);
size_t const offset = (char*)p - base;
assert_invariant((offset % Allocator::getAlignment()) == 0);
auto id = HandleBase::HandleId(offset / Allocator::getAlignment());
id |= tag & HANDLE_TAG_MASK;
assert_invariant((id & HANDLE_HEAP_FLAG) == 0);
return id;
}
@@ -350,9 +348,6 @@ private:
mutable utils::Mutex mLock;
tsl::robin_map<HandleBase::HandleId, void*> mOverflowMap;
HandleBase::HandleId mId = 0;
#if HANDLE_TYPE_SAFETY
mutable std::unordered_map<const void*, const char*> mHandleTypeId;
#endif
};
} // namespace filament::backend

41
filament/backend/src/HandleAllocator.cpp
View File

@@ -19,12 +19,17 @@
#include <backend/Handle.h>
#include <utils/Allocator.h>
#include <utils/Log.h>
#include <utils/Panic.h>
#include <utils/compiler.h>
#include <utils/debug.h>
#include <utils/Panic.h>
#include <utils/ostream.h>
#include <stdlib.h>
#include <algorithm>
#include <exception>
#include <limits>
#include <mutex>
namespace filament::backend {
@@ -36,15 +41,29 @@ UTILS_NOINLINE
HandleAllocator<P0, P1, P2>::Allocator::Allocator(AreaPolicy::HeapArea const& area)
: mArea(area) {
// The largest handle this allocator can generate currently depends on the architecture's
// min alignment, typically 8 or 16 bytes.
// e.g. On Android armv8, the alignment is 16 bytes, so for a 1 MiB heap, the largest handle
// index will be 65536. Note that this is not the same as the number of handles (which
// will always be less).
// Because our maximum representable handle currently is 0x07FFFFFF, the maximum no-nonsensical
// heap size is 2 GiB, which amounts to 7.6 millions handles per pool (in the GL case).
size_t const maxHeapSize = std::min(area.size(), HANDLE_INDEX_MASK * getAlignment());
if (UTILS_UNLIKELY(maxHeapSize != area.size())) {
slog.w << "HandleAllocator heap size reduced to "
<< maxHeapSize << " from " << area.size() << io::endl;
}
// size the different pools so that they can all contain the same number of handles
size_t const count = area.size() / (P0 + P1 + P2);
size_t const count = maxHeapSize / (P0 + P1 + P2);
char* const p0 = static_cast<char*>(area.begin());
char* const p1 = p0 + count * P0;
char* const p2 = p1 + count * P1;
mPool0 = PoolAllocator<P0, 16>(p0, count * P0);
mPool1 = PoolAllocator<P1, 16>(p1, count * P1);
mPool2 = PoolAllocator<P2, 16>(p2, count * P2);
mPool0 = Pool<P0>(p0, count * P0);
mPool1 = Pool<P1>(p1, count * P1);
mPool2 = Pool<P2>(p2, count * P2);
}
// ------------------------------------------------------------------------------------------------
@@ -82,11 +101,17 @@ template <size_t P0, size_t P1, size_t P2>
HandleBase::HandleId HandleAllocator<P0, P1, P2>::allocateHandleSlow(size_t size) noexcept {
void* p = ::malloc(size);
std::unique_lock lock(mLock);
HandleBase::HandleId id = (++mId) | HEAP_HANDLE_FLAG;
HandleBase::HandleId id = (++mId) | HANDLE_HEAP_FLAG;
ASSERT_POSTCONDITION(mId < HANDLE_HEAP_FLAG,
"No more Handle ids available! This can happen if HandleAllocator arena has been full"
" for a while. Please increase FILAMENT_OPENGL_HANDLE_ARENA_SIZE_IN_MB");
mOverflowMap.emplace(id, p);
lock.unlock();
if (UTILS_UNLIKELY(id == (HEAP_HANDLE_FLAG|1u))) { // meaning id was zero
if (UTILS_UNLIKELY(id == (HANDLE_HEAP_FLAG | 1u))) { // meaning id was zero
PANIC_LOG("HandleAllocator arena is full, using slower system heap. Please increase "
"the appropriate constant (e.g. FILAMENT_OPENGL_HANDLE_ARENA_SIZE_IN_MB).");
}
@@ -95,7 +120,7 @@ HandleBase::HandleId HandleAllocator<P0, P1, P2>::allocateHandleSlow(size_t size
template <size_t P0, size_t P1, size_t P2>
void HandleAllocator<P0, P1, P2>::deallocateHandleSlow(HandleBase::HandleId id, size_t) noexcept {
assert_invariant(id & HEAP_HANDLE_FLAG);
assert_invariant(id & HANDLE_HEAP_FLAG);
void* p = nullptr;
auto& overflowMap = mOverflowMap;

40
libs/utils/include/utils/Allocator.h
View File

@@ -250,11 +250,11 @@ public:
return mHead;
}
private:
struct Node {
Node* next;
};
private:
static Node* init(void* begin, void* end,
size_t elementSize, size_t alignment, size_t extra) noexcept;
@@ -272,8 +272,8 @@ public:
AtomicFreeList() noexcept = default;
AtomicFreeList(void* begin, void* end,
size_t elementSize, size_t alignment, size_t extra) noexcept;
AtomicFreeList(const FreeList& rhs) = delete;
AtomicFreeList& operator=(const FreeList& rhs) = delete;
AtomicFreeList(const AtomicFreeList& rhs) = delete;
AtomicFreeList& operator=(const AtomicFreeList& rhs) = delete;
void* pop() noexcept {
Node* const pStorage = mStorage;
@@ -319,7 +319,6 @@ public:
return mStorage + mHead.load(std::memory_order_relaxed).offset;
}
private:
struct Node {
// This should be a regular (non-atomic) pointer, but this causes TSAN to complain
// about a data-race that exists but is benin. We always use this atomic<> in
@@ -350,6 +349,7 @@ private:
std::atomic<Node*> next;
};
private:
// This struct is using a 32-bit offset into the arena rather than
// a direct pointer, because together with the 32-bit tag, it needs to
// fit into 8 bytes. If it was any larger, it would not be possible to
@@ -372,7 +372,8 @@ template <
size_t OFFSET = 0,
typename FREELIST = FreeList>
class PoolAllocator {
static_assert(ELEMENT_SIZE >= sizeof(void*), "ELEMENT_SIZE must accommodate at least a pointer");
static_assert(ELEMENT_SIZE >= sizeof(typename FREELIST::Node),
"ELEMENT_SIZE must accommodate at least a FreeList::Node");
public:
// our allocator concept
void* alloc(size_t size = ELEMENT_SIZE,
@@ -636,6 +637,15 @@ public:
mListener(name, mArea.data(), mArea.size()) {
}
template<typename ... ARGS>
void* alloc(size_t size, size_t alignment, size_t extra, ARGS&& ... args) noexcept {
std::lock_guard<LockingPolicy> guard(mLock);
void* p = mAllocator.alloc(size, alignment, extra, std::forward<ARGS>(args) ...);
mListener.onAlloc(p, size, alignment, extra);
return p;
}
// allocate memory from arena with given size and alignment
// (acceptable size/alignment may depend on the allocator provided)
void* alloc(size_t size, size_t alignment, size_t extra) noexcept {
@@ -668,13 +678,13 @@ public:
return (T*)alloc(count * sizeof(T), alignment);
}
// return memory pointed by p to the arena
// (actual behaviour may depend on allocator provided)
void free(void* p) noexcept {
// some allocators require more parameters
template<typename ... ARGS>
void free(void* p, size_t size, ARGS&& ... args) noexcept {
if (p) {
std::lock_guard<LockingPolicy> guard(mLock);
mListener.onFree(p);
mAllocator.free(p);
mListener.onFree(p, size);
mAllocator.free(p, size, std::forward<ARGS>(args) ...);
}
}
@@ -687,6 +697,16 @@ public:
}
}
// return memory pointed by p to the arena
// (actual behaviour may depend on allocator provided)
void free(void* p) noexcept {
if (p) {
std::lock_guard<LockingPolicy> guard(mLock);
mListener.onFree(p);
mAllocator.free(p);
}
}
// some allocators don't have a free() call, but a single reset() or rewind() instead
void reset() noexcept {
std::lock_guard<LockingPolicy> guard(mLock);