utils: Secure EntityManager and hide implementation details

Move the `mGens` array and `isAlive()` implementation from `EntityManager`
to `EntityManagerImpl` to hide implementation details and maintain a clean
public header. Make `isAlive()` strictly thread-safe by acquiring the
`mFreeListLock`.

isAlive() was relying on undefined behavior, and the comment about
the memory barrier being provided by the lock was misleading (wrong)
because the lock only provided the "release", but isAlive() didn't
have an "acquire". 

Better safe than sorry, we now have to acquire the lock to test isAlive().

build.sh

@@ -3,7 +3,7 @@ set -e
 
 # Host tools required by Android, WASM, and iOS builds
 MOBILE_HOST_TOOLS="matc resgen cmgen filamesh uberz"
-WEB_HOST_TOOLS="${MOBILE_HOST_TOOLS} mipgen filamesh glslminifier"
+WEB_HOST_TOOLS="${MOBILE_HOST_TOOLS} mipgen filamesh"
 
 function print_help {
     local self_name=$(basename "$0")

build/common/get-mesa.sh

@@ -134,23 +134,7 @@ pushd .
 cd ${MESA_DIR}
 
 # Need >= 24 to have llvmpipe for swrast.  llvmpipe is needed for GL >= 4.1.
-git checkout -f mesa-${MESA_VERSION}
+git checkout mesa-${MESA_VERSION}
-
-# Apply custom patch to fix a double-free in OSMesa
-git apply << 'EOF'
-diff --git a/src/mesa/program/program.c b/src/mesa/program/program.c
-index 74bd6a6c33b..a70814e53a1 100644
---- a/src/mesa/program/program.c
-+++ b/src/mesa/program/program.c
-@@ -130,6 +130,7 @@ _mesa_free_program_data(struct gl_context *ctx)
-       ctx->ATIFragmentShader.Current->RefCount--;
-       if (ctx->ATIFragmentShader.Current->RefCount <= 0) {
-          free(ctx->ATIFragmentShader.Current);
-+         ctx->ATIFragmentShader.Current = NULL;
-       }
-    }
- 
-EOF
 
 mkdir -p out
 

filament/backend/include/backend/platforms/PlatformOSMesa.h

@@ -75,8 +75,7 @@ private:
         OSMesaContext context;
         std::unique_ptr<uint8_t[]> buffer;
     };
-    using ContextMap = std::unordered_map<std::thread::id, ContextInfo>;
+    std::unordered_map<std::thread::id, ContextInfo> mAdditionalContexts;
-    ContextMap mAdditionalContexts;
     mutable std::shared_mutex mAdditionalContextsLock;
 };
 

filament/backend/src/opengl/OpenGLDriver.cpp

