From 6ae3673b5e4fd3d00cbc9d14e7c0953344b85f08 Mon Sep 17 00:00:00 2001
From: perryitay <85821686+perryitay@users.noreply.github.com>
Date: Sun, 11 Jul 2021 13:17:23 +0300
Subject: [PATCH] Fail EXEC command in case a watched key is expired (#9194)

There are two issues fixed in this commit:
1. we want to fail the EXEC command in case there is a watched key that's logically
   expired but not yet deleted by active expire or lazy expire.
2. we saw that currently cache time is update in every `call()` (including nested calls),
   this time is being also being use for the isKeyExpired comparison, we want to update
   the cache time only in the first call (execCommand)

Co-authored-by: Oran Agra <oran@redislabs.com>
(cherry picked from commit ac8b1df8850cc80fbf9ce8c2fbde0c1d3a1b4e91)
---
 src/multi.c          | 21 +++++++++++++++++++++
 src/server.c         | 10 +++++++---
 src/server.h         |  2 ++
 tests/unit/multi.tcl | 16 ++++++++++++++++
 4 files changed, 46 insertions(+), 3 deletions(-)

diff --git a/src/multi.c b/src/multi.c
index 5fb37098a..3a157bee6 100644
--- a/src/multi.c
+++ b/src/multi.c
@@ -168,6 +168,11 @@ void execCommand(client *c) {
         return;
     }
 
+    /* EXEC with expired watched key is disallowed*/
+    if (isWatchedKeyExpired(c)) {
+        c->flags |= (CLIENT_DIRTY_CAS);
+    }
+
     /* Check if we need to abort the EXEC because:
      * 1) Some WATCHed key was touched.
      * 2) There was a previous error while queueing commands.
@@ -342,6 +347,22 @@ void unwatchAllKeys(client *c) {
     }
 }
 
+/* iterates over the watched_keys list and
+ * look for an expired key . */
+int isWatchedKeyExpired(client *c) {
+    listIter li;
+    listNode *ln;
+    watchedKey *wk;
+    if (listLength(c->watched_keys) == 0) return 0;
+    listRewind(c->watched_keys,&li);
+    while ((ln = listNext(&li))) {
+        wk = listNodeValue(ln);
+        if (keyIsExpired(wk->db, wk->key)) return 1;
+    }
+
+    return 0;
+}
+
 /* "Touch" a key, so that if this key is being WATCHed by some client the
  * next EXEC will fail. */
 void touchWatchedKey(redisDb *db, robj *key) {
diff --git a/src/server.c b/src/server.c
index 516bcb114..6c001b266 100644
--- a/src/server.c
+++ b/src/server.c
@@ -3697,8 +3697,6 @@ void call(client *c, int flags) {
     struct redisCommand *real_cmd = c->cmd;
     static long long prev_err_count;
 
-    server.fixed_time_expire++;
-
     /* Initialization: clear the flags that must be set by the command on
      * demand, and initialize the array for additional commands propagation. */
     c->flags &= ~(CLIENT_FORCE_AOF|CLIENT_FORCE_REPL|CLIENT_PREVENT_PROP);
@@ -3708,7 +3706,13 @@ void call(client *c, int flags) {
     /* Call the command. */
     dirty = server.dirty;
     prev_err_count = server.stat_total_error_replies;
-    updateCachedTime(0);
+
+    /* Update cache time, in case we have nested calls we want to
+     * update only on the first call*/
+    if (server.fixed_time_expire++ == 0) {
+        updateCachedTime(0);
+    }
+
     elapsedStart(&call_timer);
     c->cmd->proc(c);
     const long duration = elapsedUs(call_timer);
diff --git a/src/server.h b/src/server.h
index 86c926805..64d08db60 100644
--- a/src/server.h
+++ b/src/server.h
@@ -1945,6 +1945,7 @@ void initClientMultiState(client *c);
 void freeClientMultiState(client *c);
 void queueMultiCommand(client *c);
 void touchWatchedKey(redisDb *db, robj *key);
+int isWatchedKeyExpired(client *c);
 void touchAllWatchedKeysInDb(redisDb *emptied, redisDb *replaced_with);
 void discardTransaction(client *c);
 void flagTransaction(client *c);
@@ -2318,6 +2319,7 @@ void initConfigValues();
 /* db.c -- Keyspace access API */
 int removeExpire(redisDb *db, robj *key);
 void propagateExpire(redisDb *db, robj *key, int lazy);
+int keyIsExpired(redisDb *db, robj *key);
 int expireIfNeeded(redisDb *db, robj *key);
 long long getExpire(redisDb *db, robj *key);
 void setExpire(client *c, redisDb *db, robj *key, long long when);
diff --git a/tests/unit/multi.tcl b/tests/unit/multi.tcl
index e22b6d43d..d33f94515 100644
--- a/tests/unit/multi.tcl
+++ b/tests/unit/multi.tcl
@@ -121,6 +121,22 @@ start_server {tags {"multi"}} {
         r exec
     } {}
 
+    test {EXEC fail on lazy expired WATCHed key} {
+        r flushall
+        r debug set-active-expire 0
+
+        r del key
+        r set key 1 px 2
+        r watch key
+
+        after 100
+
+        r multi
+        r incr key
+        assert_equal [r exec] {}
+        r debug set-active-expire 1
+    } {OK} {needs:debug}
+
     test {After successful EXEC key is no longer watched} {
         r set x 30
         r watch x