Minor fixes in arena and pool

- check ownership before canary guards, because if wrong pointer
  is deallocated, this panic will be more informative

- fix thread-safety of num_guard_failures()

- improve comments

src/internal_modules/roc_core/heap_arena.cpp

@@ -26,7 +26,7 @@ HeapArena::HeapArena()
 HeapArena::~HeapArena() {
     if (num_allocations_ != 0) {
         if (AtomicOps::load_seq_cst(flags_) & HeapArenaFlag_EnableLeakDetection) {
-            roc_panic("heap arena: detected leak(s): %d objects was not freed",
+            roc_panic("heap arena: detected leak(s): %d chunks were not freed",
                       (int)num_allocations_);
         }
     }
@@ -75,6 +75,19 @@ void HeapArena::deallocate(void* ptr) {
     ChunkHeader* chunk =
         ROC_CONTAINER_OF((char*)ptr - sizeof(ChunkCanary), ChunkHeader, data);
 
+    const bool is_owner = chunk->owner == this;
+
+    if (!is_owner) {
+        num_guard_failures_++;
+        if (AtomicOps::load_seq_cst(flags_) & HeapArenaFlag_EnableGuards) {
+            roc_panic("heap arena:"
+                      " attempt to deallocate chunk not belonging to this arena:"
+                      " this_arena=%p chunk_arena=%p",
+                      (const void*)this, (const void*)chunk->owner);
+        }
+        return;
+    }
+
     size_t size = chunk->size;
 
     char* canary_before = (char*)chunk->data;
@@ -94,19 +107,6 @@ void HeapArena::deallocate(void* ptr) {
         }
     }
 
-    const bool is_owner = chunk->owner == this;
-
-    if (!is_owner) {
-        num_guard_failures_++;
-        if (AtomicOps::load_seq_cst(flags_) & HeapArenaFlag_EnableGuards) {
-            roc_panic("heap arena: attempt to deallocate chunk not belonging to this "
-                      "heap arena:"
-                      " this_pool=%p chunk_pool=%p",
-                      (const void*)this, (const void*)chunk->owner);
-        }
-        return;
-    }
-
     const int n = num_allocations_--;
 
     if (n == 0) {

src/internal_modules/roc_core/heap_arena.h

@@ -35,10 +35,28 @@ enum { DefaultHeapArenaFlags = (HeapArenaFlag_EnableGuards) };
 //!
 //! Uses malloc() and free().
 //!
-//! Supports memory "poisoning" to make memory-related bugs (out of bound writes, use
-//! after free, etc) more noticeable.
+//! The memory is always maximum aligned.
 //!
-//! The memory is always maximum aligned. Thread-safe.
+//! Implements three safety measures:
+//!  - to catch double-free and other logical bugs, inserts link to owning arena before
+//!    user data, and panics if it differs when memory is returned to arena
+//!  - to catch buffer overflow bugs, inserts "canary guards" before and after user
+//!    data, and panics if they are overwritten when memory is returned to arena
+//!  - to catch uninitialized-access and use-after-free bugs, "poisons" memory when it
+//!    returned to user, and when it returned back to the arena
+//!
+//! Allocated chunks have the following format:
+//! @code
+//!  +-------------+-------------+-----------+-------------+
+//!  | ChunkHeader | ChunkCanary | user data | ChunkCanary |
+//!  +-------------+-------------+-----------+-------------+
+//! @endcode
+//!
+//! ChunkHeader contains pointer to the owning arena, checked when returning memory to
+//! arena. ChunkCanary contains magic bytes filled when returning memory to user, and
+//! checked when returning memory to arena.
+//!
+//! Thread-safe.
 class HeapArena : public IArena, public NonCopyable<> {
 public:
     //! Initialize.
@@ -65,19 +83,20 @@ class HeapArena : public IArena, public NonCopyable<> {
 
 private:
     struct ChunkHeader {
-        size_t size;
-        //! The heap arena that the chunk belongs to.
+        // The heap arena that the chunk belongs to.
         HeapArena* owner;
+        // Data size, excluding canary guards.
+        size_t size;
+        // Data surrounded with canary guards.
         AlignMax data[];
     };
 
     typedef AlignMax ChunkCanary;
 
     Atomic<int> num_allocations_;
+    Atomic<size_t> num_guard_failures_;
 
     static size_t flags_;
-
-    size_t num_guard_failures_;
 };
 
 } // namespace core

src/internal_modules/roc_core/pool.h

@@ -45,8 +45,13 @@ enum { DefaultPoolFlags = (PoolFlag_EnableGuards) };
 //!
 //! The returned memory is always maximum-aligned.
 //!
-//! Supports memory "poisoning" to make memory-related bugs (out of bound writes, use
-//! after free, etc) more noticeable.
+//! Implements three safety measures:
+//!  - to catch double-free and other logical bugs, inserts link to owning pool before
+//!    user data, and panics if it differs when memory is returned to pool
+//!  - to catch buffer overflow bugs, inserts "canary guards" before and after user
+//!    data, and panics if they are overwritten when memory is returned to pool
+//!  - to catch uninitialized-access and use-after-free bugs, "poisons" memory when it
+//!    returned to user, and when it returned back to the pool
 //!
 //! @tparam T defines pool object type. It is used to determine allocation size. If
 //! runtime size is different from static size of T, it can be provided via constructor.

src/internal_modules/roc_core/pool_impl.cpp

@@ -117,6 +117,8 @@ void PoolImpl::deallocate(void* memory) {
 }
 
 size_t PoolImpl::num_guard_failures() const {
+    Mutex::Lock lock(mutex_);
+
     return num_guard_failures_;
 }
 
@@ -142,6 +144,18 @@ PoolImpl::Slot* PoolImpl::take_slot_from_user_(void* memory) {
     SlotHeader* slot_hdr =
         ROC_CONTAINER_OF((char*)memory - sizeof(SlotCanary), SlotHeader, data);
 
+    const bool is_owner = slot_hdr->owner == this;
+
+    if (!is_owner) {
+        num_guard_failures_++;
+        if (flags_ & PoolFlag_EnableGuards) {
+            roc_panic("pool: attempt to deallocate slot not belonging to this pool:"
+                      " name=%s this_pool=%p slot_pool=%p",
+                      name_, (const void*)this, (const void*)slot_hdr->owner);
+        }
+        return NULL;
+    }
+
     void* canary_before = (char*)slot_hdr->data;
     void* canary_after = (char*)slot_hdr->data + sizeof(SlotCanary) + object_size_;
 
@@ -158,18 +172,6 @@ PoolImpl::Slot* PoolImpl::take_slot_from_user_(void* memory) {
         }
     }
 
-    const bool is_owner = slot_hdr->owner == this;
-
-    if (!is_owner) {
-        num_guard_failures_++;
-        if (flags_ & PoolFlag_EnableGuards) {
-            roc_panic("pool: attempt to deallocate slot not belonging to this pool:"
-                      " name=%s this_pool=%p slot_pool=%p",
-                      name_, (const void*)this, (const void*)slot_hdr->owner);
-        }
-        return NULL;
-    }
-
     MemoryOps::poison_after_use(memory, object_size_);
 
     return new (slot_hdr) Slot;

src/internal_modules/roc_core/pool_impl.h

@@ -28,19 +28,16 @@ namespace core {
 //! This is non-template class that implements all pool logic, to avoid
 //! polluting header file.
 //!
-//! Allocated slot have the following format:
+//! Allocated slots have the following format:
 //! @code
 //!  +------------+------------+-----------+------------+
 //!  | SlotHeader | SlotCanary | user data | SlotCanary |
 //!  +------------+------------+-----------+------------+
 //! @endcode
 //!
-//! SlotHeader contains pointer to the owning pool. It is used to ensure
-//! integrity of allocation and deallocation calls.
-//!
-//! SlotCanary contains magic bytes filled when returning slot to user,
-//! and checked when returning slot to pool. They are used to detect
-//! buffer overflow bugs.
+//! SlotHeader contains pointer to the owning pool, checked when returning memory to
+//! pool. SlotCanary contains magic bytes filled when returning memory to user, and
+//! checked when returning memory to pool.
 //!
 //! If user data requires padding to be maximum-aligned, this padding
 //! also becomes part of the trailing canary guard.