From 214c10f165c3ae3f84077d2605ad51ed965c3634 Mon Sep 17 00:00:00 2001 From: Boaz Sade Date: Wed, 9 Nov 2022 19:18:28 +0200 Subject: [PATCH] fix(server): reject eval inside transaction multi blocks #457 (#471) fix(server): block running lua script inside pipeline #457 Signed-off-by: Boaz Sade --- src/server/dragonfly_test.cc | 21 ++++++++- src/server/main_service.cc | 48 ++++++++++++-------- tests/dragonfly/server_family_test.py | 32 +++++++++++++ tests/integration/.patch_ioredis.sh | 6 --- tests/integration/.run_ioredis_valid_test.sh | 14 +----- tests/integration/ioredis.Dockerfile | 12 ++--- 6 files changed, 85 insertions(+), 48 deletions(-) delete mode 100755 tests/integration/.patch_ioredis.sh diff --git a/src/server/dragonfly_test.cc b/src/server/dragonfly_test.cc index 4c370f1c0fb2..f01e1cd2ec89 100644 --- a/src/server/dragonfly_test.cc +++ b/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) { diff --git a/src/server/main_service.cc b/src/server/main_service.cc index 107a3502dd6c..229bb44b6d9d 100644 --- a/src/server/main_service.cc +++ b/src/server/main_service.cc @@ -73,6 +73,24 @@ std::optional 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"); diff --git a/tests/dragonfly/server_family_test.py b/tests/dragonfly/server_family_test.py index b0b93c73ad34..020dcb60b399 100644 --- a/tests/dragonfly/server_family_test.py +++ b/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 diff --git a/tests/integration/.patch_ioredis.sh b/tests/integration/.patch_ioredis.sh deleted file mode 100755 index 246048620bc1..000000000000 --- a/tests/integration/.patch_ioredis.sh +++ /dev/null @@ -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 diff --git a/tests/integration/.run_ioredis_valid_test.sh b/tests/integration/.run_ioredis_valid_test.sh index 994efa3a01af..6c4bc77a172e 100755 --- a/tests/integration/.run_ioredis_valid_test.sh +++ b/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 diff --git a/tests/integration/ioredis.Dockerfile b/tests/integration/ioredis.Dockerfile index f6c6a56aa271..7786b4ed600d 100644 --- a/tests/integration/ioredis.Dockerfile +++ b/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" ]