From 41208b7df116e83576fd6bdda4b8da142e6ad388 Mon Sep 17 00:00:00 2001 From: Kefu Chai Date: Fri, 9 Jan 2026 10:02:37 +0800 Subject: [PATCH] rgw/posix: fix memory leaks in BucketCache 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 --- src/rgw/driver/posix/bucket_cache.h | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/rgw/driver/posix/bucket_cache.h b/src/rgw/driver/posix/bucket_cache.h index 359238e5a36..721c4e50abb 100644 --- a/src/rgw/driver/posix/bucket_cache.h +++ b/src/rgw/driver/posix/bucket_cache.h @@ -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* 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(); -- 2.47.3