@@ -403,9 +403,6 @@ void OpenGLDriver::terminate() {
     if (getJobWorker()) {
         getJobWorker()->terminate();
     }
-    // wait for the GPU again because JobWorker might have queued more work.
-    glFinish();
-
     if constexpr (UTILS_HAS_THREADING) {
         stopServiceThread();
     }

filament/backend/src/opengl/platforms/PlatformOSMesa.cpp

@@ -20,7 +20,6 @@
 #include <utils/Panic.h>
 #include <utils/ThreadUtils.h>
 
-#include <algorithm>
 #include <dlfcn.h>
 #include <memory>
 
@@ -204,7 +203,6 @@ void PlatformOSMesa::createContext(bool shared) {
 void PlatformOSMesa::releaseContext() noexcept {
     std::thread::id currentThreadId = utils::ThreadUtils::getThreadId();
     OSMesaContext context = nullptr;
-    std::unique_ptr<uint8_t[]> buffer;
 
     {
         std::lock_guard<std::shared_mutex> lock(mAdditionalContextsLock);
@@ -214,15 +212,16 @@ void PlatformOSMesa::releaseContext() noexcept {
             return;
         }
         context = it->second.context;
-        buffer = std::move(it->second.buffer);
-        mAdditionalContexts.erase(it);
     }
 
     OSMesaAPI* api = (OSMesaAPI*) mOsMesaApi;
-    if (api) {
+    // Passing NULL as the context is the standard way to unbind in OSMesa.
-        // Passing NULL as the context is the standard way to unbind in OSMesa.
+    api->fOSMesaMakeCurrent(NULL, NULL, 0, 0, 0);
-        api->fOSMesaMakeCurrent(NULL, NULL, 0, 0, 0);
+    api->fOSMesaDestroyContext(context);
-        api->fOSMesaDestroyContext(context);
+
+    {
+        std::lock_guard<std::shared_mutex> lock(mAdditionalContextsLock);
+        mAdditionalContexts.erase(currentThreadId);
     }
 }
 

filament/backend/test/BackendTest.cpp

@@ -83,8 +83,6 @@ BackendTest::~BackendTest() {
     driver->terminate();
     delete driver;
     recordFailedImages();
-
-    delete mPlatform;
 }
 
 void BackendTest::initializeDriver() {

filament/src/MaterialDefinition.cpp

@@ -574,15 +574,13 @@ void MaterialDefinition::processPushConstants() {
             [&](MaterialPushConstant const& constant) {
                 snprintf(buf, sizeof(buf), "%s.%s", structVarName.c_str(), constant.name.c_str());
 
-                CString const cs(buf, strlen(buf));
-
                 switch (constant.stage) {
                     case ShaderStage::VERTEX:
-                        vertexConstants.push_back({cs, constant.type});
+                        vertexConstants.push_back({CString(buf), constant.type});
                         vertexCount++;
                         break;
                     case ShaderStage::FRAGMENT:
-                        fragmentConstants.push_back({cs, constant.type});
+                        fragmentConstants.push_back({CString(buf), constant.type});
                         fragmentCount++;
                         break;
                     case ShaderStage::COMPUTE:

libs/utils/include/utils/EntityManager.h

@@ -51,7 +51,7 @@ public:
         virtual ~Listener() noexcept;
     };
 
-    using ChangeCallback = std::function<void(utils::Slice<const Entity>)>;
+    using ChangeCallback = std::function<void(Slice<const Entity>)>;
 
     /**
      * Registers a callback to be triggered when entities are destroyed.
@@ -105,10 +105,7 @@ public:
 
     // Return whether the given Entity has been destroyed (false) or not (true).
     // Thread safe.
-    bool isAlive(Entity const e) const noexcept {
+    bool isAlive(Entity e) const noexcept;
-        assert(getIndex(e) < RAW_INDEX_COUNT);
-        return (!e.isNull()) && (getGeneration(e) == mGens[getIndex(e)]);
-    }
 
     // Registers a listener to be called when an entity is destroyed. Thread safe.
     // If the listener is already registered, this method has no effect.
@@ -118,12 +115,8 @@ public:
     void unregisterListener(Listener* l) noexcept;
 
 
-    /* no user serviceable parts below */
 
-    // current generation of the given index. Use for debugging and testing.
+    /* no user serviceable parts below */
-    uint8_t getGenerationForIndex(size_t const index) const noexcept {
-        return mGens[index];
-    }
 
     // singleton, can't be copied
     EntityManager(const EntityManager& rhs) = delete;
@@ -157,9 +150,6 @@ private:
     static Entity::Type makeIdentity(Entity::Type const g, Entity::Type const i) noexcept {
         return (g << GENERATION_SHIFT) | (i & INDEX_MASK);
     }
-
-    // stores the generation of each index.
-    uint8_t* const mGens;
 };
 
 } // namespace utils

libs/utils/src/EntityManager.cpp

@@ -18,21 +18,21 @@
 
 #include "EntityManagerImpl.h"
 
+#include <utils/Entity.h>
+
+#include <cassert>
+#include <cstddef>
+#include <mutex>
 #include <new>
+#include <utility>
 
 namespace utils {
 
 EntityManager::Listener::~Listener() noexcept = default;
 
-EntityManager::EntityManager()
+EntityManager::EntityManager() = default;
-        : mGens(new uint8_t[RAW_INDEX_COUNT]) {
-    // initialize all the generations to 0
-    std::fill_n(mGens, RAW_INDEX_COUNT, 0);
-}
 
-EntityManager::~EntityManager() {
+EntityManager::~EntityManager() = default;
-    delete [] mGens;
-}
 
 EntityManager& EntityManager::get() noexcept {
     // note: we leak the EntityManager because it's more important that it survives everything else
@@ -41,11 +41,11 @@ EntityManager& EntityManager::get() noexcept {
     return *instance;
 }
 
-void EntityManager::create(size_t n, Entity* entities) {
+void EntityManager::create(size_t const n, Entity* entities) {
     static_cast<EntityManagerImpl *>(this)->create(n, entities);
 }
 
-void EntityManager::destroy(size_t n, Entity* entities) noexcept {
+void EntityManager::destroy(size_t const n, Entity* entities) noexcept {
     static_cast<EntityManagerImpl *>(this)->destroy(n, entities);
 }
 
@@ -84,4 +84,8 @@ void EntityManager::dumpActiveEntities(utils::io::ostream& out) const {
 
 #endif
 
+bool EntityManager::isAlive(Entity const e) const noexcept {
+    return static_cast<EntityManagerImpl const *>(this)->isAlive(e);
+}
+
 } // namespace utils

libs/utils/src/EntityManagerImpl.h

@@ -40,6 +40,7 @@
 #include <deque>
 #include <memory>
 #include <mutex> // for std::lock_guard
+#include <new>
 #include <utility>
 #include <vector>
 
@@ -49,15 +50,38 @@ static constexpr size_t MIN_FREE_INDICES = 1024;
 
 class UTILS_PRIVATE EntityManagerImpl : public EntityManager {
 public:
+    friend class EntityManager;
+
     using EntityManager::getGeneration;
     using EntityManager::getIndex;
     using EntityManager::makeIdentity;
     using EntityManager::create;
     using EntityManager::destroy;
 
+    EntityManagerImpl() noexcept
+            : mGens(new(std::nothrow) uint8_t[RAW_INDEX_COUNT]) {
+        // initialize all the generations to 0
+        std::fill_n(mGens, RAW_INDEX_COUNT, 0);
+    }
+
+    ~EntityManagerImpl() noexcept {
+        delete [] mGens;
+    }
+
+    bool isAlive(Entity const e) const noexcept {
+        assert(getIndex(e) < RAW_INDEX_COUNT);
+        if (e.isNull()) {
+            return false;
+        }
+
+        std::lock_guard const lock(mFreeListLock);
+        bool const alive = (getGeneration(e) == mGens[getIndex(e)]);
+        return alive;
+    }
+
     UTILS_NOINLINE
     size_t getEntityCount() const noexcept {
-        std::lock_guard<Mutex> const lock(mFreeListLock);
+        std::lock_guard const lock(mFreeListLock);
         if (mCurrentIndex < RAW_INDEX_COUNT) {
             return (mCurrentIndex - 1) - mFreeList.size();
         } else {
@@ -66,13 +90,13 @@ public:
     }
 
     UTILS_NOINLINE
-    void create(size_t n, Entity* entities) {
+    void create(size_t const n, Entity* entities) {
         Entity::Type index{};
         auto& freeList = mFreeList;
         uint8_t const* const gens = mGens;
 
         // this must be thread-safe, acquire the free-list mutex
-        std::lock_guard<Mutex> const lock(mFreeListLock);
+        std::lock_guard const lock(mFreeListLock);
         Entity::Type currentIndex = mCurrentIndex;
         for (size_t i = 0; i < n; i++) {
             if (UTILS_UNLIKELY(currentIndex >= RAW_INDEX_COUNT || freeList.size() >= MIN_FREE_INDICES)) {
@@ -98,11 +122,11 @@ public:
     }
 
     UTILS_NOINLINE
-    void destroy(size_t n, Entity* entities) noexcept {
+    void destroy(size_t const n, Entity* entities) noexcept {
         auto& freeList = mFreeList;
         uint8_t* const gens = mGens;
 
-        std::unique_lock<Mutex> lock(mFreeListLock);
+        std::unique_lock lock(mFreeListLock);
         for (size_t i = 0; i < n; i++) {
             if (!entities[i]) {
                 // behave like free(), ok to free null Entity.
@@ -110,19 +134,15 @@ public:
             }
 
             // it's an error to delete an Entity twice...
-            assert(isAlive(entities[i]));
+            bool const isAlive = getGeneration(entities[i]) == mGens[getIndex(entities[i])];
+            assert(isAlive);
 
             // ... deleting a dead Entity will corrupt the internal state, so we protect ourselves
             // against it. We don't guarantee anything about external state -- e.g. the listeners
             // will be called.
-            if (isAlive(entities[i])) {
+            if (UTILS_LIKELY(isAlive)) {
                 Entity::Type const index = getIndex(entities[i]);
                 freeList.push_back(index);
-
-                // The generation update doesn't require the lock because it's only used for isAlive()
-                // and entities work as weak references -- it just means that isAlive() could return
-                // true a little longer than expected in some other threads.
-                // We do need a memory fence though, it is provided by the mFreeListLock.unlock() below.
                 gens[index]++;
 
 #if FILAMENT_UTILS_TRACK_ENTITIES
@@ -151,22 +171,22 @@ public:
     }
 
     void registerListener(Listener* l) noexcept {
-        std::lock_guard<Mutex> const lock(mListenerLock);
+        std::lock_guard const lock(mListenerLock);
         mListeners.insert(l);
     }
 
     void unregisterListener(Listener* l) noexcept {
-        std::lock_guard<Mutex> const lock(mListenerLock);
+        std::lock_guard const lock(mListenerLock);
         mListeners.erase(l);
     }
 
     void registerChangeCallback(void const* token, ChangeCallback callback) noexcept {
-        std::lock_guard<Mutex> const lock(mListenerLock);
+        std::lock_guard const lock(mListenerLock);
         mChangeCallbacks.push_back({token, std::move(callback)});
     }
 
     void unregisterChangeCallback(void const* token) noexcept {
-        std::lock_guard<Mutex> const lock(mListenerLock);
+        std::lock_guard const lock(mListenerLock);
         mChangeCallbacks.erase(
                 std::remove_if(mChangeCallbacks.begin(), mChangeCallbacks.end(),
                         [token](auto const& info) { return info.token == token; }),
@@ -174,7 +194,7 @@ public:
     }
 
     void flushNotifications() noexcept {
-        std::unique_lock<Mutex> lock(mFreeListLock);
+        std::unique_lock lock(mFreeListLock);
         if (mDirtyCount > 0) {
             Entity localBuffer[MAX_DIRTY_COUNT];
             assert_invariant(mDirtyCount <= MAX_DIRTY_COUNT);
@@ -208,7 +228,7 @@ public:
 
 private:
     std::vector<ChangeCallback> getChangeCallbacks() const noexcept {
-        std::lock_guard<Mutex> const lock(mListenerLock);
+        std::lock_guard const lock(mListenerLock);
         std::vector<ChangeCallback> result;
         result.reserve(mChangeCallbacks.size());
         for (auto const& info : mChangeCallbacks) {
@@ -217,7 +237,7 @@ private:
         return result;
     }
 
-    void triggerChangeCallbacks(Entity const* entities, size_t n) const noexcept {
+    void triggerChangeCallbacks(Entity const* entities, size_t const n) const noexcept {
         auto const callbacks = getChangeCallbacks();
         Slice const slice(entities, n);
         for (auto const& callback : callbacks) {
@@ -226,7 +246,7 @@ private:
     }
 
     FixedCapacityVector<Listener*> getListeners() const noexcept {
-        std::lock_guard<Mutex> const lock(mListenerLock);
+        std::lock_guard const lock(mListenerLock);
         tsl::robin_set<Listener*> const& listeners = mListeners;
         FixedCapacityVector<Listener*> result(listeners.size());
         result.resize(result.capacity()); // unfortunately this memset()
@@ -234,22 +254,20 @@ private:
         return result; // the c++ standard guarantees a move
     }
 
-    uint32_t mCurrentIndex = 1;
+    static constexpr size_t MAX_DIRTY_COUNT = 16;
-
-    // stores indices that got freed
-    mutable Mutex mFreeListLock;
-    std::deque<Entity::Type> mFreeList;
-
-    mutable Mutex mListenerLock;
-    tsl::robin_set<Listener*> mListeners;
-
     struct CallbackInfo {
         void const* token;
         ChangeCallback callback;
     };
-    std::vector<CallbackInfo> mChangeCallbacks;
 
-    static constexpr size_t MAX_DIRTY_COUNT = 16;
+    mutable Mutex mFreeListLock;
+    uint32_t mCurrentIndex = 1;
+    std::deque<Entity::Type> mFreeList;     // stores indices that got freed
+    uint8_t* const mGens;                   // stores the generation of each index.
+
+    mutable Mutex mListenerLock;
+    tsl::robin_set<Listener*> mListeners;
+    std::vector<CallbackInfo> mChangeCallbacks;
     Entity mDirtyEntities[MAX_DIRTY_COUNT];
     size_t mDirtyCount = 0;
 

test/backend/test.sh

@@ -14,7 +14,6 @@ if [ ! -d "${PROJECT_ROOT_DIR}/mesa/out" ]; then
 fi
 
 BACKEND_TEST_TARGET=''
-ASAN_FLAG=''
 
 # Set environment variables to use Mesa drivers.
 os_name=$(uname -s)
@@ -28,18 +27,15 @@ if [[ "$os_name" == "Linux" ]]; then
         export VK_ICD_FILENAMES="${PROJECT_ROOT_DIR}/mesa/out/share/vulkan/icd.d/lvp_icd.x86_64.json"
     fi
     BACKEND_TEST_TARGET=backend_test_linux
-    ASAN_FLAG="-b"
 elif [[ "$os_name" == "Darwin" ]]; then
     export DYLD_LIBRARY_PATH="${PROJECT_ROOT_DIR}/mesa/out/lib"
     export VK_ICD_FILENAMES="${PROJECT_ROOT_DIR}/mesa/out/share/vulkan/icd.d/lvp_icd.aarch64.json"
     BACKEND_TEST_TARGET=backend_test_mac
-    # asan is too slow for macOs build of the backend test
-    ASAN_FLAG=""
 fi
 
-# Build backend test
+# Build backend_test_mac
-echo "Building ${BACKEND_TEST_TARGET}..."
+echo "Building backend_test_mac..."
-"${PROJECT_ROOT_DIR}/build.sh" ${ASAN_FLAG} -W -y release -p desktop -X "${PROJECT_ROOT_DIR}/mesa" debug ${BACKEND_TEST_TARGET}
+"${PROJECT_ROOT_DIR}/build.sh" -W -p desktop -X "${PROJECT_ROOT_DIR}/mesa" debug ${BACKEND_TEST_TARGET}
 
 set +e
 
@@ -55,20 +51,10 @@ do
     fi
 done
 
-# Mesa OSMesa is known to leak part of the context.
-LSAN_CMD_PREFIX=""
-if [[ "${ASAN_FLAG}" == "-b" ]]; then
-    echo "leak:PlatformOSMesa.cpp" > leak_skip.txt
-    export LSAN_OPTIONS=suppressions=leak_skip.txt
-fi
-
 FINAL_RESULT=0
 for BACKEND in ${BACKENDS[@]}; do
     echo "----- ${BACKEND} backend test -----"
-
+    ${PROJECT_ROOT_DIR}/out/cmake-debug/filament/backend/${BACKEND_TEST_TARGET} -a ${BACKEND} --ci --headless_only ${GTEST_FILTER_ARG}
-    ${PROJECT_ROOT_DIR}/out/cmake-debug/filament/backend/${BACKEND_TEST_TARGET} \
-                           -a ${BACKEND} --ci --headless_only ${GTEST_FILTER_ARG}
-
     RESULT=$(echo $?)
     if [ ${RESULT} -gt 0 ]; then
         echo "----- Error: backend ${BACKEND} test failed with result ${RESULT} -----"

third_party/abseil/absl/base/config.h

@@ -302,19 +302,7 @@ static_assert(ABSL_INTERNAL_INLINE_NAMESPACE_STR[0] != 'h' ||
 #if (defined(__clang__) && !defined(_WIN32)) ||           \
     (defined(__CUDACC__) && __CUDACC_VER_MAJOR__ >= 9) || \
     (defined(__GNUC__) && !defined(__clang__) && !defined(__CUDACC__))
-
+#define ABSL_HAVE_INTRINSIC_INT128 1
-// Disable int128 instrinsic on arm when asan is enabled
-#if defined(__arm__) || defined(__aarch64__)
-    #if defined(__has_feature) || defined(__SANITIZE_ADDRESS__)
-        #if __has_feature(address_sanitizer) || defined(__SANITIZE_ADDRESS__)
-            #define ABSL_ASAN_ON_ARM_DISABLE_INT128 1
-        #endif
-    #endif
-#endif
-#ifndef ABSL_ASAN_ON_ARM_DISABLE_INT128
-    #define ABSL_HAVE_INTRINSIC_INT128 1
-#endif
-
 #elif defined(__CUDACC__)
 // __CUDACC_VER__ is a full version number before CUDA 9, and is defined to a
 // string explaining that it has been removed starting with CUDA 9. We use

third_party/abseil/tnt/README.md

@@ -32,13 +32,3 @@ This folder previously last updated as follows:
     rsync -r abseil_new/ abseil/ --delete --exclude tnt
     rm -rf master.zip abseil_new
     git add abseil ; git status
-
-## Custom Changes
-
-A custom patch has been applied to fix compilation on ARM architectures when AddressSanitizer (ASAN) is enabled (specifically regarding `ABSL_HAVE_INTRINSIC_INT128`).
-
-If you update the `abseil` folder, you may need to re-apply the patch. You can do so by running the following command from the repository root:
-
-```shell
-git apply third_party/abseil/tnt/asan_arm_int128.diff
-```