Skip to content

Commit

Permalink
Fix dict use-after-free problem in kvs->rehashing (#13154)
Browse files Browse the repository at this point in the history
In ASAN CI, we find server may crash because of NULL ptr in `kvstoreIncrementallyRehash`.
the reason is that we use two phase unlink in `dbGenericDelete`. After `kvstoreDictTwoPhaseUnlinkFind`,
the dict may be in rehashing and only have one element in ht[0] of `db->keys`.

When we delete the last element in `db->keys` meanwhile `db->keys` is in rehashing, we may free the
dict in `kvstoreDictTwoPhaseUnlinkFree` without deleting the node in `kvs->rehashing`. Then we may
use this freed ptr in `kvstoreIncrementallyRehash` in the `serverCron` and cause the crash.
This is indeed a use-after-free problem.

The fix is to call rehashingCompleted in dictRelease and dictEmpty, so that every call for
rehashingStarted is always matched with a rehashingCompleted.

Adding a test in the unit test to catch it consistently

---------

Co-authored-by: Oran Agra <oran@redislabs.com>
Co-authored-by: debing.sun <debing.sun@redis.com>
  • Loading branch information
3 people committed Mar 20, 2024
1 parent bad33f8 commit e64d91c
Show file tree
Hide file tree
Showing 2 changed files with 11 additions and 0 deletions.
8 changes: 8 additions & 0 deletions src/dict.c
Expand Up @@ -706,6 +706,10 @@ int _dictClear(dict *d, int htidx, void(callback)(dict*)) {
/* Clear & Release the hash table */
void dictRelease(dict *d)
{
/* Someone may be monitoring a dict that started rehashing, before
* destroying the dict fake completion. */
if (dictIsRehashing(d) && d->type->rehashingCompleted)
d->type->rehashingCompleted(d);
_dictClear(d,0,NULL);
_dictClear(d,1,NULL);
zfree(d);
Expand Down Expand Up @@ -1588,6 +1592,10 @@ void *dictFindPositionForInsert(dict *d, const void *key, dictEntry **existing)
}

void dictEmpty(dict *d, void(callback)(dict*)) {
/* Someone may be monitoring a dict that started rehashing, before
* destroying the dict fake completion. */
if (dictIsRehashing(d) && d->type->rehashingCompleted)
d->type->rehashingCompleted(d);
_dictClear(d,0,callback);
_dictClear(d,1,callback);
d->rehashidx = -1;
Expand Down
3 changes: 3 additions & 0 deletions src/kvstore.c
Expand Up @@ -957,6 +957,9 @@ int kvstoreTest(int argc, char **argv, int flags) {
}
kvstoreIteratorRelease(kvs_it);

/* Make sure the dict was removed from the rehashing list. */
while (kvstoreIncrementallyRehash(kvs2, 1000)) {}

dict *d = kvstoreGetDict(kvs2, didx);
assert(d == NULL);
assert(kvstoreDictSize(kvs2, didx) == 0);
Expand Down

0 comments on commit e64d91c

Please sign in to comment.