fix(server): reject eval inside transaction multi blocks #457 (#471)

fix(server): block running lua script inside pipeline #457

Signed-off-by: Boaz Sade <boaz@dragonflydb.io>

src/server/dragonfly_test.cc

@@ -78,6 +78,20 @@ TEST_F(DflyEngineTest, Sds) {
   sdsfreesplitres(argv, argc);
 }
 
+TEST_F(DflyEngineTest, MultiAndEval) {
+  RespExpr resp = Run({"multi"});
+  ASSERT_EQ(resp, "OK");
+
+  resp = Run({"get", kKey1});
+  ASSERT_EQ(resp, "QUEUED");
+
+  resp = Run({"get", kKey4});
+  ASSERT_EQ(resp, "QUEUED");
+  EXPECT_THAT(Run({"eval", "return redis.call('exists', KEYS[2])", "2", "a", "b"}),
+              ErrArg("'EVAL' Dragonfly does not allow execution of a server-side Lua script inside "
+                     "transaction block"));
+}
+
 TEST_F(DflyEngineTest, Multi) {
   RespExpr resp = Run({"multi"});
   ASSERT_EQ(resp, "OK");
@@ -189,9 +203,12 @@ TEST_F(DflyEngineTest, MultiConsistent) {
 }
 
 TEST_F(DflyEngineTest, MultiWeirdCommands) {
+  // FIXME: issue https://github.com/dragonflydb/dragonfly/issues/457
+  // once we would have fix for supporting EVAL from within transaction
   Run({"multi"});
-  ASSERT_EQ(Run({"eval", "return 42", "0"}), "QUEUED");
-  EXPECT_THAT(Run({"exec"}), IntArg(42));
+  EXPECT_THAT(Run({"eval", "return 42", "0"}),
+              ErrArg("'EVAL' Dragonfly does not allow execution of a server-side Lua script inside "
+                     "transaction block"));
 }
 
 TEST_F(DflyEngineTest, MultiRename) {

src/server/main_service.cc

@@ -73,6 +73,24 @@ std::optional<VarzFunction> engine_varz;
 
 constexpr size_t kMaxThreadSize = 1024;
 
+// Unwatch all keys for a connection and unregister from DbSlices.
+// Used by UNWATCH, DICARD and EXEC.
+void UnwatchAllKeys(ConnectionContext* cntx) {
+  auto& exec_info = cntx->conn_state.exec_info;
+  if (!exec_info.watched_keys.empty()) {
+    auto cb = [&](EngineShard* shard) {
+      shard->db_slice().UnregisterConnectionWatches(&exec_info);
+    };
+    shard_set->RunBriefInParallel(std::move(cb));
+  }
+  exec_info.ClearWatched();
+}
+
+void MultiCleanup(ConnectionContext* cntx) {
+  UnwatchAllKeys(cntx);
+  cntx->conn_state.exec_info.Clear();
+}
+
 void DeactivateMonitoring(ConnectionContext* server_ctx) {
   if (server_ctx->monitor) {
     // remove monitor on this connection
@@ -602,6 +620,15 @@ void Service::DispatchCommand(CmdArgList args, facade::ConnectionContext* cntx)
   etl.connection_stats.cmd_count_map[cmd_name]++;
 
   if (dfly_cntx->conn_state.exec_info.IsActive() && !is_trans_cmd) {
+    auto cmd_name = ArgS(args, 0);
+    if (cmd_name == "EVAL" || cmd_name == "EVALSHA") {
+      auto error =
+          absl::StrCat("'", cmd_name,
+                       "' Dragonfly does not allow execution of a server-side Lua script inside "
+                       "transaction block");
+      MultiSetError(dfly_cntx);
+      return (*cntx)->SendError(error);
+    }
     // TODO: protect against aggregating huge transactions.
     StoredCmd stored_cmd{cid};
     stored_cmd.cmd.reserve(args.size());
@@ -881,19 +908,6 @@ void Service::Watch(CmdArgList args, ConnectionContext* cntx) {
   return (*cntx)->SendOk();
 }
 
-// Unwatch all keys for a connection and unregister from DbSlices.
-// Used by UNWATCH, DICARD and EXEC.
-void UnwatchAllKeys(ConnectionContext* cntx) {
-  auto& exec_info = cntx->conn_state.exec_info;
-  if (!exec_info.watched_keys.empty()) {
-    auto cb = [&](EngineShard* shard) {
-      shard->db_slice().UnregisterConnectionWatches(&exec_info);
-    };
-    shard_set->RunBriefInParallel(std::move(cb));
-  }
-  exec_info.ClearWatched();
-}
-
 void Service::Unwatch(CmdArgList args, ConnectionContext* cntx) {
   UnwatchAllKeys(cntx);
   return (*cntx)->SendOk();
@@ -1050,8 +1064,7 @@ void Service::Discard(CmdArgList args, ConnectionContext* cntx) {
     return rb->SendError("DISCARD without MULTI");
   }
 
-  UnwatchAllKeys(cntx);
-  cntx->conn_state.exec_info.Clear();
+  MultiCleanup(cntx);
   rb->SendOk();
 }
 
@@ -1108,10 +1121,7 @@ void Service::Exec(CmdArgList args, ConnectionContext* cntx) {
   }
 
   auto& exec_info = cntx->conn_state.exec_info;
-  absl::Cleanup exec_clear = [&cntx, &exec_info] {
-    UnwatchAllKeys(cntx);
-    exec_info.Clear();
-  };
+  absl::Cleanup exec_clear = [&cntx] { MultiCleanup(cntx); };
 
   if (IsWatchingOtherDbs(cntx->db_index(), exec_info)) {
     return rb->SendError("Dragonfly does not allow WATCH and EXEC on different databases");

tests/dragonfly/server_family_test.py

@@ -20,3 +20,35 @@ def test_quit_after_sub(connection):
 
     with pytest.raises(redis.exceptions.ConnectionError) as e:
         connection.read_response()
+
+
+def test_multi_exec(client):
+    pipeline = client.pipeline()
+    pipeline.set("foo", "bar")
+    pipeline.get("foo")
+    val = pipeline.execute()
+    assert val == [True, "bar"]
+
+
+'''
+see https://github.com/dragonflydb/dragonfly/issues/457
+For now we would not allow for eval command inside multi
+As this would create to level transactions (in effect recursive call
+to Schedule function).
+When this issue is fully fixed, this test would failed, and then it should
+change to match the fact that we supporting this operation.
+For now we are expecting to get an error
+'''
+def test_multi_eval(client):
+    try:
+        pipeline = client.pipeline()
+        pipeline.set("foo", "bar")
+        pipeline.get("foo")
+        pipeline.eval("return 43", 0)
+        assert True, "This part should not executed due to issue #457"
+
+        val = pipeline.execute()
+        assert val == "foo"
+    except Exception as e:
+        msg = str(e)
+        assert "Dragonfly does not allow execution of" in msg

tests/integration/.patch_ioredis.sh

@@ -1,6 +0,0 @@
-#!/usr/bin/env bash
-
-# This script is only temporary until issue https://github.com/luin/ioredis/issues/1671 is resolved (hopefully soon)
-# what we are doing here, is making sure that we would not compare the result case sensitive.
-sed -i 's/expect(args\[0\]).to.eql("get")/expect(args\[0\]).to.match(\/get\/i)/' $1
-sed -i 's/args\[0\] === "set"/args\[0\] === "set" || args\[0\] === "SET"/' $1

tests/integration/.run_ioredis_valid_test.sh

@@ -2,15 +2,12 @@
 
 # The following tests are not supported
 #"should reconnect if reconnectOnError
-# should reload scripts on redis restart
-# should check and load uniq scripts only
 # supported in transaction blocks
-# rejects when monitor is disabled"
+# rejects when monitor is disabled
 # should resend unfulfilled commands to the correct
 # should set the name before any subscribe
 # should name the connection if options
 # scanStream
-# scripting
 # should affect the old way
 # should support Map
 # should support object
@@ -22,21 +19,14 @@
 # https://github.com/dragonflydb/dragonfly/issues/457
 # and https://github.com/dragonflydb/dragonfly/issues/458
 
-# The followng test will pass once https://github.com/luin/ioredis/issues/1671 is resolved:
-# should check and load uniq scripts only
-# should load scripts first before execution
-# should allow omitting callback
 
 # The follwing tests would pass once we support script flush command:
 # does not fallback to EVAL in manual transaction
 # does not fallback to EVAL in regular
-# should load scripts first before execution
 # should reload scripts on redis restart (reconnect)"
 
 
-
-
 TS_NODE_TRANSPILE_ONLY=true NODE_ENV=test mocha \
 "test/helpers/*.ts" "test/unit/**/*.ts" "test/functional/**/*.ts" \
--g "should reconnect if reconnectOnError|should reload scripts on redis restart|should check and load uniq scripts only|should be supported in transaction blocks|rejects when monitor is disabled|should resend unfulfilled commands to the correct|should set the name before any subscribe|should name the connection if options|scanStream|scripting|should affect the old way|should support Map|should support object|should batch all commands before ready event|should support key prefixing for sort|should be sent on the connect event" \
+-g "should reload scripts on redis restart|should reconnect if reconnectOnError|should be supported in transaction blocks|rejects when monitor is disabled|should resend unfulfilled commands to the correct|should set the name before any subscribe|should name the connection if options|scanStream|does not fallback to EVAL|should try to use EVALSHA and fallback to EVAL|should use evalsha when script|should affect the old way|should support Map|should support object|should batch all commands before ready event|should support key prefixing for sort|should be sent on the connect event" \
 --invert

tests/integration/ioredis.Dockerfile

@@ -9,16 +9,14 @@ WORKDIR /app
 # Git
 RUN apt update -y && apt install -y git
 
-# Clone ioredis v5.2.3
-RUN git clone --branch v5.2.3 https://github.com/luin/ioredis
+# The latest version from io-redis contain changes that we need to have
+# to successfully run the tests
+RUN git clone https://github.com/luin/ioredis
 
 WORKDIR /app/ioredis
 
 RUN npm install
 
-# This is required until https://github.com/luin/ioredis/issues/1671 is resolved.
-ADD .patch_ioredis.sh patch_ioredis.sh
-
 # Script to run the tests that curretly pass successfully.
 # Note that in DF we still don't have support for cluster and we
 # want to skip tests such as elasticache, also we have some issues that
@@ -27,8 +25,4 @@ ADD .patch_ioredis.sh patch_ioredis.sh
 # and https://github.com/dragonflydb/dragonfly/issues/458
 ADD .run_ioredis_valid_test.sh run_tests.sh
 
-# this would enable running monitor commands successfully
-# we may need to remove this incase we have other fix
-RUN ./patch_ioredis.sh test/functional/monitor.ts
-
 ENTRYPOINT [ "npm", "run", "env", "--", "TS_NODE_TRANSPILE_ONLY=true", "NODE_ENV=test" ]