Repository navigation
Conversation
…ves it Signed-off-by: Sai Asish Y <say.apm35@gmail.com>
1a1a11a
left a comment
There was a problem hiding this comment.
Reviewed and independently validated commit 95b0820. The early return is the correct fix for #349; I found no blocking correctness issue in this patch.
Lookup semantics:
- With
update_cache=true, the algorithm's removal callback unlinks/deletes the expired object and updates object/byte counts. Returning immediately avoids both the NULL dereference and subsequent metadata updates on a removed object. - With
update_cache=false, an expired object still returns NULL, remains resident, and retains its metadata and accounting. This matches the pre-patch non-updating behavior. - FIFO, LRU, Sieve, and the FIFO sub-caches used by S3FIFO accept NULL as a miss.
cache_get_basethen follows the normal insertion path. The existing strict expiry boundary (exp_time < clock_time) and zero-expiry sentinel are preserved.
Independent execution on Linux / GCC 13:
| Configuration / check | Result |
|---|---|
Debug, -DSUPPORT_TTL=ON, full build + ctest --output-on-failure --parallel 4 |
10/10 CTest targets passed, including CLI; build warning-free |
Default Release (SUPPORT_TTL=OFF), full build + same CTest command |
10/10 CTest targets passed, including CLI; build warning-free |
Added /libCacheSim/cacheAlgo_LRU_expired_object test, run directly |
Passed |
| Additional local TTL contract checks for FIFO, LRU, Sieve, and S3FIFO | Passed, including request/default/zero TTL, exact expiry boundary, non-updating lookup without mutation, updating removal, accounting, repeated expiration/reinsertion, and S3FIFO main-queue expiration |
| Same contract checks with AddressSanitizer + UndefinedBehaviorSanitizer | Passed |
Baseline develop cache.c with the same sanitizer harness |
Reproduced the NULL dereference in cache_find_base for all four reported algorithms |
clang-format 18.1.8 --dry-run --Werror, both changed files |
Passed |
Execution limits: the Release build used the system allocator because optional tcmalloc was not discovered. LeakSanitizer could not enumerate sandboxed processes, so leak detection was disabled for the targeted ASan/UBSan harness. The additional contract harness was local validation, not an addition to this PR.
Non-blocking follow-up: parameterize the committed TTL regression over FIFO/LRU/Sieve/S3FIFO and include an expired find(..., false) case. A TTL-enabled CI configuration would keep this coverage active: the current build workflow uses TTL=OFF and compiles out the new regression.
Merge status: build, Code Quality, and CodeQL are currently action_required, each with zero jobs; only GitGuardian has passed. Please approve those first-time contributor workflow runs and obtain any required code-owner review before merging, consistent with CONTRIBUTING.md's green-CI requirement. The connected GitHub integration cannot read the full branch-protection settings, so this approval does not assert that every repository merge requirement is satisfied. Issue #349 can be closed when this fix reaches the default develop branch; the PR already contains Fixes #349.
|
Added the follow-up coverage in 2879afb: FIFO, LRU, Sieve and both S3FIFO queues now exercise expired non-updating lookups, unchanged residency/accounting, the expiry boundary and repeated removal/reinsertion. Ubuntu CI now runs TTL OFF and ON, preserving the existing All five regression cases pass on the repaired code and crash against the exact parent’s The changes and verification were AI-assisted and independently reviewed. |
What does this PR do?
With
-DSUPPORT_TTL=ON,cache_find_basesetscache_objto NULL after it has removed an expired object and then writesnext_access_vtimeandfreqthrough that pointer, so the first request for an expired object segfaults for every algorithm that goes through it. Return NULL right after the removal instead. Fixes #349.Type of change
How was it tested?
The new
/libCacheSim/cacheAlgo_LRU_expired_objecttest inserts an object with a 10 second TTL and requests it again 20 seconds later. Ondevelopit segfaults; with this change the second request is a miss and the object is reinserted. Built with-DSUPPORT_TTL=ON(Debug) for the new test and in plain Release for the CI configuration,ctestpasses in both.ctest --test-dir _build --output-on-failurepasses-Wall -Wextra -Werror)Checklist
clang-format(or the pre-commit hook fromscripts/setup_hooks.sh)