From 0ecf1a41370f364491c4b26453268b3833776728 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=E6=9C=A8=E5=A4=B4=E4=BA=91?= <orz@orzz.org>
Date: Sun, 30 Nov 2025 05:28:48 +0000
Subject: [PATCH] docs(shm): add semantic comments for release/remove and fix
 double-free in tests

- Add comprehensive documentation for shm::release(id), shm::remove(id), and shm::remove(name)
  - release(id): Decrements ref count, cleans up memory and disk file when count reaches zero
  - remove(id): Calls release(id) internally, then forces disk file cleanup (WARNING: do not use after release)
  - remove(name): Only removes disk file, safe to use anytime

- Fix critical double-free bug in ShmTest test cases
  - Problem: calling release(id) followed by remove(id) causes use-after-free crash
    because release() already frees the id structure
  - Solution: replace 'release(id); remove(id);' pattern with just 'remove(id)'
  - Fixed tests: AcquireCreate, AcquireCreateOrOpen, GetMemory, GetMemoryNoSize,
    RemoveById, SubtractReference
  - Kept 'release(id); remove(name);' pattern unchanged (safe usage)

- Add explanatory comments in test code to prevent future misuse
---
 include/libipc/shm.h | 22 ++++++++++++++++++++++
 test/test_shm.cpp    | 12 ++++++------
 2 files changed, 28 insertions(+), 6 deletions(-)

diff --git a/include/libipc/shm.h b/include/libipc/shm.h
index dc24ab4..7035f7e 100755
--- a/include/libipc/shm.h
+++ b/include/libipc/shm.h
@@ -17,8 +17,30 @@ enum : unsigned {
 
 IPC_EXPORT id_t         acquire(char const * name, std::size_t size, unsigned mode = create | open);
 IPC_EXPORT void *       get_mem(id_t id, std::size_t * size);
+
+// Release shared memory resource and clean up disk file if reference count reaches zero.
+// This function decrements the reference counter. When the counter reaches zero, it:
+// 1. Unmaps the shared memory region
+// 2. Removes the backing file from disk (shm_unlink on POSIX)
+// 3. Frees the id structure
+// After calling this function, the id becomes invalid and must not be used again.
+// Returns: The reference count before decrement, or -1 on error.
 IPC_EXPORT std::int32_t release(id_t id) noexcept;
+
+// Release shared memory resource and force cleanup of disk file.
+// This function calls release(id) internally, then unconditionally attempts to
+// remove the backing file. WARNING: Do NOT call this after release(id) on the
+// same id, as the id is already freed by release(). Use this function alone,
+// not in combination with release().
+// Typical use case: Force cleanup when you want to ensure the disk file is removed
+// regardless of reference count state.
 IPC_EXPORT void         remove (id_t id) noexcept;
+
+// Remove shared memory backing file by name.
+// This function only removes the disk file and does not affect any active memory
+// mappings or id structures. Use this for cleanup of orphaned files or for explicit
+// file removal without affecting runtime resources.
+// Safe to call at any time, even if shared memory is still in use elsewhere.
 IPC_EXPORT void         remove (char const * name) noexcept;
 
 IPC_EXPORT std::int32_t get_ref(id_t id);
diff --git a/test/test_shm.cpp b/test/test_shm.cpp
index c2c4d3f..51e6b33 100644
--- a/test/test_shm.cpp
+++ b/test/test_shm.cpp
@@ -50,7 +50,7 @@ TEST_F(ShmTest, AcquireCreate) {
   EXPECT_NE(mem, nullptr);
   EXPECT_GE(actual_size, size);
   
-  shm::release(id);
+  // Use remove(id) to clean up - it internally calls release()
   shm::remove(id);
 }
 
@@ -78,7 +78,7 @@ TEST_F(ShmTest, AcquireCreateOrOpen) {
   EXPECT_NE(mem, nullptr);
   EXPECT_GE(actual_size, size);
   
-  shm::release(id);
+  // Use remove(id) to clean up - it internally calls release()
   shm::remove(id);
 }
 
@@ -101,7 +101,7 @@ TEST_F(ShmTest, GetMemory) {
   std::strcpy(static_cast<char*>(mem), test_data);
   EXPECT_STREQ(static_cast<char*>(mem), test_data);
   
-  shm::release(id);
+  // Use remove(id) to clean up - it internally calls release()
   shm::remove(id);
 }
 
@@ -115,7 +115,7 @@ TEST_F(ShmTest, GetMemoryNoSize) {
   void* mem = shm::get_mem(id, nullptr);
   EXPECT_NE(mem, nullptr);
   
-  shm::release(id);
+  // Use remove(id) to clean up - it internally calls release()
   shm::remove(id);
 }
 
@@ -139,7 +139,7 @@ TEST_F(ShmTest, RemoveById) {
   shm::id_t id = shm::acquire(name.c_str(), 256, shm::create);
   ASSERT_NE(id, nullptr);
   
-  shm::release(id);
+  // remove(id) internally calls release(id), so we don't need to call release first
   shm::remove(id); // Should succeed
 }
 
@@ -190,7 +190,7 @@ TEST_F(ShmTest, SubtractReference) {
   
   EXPECT_EQ(ref_after, ref_before - 1);
   
-  shm::release(id);
+  // Use remove(id) to clean up - it internally calls release()
   shm::remove(id);
 }