Skip to content

test(lru): demonstrate TOCTOU race in lru implementation#4865

Draft
kislaykishore wants to merge 2 commits into
masterfrom
toctou-bug
Draft

test(lru): demonstrate TOCTOU race in lru implementation#4865
kislaykishore wants to merge 2 commits into
masterfrom
toctou-bug

Conversation

@kislaykishore

@kislaykishore kislaykishore commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator

Description

When downloading sparse files, cached objects grow incrementally from 0 bytes as chunks arrive. Callers invoke cache.UpdateSize(key, delta) to dynamically update LRU size accounting (currentSize). (Note: Normal sequential file-cache is immune to this issue because its full file size is allocated upfront at insertion time).

  • The Race: If a sparse object is concurrently evicted or erased via Erase(key) after its in-memory size changes but before UpdateSize finishes, Erase subtracts the new size instead of the size known to the cache.
  • The Impact: This causes currentSize underflows (wrapping around unsigned uint64), leading to invariant check panics or aggressive, endless cache eviction. Additionally, if the same key is rapidly re-inserted, stale deltas from the old object can corrupt the new entry's accounting.

Link to the issue in case of a bug fix.

N/A

Testing details

  1. Manual - Analyzed TOCTOU race condition and verified sparse-file vs normal file-cache accounting differences.
  2. Unit tests - Executed go test -v ./internal/cache/lru/... on TestMapCache_UpdateSize_TOCTOU_Underflow with maxSize=2000 which successfully reproduced and demonstrated the underflow (CurrentSize 18446744073709551586 over maxSize 2000).
  3. Integration tests - N/A

Any backward incompatible change? If so, please explain.

N/A

@kislaykishore
kislaykishore requested a review from a team as a code owner July 10, 2026 10:40
@github-actions github-actions Bot added the remind-reviewers Auto remind reviewers in attention set for review post 24hrs of inactivity on PR. label Jul 10, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request adds a new test, TestMapCache_UpdateSize_TOCTOU_Underflow, to demonstrate a TOCTOU race and size accounting underflow in the LRU cache. The reviewer pointed out that the cache size of 100 in the test is too small and can lead to false-positive test failures when cumulative updates exceed the cache limit. They suggested increasing the cache size to 2000 to ensure the test only fails on actual underflow issues.

Comment thread internal/cache/lru/lru_test.go Outdated
@kislaykishore
kislaykishore marked this pull request as draft July 10, 2026 10:43
@kislaykishore kislaykishore changed the title test(lru): simplify TestMapCache_UpdateSize_TOCTOU_Underflow to use strictly positive delta test(lru): demonstrate TOCTOU race in lru implementation Jul 10, 2026
@codecov

codecov Bot commented Jul 10, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 83.72%. Comparing base (4401c60) to head (0eda53e).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4865      +/-   ##
==========================================
+ Coverage   83.68%   83.72%   +0.04%     
==========================================
  Files         170      170              
  Lines       20948    20948              
==========================================
+ Hits        17530    17539       +9     
+ Misses       2762     2757       -5     
+ Partials      656      652       -4     
Flag Coverage Δ
unittests 83.72% <ø> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

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

Labels

remind-reviewers Auto remind reviewers in attention set for review post 24hrs of inactivity on PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant