Bound the block caches by bytes held instead of by entry count

The read caches sized themselves as MAX_CACHE_SIZE / (SectorSize << shift),
but ctx->shift is only ever assigned on the DDT v1 path, and the context is
zeroed at open. On every DDT v2 image the divisor therefore collapsed to the
sector size and the limit came out as 262144 entries, while each block_cache
entry holds a whole decompressed data block of SectorSize << dataShift bytes,
8 MiB with the default data_shift of 12. That is a ceiling of roughly 2 TiB
rather than the intended 512 MiB, so the count limit was never reached: a
sequential sector-by-sector pass cached the entire decompressed image and
freed none of it until aaruf_close(). Comparing two 40 GB images this way was
killed by the OOM reaper.

An entry count cannot express a memory budget when the values vary in size,
and here they vary by five orders of magnitude: block_header_cache holds
BlockHeader structs while block_cache holds megabyte payloads, yet both were
given the same limit. Track the byte size of each value instead and evict
least-recently-used entries until the cache is back under budget, and drop the
block geometry from cache initialization entirely. The block cache gets the
512 MiB MAX_CACHE_SIZE it always intended; block headers get a separate 8 MiB
budget, which covers far more blocks than the payload cache can hold.

The entry just inserted is never evicted, so callers may keep using the
pointer they handed over for the rest of the call. That also removes the
use-after-free that a zero limit used to cause: the old eviction loop started
at the hash head and could free the entry just added, after which the caller
read from freed memory.

include/aaruformat/consts.h

@@ -78,6 +78,10 @@
  *  still enabling efficient sequential and moderate random access patterns. */
 #define MAX_CACHE_SIZE 536870912ULL
 
+/** Maximum block header cache size (bytes). Block headers are a few dozen bytes each, so 8 MiB
+ *  keeps headers cached for far more blocks than MAX_CACHE_SIZE can hold payloads for. */
+#define MAX_HEADER_CACHE_SIZE 8388608ULL
+
 /** Size in bytes of the fixed LZMA properties header (lc/lp/pb + dictionary size). */
 #define LZMA_PROPERTIES_LENGTH 5
 

include/aaruformat/lru.h

@@ -5,6 +5,7 @@
 #ifndef LIBAARUFORMAT_LRU_H
 #define LIBAARUFORMAT_LRU_H
 
+#include <stddef.h>
 #include <stdint.h>
 #include <uthash.h>
 
@@ -24,29 +25,38 @@ struct CacheEntry
 {
     uint64_t       key;    ///< 64-bit integer key (unique within the cache).
     void          *value;  ///< Opaque value pointer associated with key.
+    size_t         size;   ///< Size in bytes of the memory pointed to by value.
     UT_hash_handle hh;     ///< uthash handle (must remain per uthash docs).
 };
 
 /** \struct CacheHeader
  *  \brief Cache top-level descriptor encapsulating the hash table root and capacity limit.
  *
- *  The cache enforces an upper bound (max_items) on the number of tracked entries. On insert,
- *  the oldest entry is evicted when the limit is reached (LRU via uthash insertion order).
+ *  The cache is bounded by the total number of *bytes* held by its values, not by the
+ *  number of entries: the caches in this library store values of wildly different sizes
+ *  (a block header is a few dozen bytes, a decompressed data block is megabytes), so an
+ *  entry-count limit cannot express a memory budget. On insert, the least recently used
+ *  entries are evicted until the cache is back under max_bytes.
  *
  *  Fields:
- *   - max_items: Maximum number of entries allowed; 0 means unlimited.
+ *   - max_bytes: Memory budget in bytes; 0 means unlimited.
+ *   - cur_bytes: Bytes currently held by the cached values.
  *   - cache:     uthash root pointer; NULL when the cache is empty.
  *   - free_func: Optional callback to free cached values on eviction/clear.
+ *
+ *  The entry just inserted is never evicted, so a caller may keep using the pointer it
+ *  handed over for the remainder of the call even if the value is larger than max_bytes.
  */
 struct CacheHeader
 {
-    uint64_t max_items;         ///< Hard limit for number of entries.
+    uint64_t max_bytes;         ///< Memory budget in bytes for the cached values. 0 means unlimited.
+    uint64_t cur_bytes;         ///< Bytes currently held by the cached values.
     struct CacheEntry *cache;   ///< Hash root (uthash). NULL when empty.
     void (*free_func)(void *);  ///< Optional callback to free cached values. NULL if not needed.
 };
 
 void *find_in_cache_uint64(struct CacheHeader *cache, uint64_t key);
