]> git-server-git.apps.pok.os.sepia.ceph.com Git - ceph.git/commitdiff
rgw/posix: fix memory leaks in BucketCache 66850/head
authorKefu Chai <k.chai@proxmox.com>
Fri, 9 Jan 2026 02:02:37 +0000 (10:02 +0800)
committerKefu Chai <k.chai@proxmox.com>
Mon, 12 Jan 2026 12:52:25 +0000 (20:52 +0800)
The RGW POSIX driver's BucketCache had two memory leak issues:

1. BucketCache destructor did not clean up cached entries
2. list_bucket() had missing lru.unref() calls on early return paths

Issue 1: Missing destructor cleanup
------------------------------------
BucketCache did not have a destructor to clean up BucketCacheEntry
objects at destruction, causing LeakSanitizer to report ~22-31MB of
leaks in unittest_rgw_posix_driver and unittest_posix_bucket_cache.

The cache is properly bounded at runtime (max 100 entries with LRU
eviction), but entries remaining at test shutdown were not freed.

Issue 2: Missing unref calls in list_bucket()
----------------------------------------------
The list_bucket() function had three early return paths that failed
to call lru.unref() before returning:

1. Line 475: When marker not found (MDB_NOTFOUND)
2. Line 483: When bucket is empty (MDB_NOTFOUND)
3. Line 491: When iteration stops early (!again)

This left entries with refcount=2 instead of the expected refcount=1
(sentinel state), preventing proper cleanup in the destructor.

Root cause analysis
-------------------
Through debugging, we discovered:
- Entries created with FLAG_INITIAL start with refcount=2
- list_bucket() should call lru.unref() to decrement to refcount=1
- Missing unref calls left entries with refcount=2
- Destructor's single unref only decremented to refcount=1, not 0
- Entries with refcount=1 were moved to LRU instead of deleted

The fix
-------
1. Add BucketCache destructor that:
   - Stops inotify thread first (prevents heap-use-after-free)
   - Drains AVL cache and calls lru.unref() on each entry

2. Add lru.unref() calls to all three early return paths in list_bucket()

This ensures all entries reach refcount=0 and are properly deleted,
eliminating all memory leaks while maintaining cache performance.

Signed-off-by: Kefu Chai <k.chai@proxmox.com>
src/rgw/driver/posix/bucket_cache.h

index 359238e5a367a33857589f954b741a450aeaa988..721c4e50abba125d9e199ada3f25419dfd3463aa 100644 (file)
@@ -276,6 +276,27 @@ public:
       }
     }
 
+  ~BucketCache() {
+    /* Stop the inotify thread first to prevent it from accessing
+     * cache entries while we're draining them. This prevents
+     * heap-use-after-free errors. */
+    un.reset();
+
+    /* Clean up all cached bucket entries to prevent memory leaks.
+     * Drain the AVL cache and unref each entry to trigger deletion.
+     *
+     * Entries should have refcount=1 (sentinel state) after normal use
+     * because list_bucket() calls lru.unref() on all code paths. When
+     * we call unref here, the refcount goes to 0 and the entry is deleted.
+     *
+     * The drain() method safely removes all entries
+     * from the AVL cache partitions and calls the supplied lambda on each,
+     * allowing proper cleanup without accessing private members. */
+    cache.drain([this](BucketCacheEntry<D, B>* entry) {
+      lru.unref(entry, cohort::lru::FLAG_NONE);
+    }, cache.FLAG_LOCK);
+  }
+
   static constexpr uint32_t FLAG_NONE     = 0x0000;
   static constexpr uint32_t FLAG_CREATE   = 0x0001;
   static constexpr uint32_t FLAG_LOCK     = 0x0002;
@@ -446,6 +467,7 @@ public:
        auto rc = cursor.lower_bound(k, key, data);
        if (rc == MDB_NOTFOUND) {
          /* no key sorts after k/marker, so there is nothing to do */
+         lru.unref(b, cohort::lru::FLAG_NONE);
          return 0;
        }
        proc_result();
@@ -454,6 +476,7 @@ public:
        auto rc = cursor.get(key, data, MDB_FIRST);
        if (rc == MDB_NOTFOUND) {
          /* no initial key */
+         lru.unref(b, cohort::lru::FLAG_NONE);
          return 0;
        }
        if (rc == MDB_SUCCESS) {
@@ -462,6 +485,7 @@ public:
       }
       while(cursor.get(key, data, MDB_NEXT) == MDB_SUCCESS) {
        if (!again) {
+         lru.unref(b, cohort::lru::FLAG_NONE);
          return 0;
        }
        proc_result();