Skip to content

fix: prevent panic during eviction when short dummy entries are encountered - #438

Open
AdamMagued wants to merge 1 commit into
allegro:mainfrom
AdamMagued:fix-eviction-dummy-entry-panic
Open

AdamMagued wants to merge 1 commit into
allegro:mainfrom
AdamMagued:fix-eviction-dummy-entry-panic

Conversation

@AdamMagued

Copy link
Copy Markdown

When BytesQueue allocates additional memory, it may insert a dummy/padding entry into the queue to fill fragmented space between tail and head. Such dummy entries can have a length shorter than the minimum header size (headersSizeInBytes = 18, e.g. 0 to 17 bytes).

During expiration or eviction in cleanUp, Set, or onEvict, isExpired and removeOldestEntry attempt to decode headers from the entry. Without checking len(oldestEntry) < headersSizeInBytes, slicing or binary decoding triggered a runtime slice bounds out of range panic (runtime error: index out of range [7] with length 0).

Changes:

  • In shard.go:
    • In isExpired, safely check len(oldestEntry) < headersSizeInBytes and return true so dummy/padding entries are evicted safely.
    • In removeOldestEntry, verify len(oldest) >= headersSizeInBytes before decoding the hash or invoking onRemove.
    • In entry retrieval and delete methods, guard against entries shorter than headersSizeInBytes.
  • In encoding.go:
    • Add slice bounds checks to readEntry, readTimestampFromEntry, readKeyFromEntry, compareKeyFromEntry, readHashFromEntry, and resetHashFromEntry.
  • Add unit tests in shard_test.go, bigcache_test.go, and encoding_test.go verifying that short entries (< 18 bytes) do not cause panics during eviction or decoding.

Fixes #312

…ntered

When BytesQueue reallocates memory, it can insert padding/dummy entries (< 18 bytes) into the queue. When isExpired or removeOldestEntry processed these short entries without verifying the length against headersSizeInBytes, a runtime slice bounds panic was triggered.

Check entry lengths safely before reading headers in isExpired, removeOldestEntry, and encoding helpers, safely treating short entries as expired so they can be evicted.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

shard onEvict get an empty oldestEntry.so panic and not unLock

1 participant