From 844dde4e6d60930dce203a2c8ff5f76bf0f38b1a Mon Sep 17 00:00:00 2001
From: Mathias Agopian <mathias@google.com>
Date: Wed, 28 Aug 2019 15:23:18 -0700
Subject: [PATCH] simple allocation debugger

This can be enabled by using the Tracking::Debug policy,
currently this just fills allocation on alloc() and free(),
which is useful to help detect access to uninitialized memory
and use after free.

Now enabled by default in libfilament allocators on debug builds.

This caught uninitialized access in the froxelizer.
---
 filament/src/Froxelizer.cpp          |  7 ++----
 filament/src/details/Allocators.h    |  2 +-
 libs/utils/include/utils/Allocator.h | 29 ++++++++++++++----------
 libs/utils/src/Allocator.cpp         | 34 ++++++++++++++++++++++++++++
 4 files changed, 54 insertions(+), 18 deletions(-)

diff --git a/filament/src/Froxelizer.cpp b/filament/src/Froxelizer.cpp
index 2f9f8ce598..e8613c86fe 100644
--- a/filament/src/Froxelizer.cpp
+++ b/filament/src/Froxelizer.cpp
@@ -195,11 +195,8 @@ bool Froxelizer::prepare(
     assert(mLightRecords.begin());
     assert(mFroxelShardedData.begin());
 
-#ifndef NDEBUG
-    memset(mFroxelBufferUser.data(),    0x55, mFroxelBufferUser.sizeInBytes());
-    memset(mRecordBufferUser.data(),    0xEB, mRecordBufferUser.sizeInBytes());
-    memset(mFroxelShardedData.data(),   0xFD, mFroxelShardedData.sizeInBytes());
-#endif
+    // initialize buffers that need to be
+    memset(mLightRecords.data(),0, mLightRecords.sizeInBytes());
 
     return uniformsNeedUpdating;
 }
diff --git a/filament/src/details/Allocators.h b/filament/src/details/Allocators.h
index 8f434d8268..c581df77d8 100644
--- a/filament/src/details/Allocators.h
+++ b/filament/src/details/Allocators.h
@@ -42,7 +42,7 @@ using HeapAllocatorArena = utils::Arena<
 using LinearAllocatorArena = utils::Arena<
         utils::LinearAllocator,
         utils::LockingPolicy::NoLock,
-        utils::TrackingPolicy::HighWatermark>;
+        utils::TrackingPolicy::Debug>;
 
 #else
 
diff --git a/libs/utils/include/utils/Allocator.h b/libs/utils/include/utils/Allocator.h
index a81824c083..64bebfbe26 100644
--- a/libs/utils/include/utils/Allocator.h
+++ b/libs/utils/include/utils/Allocator.h
@@ -440,6 +440,7 @@ using Mutex = utils::Mutex;
 
 namespace TrackingPolicy {
 
+// default no-op tracker
 struct Untracked {
     Untracked() noexcept = default;
     Untracked(const char* name, size_t size) noexcept { }
@@ -449,24 +450,16 @@ struct Untracked {
     void onRewind(void* addr) noexcept { }
 };
 
-// This high watermark tracker works only with allocator that either implement
-// free(void*, size_t), or reset() / rewind()
-
+// This just track the max memory usage and logs it in the destructor
 struct HighWatermark {
     HighWatermark() noexcept = default;
-    HighWatermark(const char* name, size_t size) noexcept
-            : mName(name), mSize(uint32_t(size)) { }
+    HighWatermark(const char* name, size_t size) noexcept : mName(name), mSize(uint32_t(size)) { }
     ~HighWatermark() noexcept;
-    void onAlloc(void* p, size_t size, size_t alignment, size_t extra) noexcept {
-        if (!mBase) { mBase = p; }
-        mCurrent += uint32_t(size);
-        mHighWaterMark = mCurrent > mHighWaterMark ? mCurrent : mHighWaterMark;
-    }
+    void onAlloc(void* p, size_t size, size_t alignment, size_t extra) noexcept;
     void onFree(void* p, size_t size) noexcept { mCurrent -= uint32_t(size); }
     void onReset() noexcept {  mCurrent = 0; }
     void onRewind(void const* addr) noexcept { mCurrent = uint32_t(uintptr_t(addr) - uintptr_t(mBase)); }
-
-private:
+protected:
     const char* mName = nullptr;
     void* mBase = nullptr;
     uint32_t mSize = 0;
@@ -474,6 +467,18 @@ private:
     uint32_t mHighWaterMark = 0;
 };
 
+// This just fills buffers with known values to help catch uninitialized access and use after free.
+// It also tracks the high water mark
+struct Debug : protected HighWatermark {
+    Debug() noexcept = default;
+    Debug(const char* name, size_t size) noexcept : HighWatermark(name, size) { }
+    void onAlloc(void* p, size_t size, size_t alignment, size_t extra) noexcept;
+    void onFree(void* p, size_t = 0) noexcept;
+    void onReset() noexcept;
+    void onRewind(void* addr) noexcept;
+};
+
+
 } // namespace TrackingPolicy
 
 // ------------------------------------------------------------------------------------------------
diff --git a/libs/utils/src/Allocator.cpp b/libs/utils/src/Allocator.cpp
index b5a4c25e5e..cd643d9a16 100644
--- a/libs/utils/src/Allocator.cpp
+++ b/libs/utils/src/Allocator.cpp
@@ -125,6 +125,15 @@ AtomicFreeList::AtomicFreeList(void* begin, void* end,
     mHead.store({ int32_t(head - mStorage), 0 });
 }
 
+// ------------------------------------------------------------------------------------------------
+
+void TrackingPolicy::HighWatermark::onAlloc(
+        void* p, size_t size, size_t alignment, size_t extra) noexcept {
+    if (!mBase) { mBase = p; }
+    mCurrent += uint32_t(size);
+    mHighWaterMark = mCurrent > mHighWaterMark ? mCurrent : mHighWaterMark;
+}
+
 TrackingPolicy::HighWatermark::~HighWatermark() noexcept {
     size_t wm = mHighWaterMark;
     size_t wmpct = wm / (mSize / 100);
@@ -134,4 +143,29 @@ TrackingPolicy::HighWatermark::~HighWatermark() noexcept {
     }
 }
 
+void TrackingPolicy::Debug::onAlloc(void* p, size_t size, size_t alignment, size_t extra) noexcept {
+    memset(p, 0xeb, size);
+    HighWatermark::onAlloc(p, size, alignment, extra);
+}
+
+void TrackingPolicy::Debug::onFree(void* p, size_t size) noexcept {
+    memset(p, 0xef, size);
+    HighWatermark::onFree(p, size);
+}
+
+void TrackingPolicy::Debug::onReset() noexcept {
+    if (mBase) {
+        memset(mBase, 0xec, mSize);
+    }
+    HighWatermark::onReset();
+}
+
+void TrackingPolicy::Debug::onRewind(void* addr) noexcept {
+    if (mBase) {
+        memset(addr, 0x55, uintptr_t(mBase) + mSize - uintptr_t(addr));
+    }
+    HighWatermark::onRewind(addr);
+}
+
+
 } // namespace utils