FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

LRUCache.Add corrupts state on existing keys: duplicate elements, wrong evictions, leaked mmap regions · Issue #686 · nutsdb/nutsdb · GitHub

/ nutsdb Public

LRUCache.Add corrupts state on existing keys: duplicate elements, wrong evictions, leaked mmap regions #686

Description

Summary

utils.LRUCache.Add() never checks whether key already exists. Re-adding an existing key pushes a second list element for the same key and overwrites the map entry, leaving the old element stranded in the list. This progressively corrupts the cache: Len() overcounts, later evictions delete the live map entry while a stale element stays in the list, and (in the mmap read path) an entire mmap mapping is leaked. The check-then-act pattern used by callers (Get → miss → Add) is also not atomic under concurrent readers, so duplicates can be created even by "guarded" call sites.

Location

func (c *LRUCache) Add(key any, value any) {
	...
	if c.l.Len() >= c.cap {
		c.removeOldest()
	}
	e := &LruEntry{Key: key, Value: value}
	entry := c.l.PushFront(e)
	c.m[key] = entry        // old element for `key` remains in c.l
}

func (c *LRUCache) removeOldest() {
	entry := c.l.Back()
	...
	key := entry.Value.(*LruEntry).Key
	delete(c.m, key)        // may delete the LIVE entry of a duplicated key
	c.l.Remove(entry)
}

Problem

Standard LRU semantics require update-on-existing (MoveToFront + value overwrite). Here, Add on an existing key instead creates a duplicate list node:

  1. The list grows beyond the number of distinct keys, so Len() (used for the capacity check at line 35) no longer reflects real occupancy — eviction fires too early or too late.
  2. When a stale duplicate eventually reaches the back of the list, removeOldest runs delete(c.m, key) against the map entry that now points to the newer front element. The live key disappears from the map while its ghost element remains in the list forever.
  3. In internal/fileio/rwmanger_mmap.go (accessMMap, lines 153–165), each Add stores a fresh *mmapData created by newMMapData (a new mmap region). When two concurrent readers both observe a Get miss for the same offset and both call Add, one of the two mappings becomes unreachable except through a doomed list element — and nothing ever calls munmap on it when it is removed from the list.

Concretely, in mmap mode with HintKeyAndRAMIdxCacheSize > 0 or default mmap caching:

// accessMMap — not atomic:
item := cache.Get(offset)      // reader A: nil;  reader B: nil  (same offset)
...
cache.Add(offset, newItem)     // A adds elem1; B adds elem2 → duplicate + leaked mmapData

Read-only transactions in nutsdb run concurrently, so two View transactions touching the same data-file block can hit this window.

Trigger / Reproduction

Static analysis finding — behavior derived from source at master (5ee3eaf9); not confirmed by execution.

Deterministic single-thread reproduction of the corruption logic:

c := utils.NewLruCache(3)
c.Add("A", 1); c.Add("B", 2); c.Add("C", 3) // [C B A], len=cap
c.Add("C", 33)                              // evicts A; pushes C' → list [C' C B]; m has no A, C→elem(C')
c.Get("C")                                  // hit, MoveToFront(elem(C')) → [C' C B] unchanged
c.Add("D", 4)                               // len(3)>=3 → removeOldest removes B ✓
c.Add("E", 5)                               // removeOldest removes BACK = stale C element
                                            //   → delete(m,"C") deletes the LIVE C entry!
c.Get("C")                                  // == nil although C was recently added and cached

The concurrency variant needs no artificial setup: simultaneous first reads of the same block via MMapRWManager.ReadAt from two goroutines.

The existing tests in internal/utils/lru_test.go only add distinct keys, so none of this is covered.

Expected Behavior

  • Add on an existing key should update the value and move the existing element to the front (no second element).
  • Eviction should unmap/release resources held by evicted values where the value owns them (or the cache should document that ownership stays with the caller).
  • The Get→Add sequence in accessMMap should be atomic with respect to other readers (single lookup-and-insert method, or per-key locking).

Actual Behavior

Duplicate elements accumulate; Len() diverges from distinct-key count; evictions delete live entries while ghosts persist; mmap regions created by racing readers are never unmapped.

Impact

Progressive memory growth (ghost elements + leaked mmap regions) and incorrect cache behavior over time in long-lived processes using mmap RW mode or HintKeyAndRAMIdxMode. Data itself is not corrupted — the mappings are still valid memory — but the cache's accounting and eviction decisions become wrong, and leaked mappings are never released until process exit.

Suggested Direction

Rewrite Add as get-or-insert:

if elem, ok := c.m[key]; ok {
    elem.Value.(*LruEntry).Value = value
    c.l.MoveToFront(elem)
    return
}

and have accessMMap use a combined GetOrAdd (or hold the manager-level lock across the check+insert). If values own OS resources, add an optional onEvict func(key, value) callback so mmap caches can unmap evicted blocks.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

No labels
No labels

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions


    Back | FazBrowse Home | New Git URL