From 28ef18a8946815e0d83a1c0a9b6baf9d27022461 Mon Sep 17 00:00:00 2001 From: Oran Agra Date: Wed, 5 Feb 2020 18:24:14 +0200 Subject: RM_Scan disable dict rehashing The callback approach we took is very efficient, the module can do any filtering of keys without building any list and cloning strings, it can also read data from the key's value. but if the user tries to re-open the key, or any other key, this can cause dict re-hashing (dictFind does that), and that's very bad to do from inside dictScan. this commit protects the dict from doing any rehashing during scan, but also warns the user not to attempt any writes or command calls from within the callback, for fear of unexpected side effects and crashes. --- src/dict.c | 7 +++++++ src/module.c | 20 ++++++++++++++------ 2 files changed, 21 insertions(+), 6 deletions(-) (limited to 'src') diff --git a/src/dict.c b/src/dict.c index 106467ef7..93e6c39a7 100644 --- a/src/dict.c +++ b/src/dict.c @@ -871,6 +871,10 @@ unsigned long dictScan(dict *d, if (dictSize(d) == 0) return 0; + /* Having a safe iterator means no rehashing can happen, see _dictRehashStep. + * This is needed in case the scan callback tries to do dictFind or alike. */ + d->iterators++; + if (!dictIsRehashing(d)) { t0 = &(d->ht[0]); m0 = t0->sizemask; @@ -937,6 +941,9 @@ unsigned long dictScan(dict *d, } while (v & (m0 ^ m1)); } + /* undo the ++ at the top */ + d->iterators--; + return v; } diff --git a/src/module.c b/src/module.c index 914c50df3..e9bceac47 100644 --- a/src/module.c +++ b/src/module.c @@ -6515,9 +6515,13 @@ void RM_ScanCursorDestroy(RedisModuleScanCursor *cursor) { * } * RedisModule_ScanCursorDestroy(c); * - * The function will return 1 if there are more elements to scan and 0 otherwise, - * possibly setting errno if the call failed. - * It is also possible to restart and existing cursor using RM_CursorRestart. */ + * The function will return 1 if there are more elements to scan and 0 otherwise, + * possibly setting errno if the call failed. + * It is also possible to restart and existing cursor using RM_CursorRestart. + * + * NOTE: You must avoid doing any database changes from within the callback, you should avoid any + * RedisModule_OpenKey or RedisModule_Call, if you need to do these, you need to keep the key name + * and do any work you need to do after the call to Scan returns. */ int RM_Scan(RedisModuleCtx *ctx, RedisModuleScanCursor *cursor, RedisModuleScanCB fn, void *privdata) { if (cursor->done) { errno = ENOENT; @@ -6595,9 +6599,13 @@ static void moduleScanKeyCallback(void *privdata, const dictEntry *de) { * RedisModule_CloseKey(key); * RedisModule_ScanCursorDestroy(c); * - * The function will return 1 if there are more elements to scan and 0 otherwise, - * possibly setting errno if the call failed. - * It is also possible to restart and existing cursor using RM_CursorRestart. */ + * The function will return 1 if there are more elements to scan and 0 otherwise, + * possibly setting errno if the call failed. + * It is also possible to restart and existing cursor using RM_CursorRestart. + * + * NOTE: You must avoid doing any database changes from within the callback, you should avoid any + * RedisModule_OpenKey or RedisModule_Call, if you need to do these, you need to keep the field name + * and do any work you need to do after the call to Scan returns. */ int RM_ScanKey(RedisModuleKey *key, RedisModuleScanCursor *cursor, RedisModuleScanKeyCB fn, void *privdata) { if (key == NULL || key->value == NULL) { errno = EINVAL; -- cgit v1.2.1