-void  add_to_cache_uint64(struct CacheHeader *cache, uint64_t key, void *value);
+void  add_to_cache_uint64(struct CacheHeader *cache, uint64_t key, void *value, size_t size);
 void  free_cache(struct CacheHeader *cache);
 
 #endif  // LIBAARUFORMAT_LRU_H

src/create.c

@@ -402,10 +402,11 @@ AARU_EXPORT void *AARU_CALL aaruf_create(const char *filepath, const uint32_t me
     // Initialize caches
     TRACE("Initializing caches");
     ctx->block_header_cache.cache     = NULL;
-    const uint64_t cache_divisor      = (uint64_t)ctx->image_info.SectorSize * (1ULL << ctx->shift);
-    ctx->block_header_cache.max_items = cache_divisor == 0 ? 0 : MAX_CACHE_SIZE / cache_divisor;
+    ctx->block_header_cache.cur_bytes = 0;
+    ctx->block_header_cache.max_bytes = MAX_HEADER_CACHE_SIZE;
     ctx->block_cache.cache            = NULL;
-    ctx->block_cache.max_items        = ctx->block_header_cache.max_items;
+    ctx->block_cache.cur_bytes        = 0;
+    ctx->block_cache.max_bytes        = MAX_CACHE_SIZE;
 
     // TODO: Cache tracks and sessions?
 

src/erasure.c

@@ -1507,14 +1507,14 @@ int32_t ec_recover_data_block(aaruformat_context *ctx, uint64_t block_offset, ui
 
                 /* Cache the recovered block so subsequent sector reads from the same
                  * block don't re-trigger recovery (this is the critical optimization). */
-                add_to_cache_uint64(&ctx->block_cache, block_offset, block);
+                add_to_cache_uint64(&ctx->block_cache, block_offset, block, recovered_header.length);
 
                 /* Also cache the recovered BlockHeader */
                 BlockHeader *cached_hdr = (BlockHeader *)malloc(sizeof(BlockHeader));
                 if(cached_hdr)
                 {
                     memcpy(cached_hdr, &recovered_header, sizeof(BlockHeader));
-                    add_to_cache_uint64(&ctx->block_header_cache, block_offset, cached_hdr);
+                    add_to_cache_uint64(&ctx->block_header_cache, block_offset, cached_hdr, sizeof(BlockHeader));
                 }
 
                 block = NULL; /* Ownership transferred to cache — don't free */

src/lru.c

@@ -11,6 +11,23 @@
 // eliminating all string conversion, malloc, and snprintf overhead
 // from the hot path.
 
+/**
+ * @brief Frees a cache entry's value and unlinks it from the cache.
+ *
+ * @param cache Pointer to the cache header.
+ * @param entry Entry to drop. Must be linked in cache.
+ */
+static void drop_entry(struct CacheHeader *cache, struct CacheEntry *entry)
+{
+    HASH_DELETE(hh, cache->cache, entry);
+
+    cache->cur_bytes -= entry->size;
+
+    if(cache->free_func && entry->value) cache->free_func(entry->value);
+
+    free(entry);
+}
+
 /**
  * @brief Finds a value in the cache by uint64_t key.
  *
@@ -36,38 +53,39 @@ void *find_in_cache_uint64(struct CacheHeader *cache, const uint64_t key)
 }
 
 /**
- * @brief Adds a value to the cache with a uint64_t key, evicting LRU if full.
+ * @brief Adds a value to the cache with a uint64_t key, evicting LRU entries if over budget.
  *
- * Adds a new entry to the cache. If the cache exceeds its maximum size,
- * evicts the least recently used (oldest insertion-order) entry.
+ * Adds a new entry to the cache. If the cache exceeds its memory budget, the least recently
+ * used entries are evicted until it fits again. The entry just inserted is never evicted, so the
+ * caller may keep using the pointer it handed over for the remainder of the call.
  *
  * @param cache Pointer to the cache header.
  * @param key 64-bit integer key to add.
  * @param value Pointer to the value to store.
+ * @param size Size in bytes of the memory pointed to by value.
  */
-void add_to_cache_uint64(struct CacheHeader *cache, const uint64_t key, void *value)
+void add_to_cache_uint64(struct CacheHeader *cache, const uint64_t key, void *value, const size_t size)
 {
     struct CacheEntry *entry = malloc(sizeof(struct CacheEntry));
     if(!entry) return;
 
     entry->key   = key;
     entry->value = value;
+    entry->size  = size;
     HASH_ADD(hh, cache->cache, key, sizeof(uint64_t), entry);
+    cache->cur_bytes += size;
 
-    // Evict oldest entry if cache exceeded capacity.
-    if(HASH_COUNT(cache->cache) > cache->max_items)
+    if(cache->max_bytes == 0) return;
+
+    // Evict least recently used entries until back under budget. Never evict the entry just
+    // inserted: the caller still uses that pointer after this returns.
+    while(cache->cur_bytes > cache->max_bytes && HASH_COUNT(cache->cache) > 1)
     {
-        struct CacheEntry *tmp_entry;
-        HASH_ITER(hh, cache->cache, entry, tmp_entry)
-        {
-            HASH_DELETE(hh, cache->cache, entry);
+        struct CacheEntry *oldest = cache->cache;
 
-            if(cache->free_func && entry->value)
-                cache->free_func(entry->value);
+        if(oldest == NULL || oldest == entry) break;
 
-            free(entry);
-            break;
-        }
+        drop_entry(cache, oldest);
     }
 }
 
@@ -85,15 +103,8 @@ void free_cache(struct CacheHeader *cache)
 
     if(!cache || !cache->cache) return;
 
-    HASH_ITER(hh, cache->cache, entry, tmp)
-    {
-        HASH_DELETE(hh, cache->cache, entry);
+    HASH_ITER(hh, cache->cache, entry, tmp) { drop_entry(cache, entry); }
 
-        if(cache->free_func && entry->value)
-            cache->free_func(entry->value);
-
-        free(entry);
-    }
-
-    cache->cache = NULL;
+    cache->cache     = NULL;
+    cache->cur_bytes = 0;
 }

src/open.c

@@ -718,17 +718,14 @@ AARU_EXPORT void *AARU_CALL aaruf_open(const char *filepath, const bool resume_m
     ctx->block_header_cache.free_func = free;
     ctx->block_cache.free_func        = free;
 
-    const uint64_t cache_divisor = (uint64_t)ctx->image_info.SectorSize * (1ULL << ctx->shift);
-    if(cache_divisor == 0)
-    {
-        ctx->block_header_cache.max_items = 0;
-        ctx->block_cache.max_items        = 0;
-    }
-    else
-    {
-        ctx->block_header_cache.max_items = MAX_CACHE_SIZE / cache_divisor;
-        ctx->block_cache.max_items        = ctx->block_header_cache.max_items;
-    }
+    // Both caches are bounded by bytes held, so no block geometry is needed here. Deriving an
+    // entry count from a block size got this wrong: the divisor used ctx->shift, which is only
+    // ever set for DDT v1 images, so on every DDT v2 image the limit came out ~262144 entries of
+    // up to 8 MiB each and the cache grew until the whole decompressed image was resident.
+    ctx->block_header_cache.cur_bytes = 0;
+    ctx->block_cache.cur_bytes        = 0;
+    ctx->block_header_cache.max_bytes = MAX_HEADER_CACHE_SIZE;
+    ctx->block_cache.max_bytes        = MAX_CACHE_SIZE;
 
     // TODO: Cache tracks and sessions?
 

src/read.c

@@ -605,7 +605,7 @@ AARU_EXPORT int32_t AARU_CALL aaruf_read_sector(void *context, const uint64_t se
         }
 
         TRACE("Adding block header to cache");
-        add_to_cache_uint64(&ctx->block_header_cache, block_offset, block_header);
+        add_to_cache_uint64(&ctx->block_header_cache, block_offset, block_header, sizeof(BlockHeader));
     }
     else if(aaruf_fseek(ctx->imageStream, (aaru_off_t)(block_offset + sizeof(BlockHeader)), SEEK_SET) != 0)
     {
@@ -1027,7 +1027,7 @@ AARU_EXPORT int32_t AARU_CALL aaruf_read_sector(void *context, const uint64_t se
 
     // Add block to cache
     TRACE("Adding block to cache");
-    add_to_cache_uint64(&ctx->block_cache, block_offset, block);
+    add_to_cache_uint64(&ctx->block_cache, block_offset, block, block_header->length);
 
     memcpy(data, block + offset * block_header->sectorSize, block_header->sectorSize);
     *length = block_header->sectorSize;