From e64d91c37105bc2e23816b6f81b9ffc5e5d99801 Mon Sep 17 00:00:00 2001 From: Yanqi Lv Date: Thu, 21 Mar 2024 04:44:28 +0800 Subject: [PATCH] Fix dict use-after-free problem in kvs->rehashing (#13154) 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 Co-authored-by: debing.sun --- src/dict.c | 8 ++++++++ src/kvstore.c | 3 +++ 2 files changed, 11 insertions(+) diff --git a/src/dict.c b/src/dict.c index 6e9b13150..d04f9e123 100644 --- a/src/dict.c +++ b/src/dict.c @@ -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); @@ -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; diff --git a/src/kvstore.c b/src/kvstore.c index 505f92957..62b799ddd 100644 --- a/src/kvstore.c +++ b/src/kvstore.c @@ -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);