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.

2 files changed, 21 insertions, 6 deletions

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 b00118681..b821f0c31 100644
--- a/src/module.c
+++ b/src/module.c
@@ -6553,9 +6553,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;
@@ -6633,9 +6637,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;