From 48cde3fe4710b43be6537854ab464b8568616fb8 Mon Sep 17 00:00:00 2001
From: antirez <antirez@gmail.com>
Date: Fri, 16 Aug 2013 14:08:04 +0200
Subject: [PATCH] dict.c iterator API misuse protection.

dict.c allows the user to create unsafe iterators, that are iterators
that will not touch the dictionary data structure in any way, preventing
copy on write, but at the same time are limited in their usage.

The limitation is that when itearting with an unsafe iterator, no call
to other dictionary functions must be done inside the iteration loop,
otherwise the dictionary may be incrementally rehashed resulting into
missing elements in the set of the elements returned by the iterator.

However after introducing this kind of iterators a number of bugs were
found due to misuses of the API, and we are still finding
bugs about this issue. The bugs are not trivial to track because the
effect is just missing elements during the iteartion.

This commit introduces auto-detection of the API misuse. The idea is
that an unsafe iterator has a contract: from initialization to the
release of the iterator the dictionary should not change.

So we take a fingerprint of the dictionary state, xoring a few important
dict properties when the unsafe iteartor is initialized. We later check
when the iterator is released if the fingerprint is still the same. If it
is not, we found a misuse of the iterator, as not allowed API calls
changed the internal state of the dictionary.

This code was checked against a real bug, issue #1240.

This is what Redis prints (aborting) when a misuse is detected:

Assertion failed: (iter->fingerprint == dictFingerprint(iter->d)),
function dictReleaseIterator, file dict.c, line 587.
---
 src/dict.c | 34 ++++++++++++++++++++++++++++++----
 src/dict.h |  1 +
 2 files changed, 31 insertions(+), 4 deletions(-)

diff --git a/src/dict.c b/src/dict.c
index 2346f5be..26d0b1ff 100644
--- a/src/dict.c
+++ b/src/dict.c
@@ -505,6 +505,24 @@ void *dictFetchValue(dict *d, const void *key) {
     return he ? dictGetVal(he) : NULL;
 }
 
+/* A fingerprint is a 64 bit number that represents the state of the dictionary
+ * at a given time, it's just a few dict properties xored together.
+ * When an unsafe iterator is initialized, we get the dict fingerprint, and check
+ * the fingerprint again when the iterator is released.
+ * If the two fingerprints are different it means that the user of the iterator
+ * performed forbidden operations against the dictionary while iterating. */
+long long dictFingerprint(dict *d) {
+    long long fingerprint = 0;
+
+    fingerprint ^= (long long) d->ht[0].table;
+    fingerprint ^= (long long) d->ht[0].size;
+    fingerprint ^= (long long) d->ht[0].used;
+    fingerprint ^= (long long) d->ht[1].table;
+    fingerprint ^= (long long) d->ht[1].size;
+    fingerprint ^= (long long) d->ht[1].used;
+    return fingerprint;
+}
+
 dictIterator *dictGetIterator(dict *d)
 {
     dictIterator *iter = zmalloc(sizeof(*iter));
@@ -530,8 +548,12 @@ dictEntry *dictNext(dictIterator *iter)
     while (1) {
         if (iter->entry == NULL) {
             dictht *ht = &iter->d->ht[iter->table];
-            if (iter->safe && iter->index == -1 && iter->table == 0)
-                iter->d->iterators++;
+            if (iter->index == -1 && iter->table == 0) {
+                if (iter->safe)
+                    iter->d->iterators++;
+                else
+                    iter->fingerprint = dictFingerprint(iter->d);
+            }
             iter->index++;
             if (iter->index >= (signed) ht->size) {
                 if (dictIsRehashing(iter->d) && iter->table == 0) {
@@ -558,8 +580,12 @@ dictEntry *dictNext(dictIterator *iter)
 
 void dictReleaseIterator(dictIterator *iter)
 {
-    if (iter->safe && !(iter->index == -1 && iter->table == 0))
-        iter->d->iterators--;
+    if (!(iter->index == -1 && iter->table == 0)) {
+        if (iter->safe)
+            iter->d->iterators--;
+        else
+            assert(iter->fingerprint == dictFingerprint(iter->d));
+    }
     zfree(iter);
 }
 
diff --git a/src/dict.h b/src/dict.h
index 3a311f17..4d750ae8 100644
--- a/src/dict.h
+++ b/src/dict.h
@@ -88,6 +88,7 @@ typedef struct dictIterator {
     dict *d;
     int table, index, safe;
     dictEntry *entry, *nextEntry;
+    long long fingerprint; /* unsafe iterator fingerprint for misuse detection */
 } dictIterator;
 
 /* This is the initial size of every hash table */