From 6cf8fc08f59cd34cfcf364ab6a233f100611078c Mon Sep 17 00:00:00 2001
From: Shaya Potter <shaya@redislabs.com>
Date: Mon, 20 Mar 2023 08:04:13 +0200
Subject: [PATCH] Don't run command filter on blocked command reprocessing
 (#11895)

Previously we would run the module command filters even upon blocked
command reprocessing.  This could modify the command, and it's args.
This is irrelevant in the context of a command being reprocessed (it already
went through the filters), as well as breaks the crashed command lookup
that exists in the case of a reprocessed command.

fixes #11894.

Co-authored-by: Oran Agra <oran@redislabs.com>
---
 src/server.c                           |  7 +++---
 tests/modules/commandfilter.c          | 31 +++++++++++++++++++++++++-
 tests/unit/moduleapi/commandfilter.tcl | 25 +++++++++++++++++++++
 3 files changed, 59 insertions(+), 4 deletions(-)

diff --git a/src/server.c b/src/server.c
index 0990ff7fa..4e77d2f6d 100644
--- a/src/server.c
+++ b/src/server.c
@@ -3813,14 +3813,15 @@ int processCommand(client *c) {
         serverAssert(!scriptIsRunning());
     }
 
-    moduleCallCommandFilters(c);
-
     /* in case we are starting to ProcessCommand and we already have a command we assume
      * this is a reprocessing of this command, so we do not want to perform some of the actions again. */
     int client_reprocessing_command = c->cmd ? 1 : 0;
 
-    if (!client_reprocessing_command)
+    /* only run command filter if not reprocessing command */
+    if (!client_reprocessing_command) {
+        moduleCallCommandFilters(c);
         reqresAppendRequest(c);
+    }
 
     /* Handle possible security attacks. */
     if (!strcasecmp(c->argv[0]->ptr,"host:") || !strcasecmp(c->argv[0]->ptr,"post")) {
diff --git a/tests/modules/commandfilter.c b/tests/modules/commandfilter.c
index 8ed779714..043b6805f 100644
--- a/tests/modules/commandfilter.c
+++ b/tests/modules/commandfilter.c
@@ -10,7 +10,7 @@ static const char retained_command_name[] = "commandfilter.retained";
 static const char unregister_command_name[] = "commandfilter.unregister";
 static int in_log_command = 0;
 
-static RedisModuleCommandFilter *filter;
+static RedisModuleCommandFilter *filter, *filter1;
 static RedisModuleString *retained;
 
 int CommandFilter_UnregisterCommand(RedisModuleCtx *ctx, RedisModuleString **argv, int argc)
@@ -89,6 +89,32 @@ int CommandFilter_LogCommand(RedisModuleCtx *ctx, RedisModuleString **argv, int
     return REDISMODULE_OK;
 }
 
+/* Filter to protect against Bug #11894 reappearing
+ *
+ * ensures that the filter is only run the first time through, and not on reprocessing
+ */
+void CommandFilter_BlmoveSwap(RedisModuleCommandFilterCtx *filter)
+{
+    if (RedisModule_CommandFilterArgsCount(filter) != 6)
+        return;
+
+    RedisModuleString *arg = RedisModule_CommandFilterArgGet(filter, 0);
+    size_t arg_len;
+    const char *arg_str = RedisModule_StringPtrLen(arg, &arg_len);
+
+    if (arg_len != 6 || strncmp(arg_str, "blmove", 6))
+        return;
+
+    /*
+     * Swapping directional args (right/left) from source and destination.
+     * need to hold here, can't push into the ArgReplace func, as it will cause other to freed -> use after free
+     */
+    RedisModuleString *dir1 = RedisModule_HoldString(NULL, RedisModule_CommandFilterArgGet(filter, 3));
+    RedisModuleString *dir2 = RedisModule_HoldString(NULL, RedisModule_CommandFilterArgGet(filter, 4));
+    RedisModule_CommandFilterArgReplace(filter, 3, dir2);
+    RedisModule_CommandFilterArgReplace(filter, 4, dir1);
+}
+
 void CommandFilter_CommandFilter(RedisModuleCommandFilterCtx *filter)
 {
     if (in_log_command) return;  /* don't process our own RM_Call() from CommandFilter_LogCommand() */
@@ -170,6 +196,9 @@ int RedisModule_OnLoad(RedisModuleCtx *ctx, RedisModuleString **argv, int argc)
                     noself ? REDISMODULE_CMDFILTER_NOSELF : 0))
             == NULL) return REDISMODULE_ERR;
 
+    if ((filter1 = RedisModule_RegisterCommandFilter(ctx, CommandFilter_BlmoveSwap, 0)) == NULL)
+        return REDISMODULE_ERR;
+
     return REDISMODULE_OK;
 }
 
diff --git a/tests/unit/moduleapi/commandfilter.tcl b/tests/unit/moduleapi/commandfilter.tcl
index 427609d3e..d6193b1ca 100644
--- a/tests/unit/moduleapi/commandfilter.tcl
+++ b/tests/unit/moduleapi/commandfilter.tcl
@@ -116,3 +116,28 @@ test {RM_CommandFilterArgInsert and script argv caching} {
     }
 }
 
+# previously, there was a bug that command filters would be rerun (which would cause args to swap back)
+# this test is meant to protect against that bug
+test {Blocking Commands don't run through command filter when reprocessed} {
+    start_server {tags {"modules"}} {
+        r module load $testmodule log-key 0
+
+        r del list1{t}
+        r del list2{t}
+
+        r lpush list2{t} a b c d e
+
+        set rd [redis_deferring_client]
+        # we're asking to pop from the left, but the command filter swaps the two arguments,
+        # if it didn't swap it, we would end up with e d c b a 5 (5 being the left most of the following lpush)
+        # but since we swap the arguments, we end up with 1 e d c b a (1 being the right most of it).
+        # if the command filter would run again on unblock, they would be swapped back.
+        $rd blmove list1{t} list2{t} left right 0
+        wait_for_blocked_client
+        r lpush list1{t} 1 2 3 4 5
+        # validate that we moved the correct element with the swapped args
+        assert_equal [$rd read] 1
+        # validate that we moved the correct elements to the correct side of the list
+        assert_equal [r lpop list2{t}] 1
+    }
+}