Skip to content

cache: fix cache eviction using deallocated key#5088

Merged
ethomson merged 1 commit into
libgit2:masterfrom
pks-t:pks/cache-eviction-segfault
May 24, 2019
Merged

cache: fix cache eviction using deallocated key#5088
ethomson merged 1 commit into
libgit2:masterfrom
pks-t:pks/cache-eviction-segfault

Conversation

@pks-t

@pks-t pks-t commented May 24, 2019

Copy link
Copy Markdown
Member

When evicting cache entries, we first retrieve the object that is
to be evicted, delete the object and then finally delete the key
from the cache. In case where the cache eviction caused us to
free the cached object, though, its key will point to invalid
memory now when trying to remove it from the cache map. On my
system, this causes us to not properly remove the key from the
map, as its memory has been overwritten already and thus the key
lookup it will fail and we cannot delete it.

Fix this by only decrementing the refcount of the evictee after
we have removed it from our cache map. Add a test that caused a
segfault previous to that change.

Fixes #5081

When evicting cache entries, we first retrieve the object that is
to be evicted, delete the object and then finally delete the key
from the cache. In case where the cache eviction caused us to
free the cached object, though, its key will point to invalid
memory now when trying to remove it from the cache map. On my
system, this causes us to not properly remove the key from the
map, as its memory has been overwritten already and thus the key
lookup it will fail and we cannot delete it.

Fix this by only decrementing the refcount of the evictee after
we have removed it from our cache map. Add a test that caused a
segfault previous to that change.
@pks-t pks-t mentioned this pull request May 24, 2019
@pks-t

pks-t commented May 24, 2019

Copy link
Copy Markdown
Member Author

Verified by @eaigner to fix the issue in #5081

@ethomson

Copy link
Copy Markdown
Member

Nice, thanks the helpful report @eaigner and the fix @pks-t.

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.

cache: crash on evict

2 participants