gvsafronov/futriix
1
0
Fork 0
Code Issues Pull Requests Actions 8 Packages Projects Releases Wiki Activity

Ban snapshot-creating commands and other admin commands from transactions (#10015)

Browse Source 
Creating fork (or even a foreground SAVE) during a transaction breaks the atomicity of the transaction.
In addition to that, it could mess up the propagated transaction to the AOF file.

This change blocks SAVE, PSYNC, SYNC and SHUTDOWN from being executed inside MULTI-EXEC.
It does that by adding a command flag, so that modules can flag their commands with that flag too.

Besides it changes BGSAVE, BGREWRITEAOF, and CONFIG SET appendonly, to turn the
scheduled flag instead of forking righ taway.

Other changes:
* expose `protected`, `no-async-loading`, and `no_multi` flags in COMMAND command
* add a test to validate propagation of FLUSHALL inside a transaction.
* add a test to validate how CONFIG SET that errors reacts in a transaction

Co-authored-by: Oran Agra <oran@redislabs.com>
This commit is contained in:
guybe7 2022-01-04 12:37:47 +01:00 committed by GitHub
parent 2e1979a21e
commit ac84b1cd82
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
11 changed files with 114 additions and 11 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
src/aof.c
View File

@ -876,6 +876,9 @@ int startAppendOnly(void) {
if (hasActiveChildProcess() && server.child_type != CHILD_TYPE_AOF) {
server.aof_rewrite_scheduled = 1;
serverLog(LL_WARNING,"AOF was enabled but there is already another background operation. An AOF background was scheduled to start when possible.");
} else if (server.in_exec){
server.aof_rewrite_scheduled = 1;
serverLog(LL_WARNING,"AOF was enabled during a transaction. An AOF background was scheduled to start when possible.");
} else {
/* If there is a pending AOF rewrite, we need to switch it off and
* start a new one: the old one cannot be reused because it is not
@ -2305,7 +2308,7 @@ int rewriteAppendOnlyFileBackground(void) {
void bgrewriteaofCommand(client *c) {
if (server.child_type == CHILD_TYPE_AOF) {
addReplyError(c,"Background append only file rewriting already in progress");
} else if (hasActiveChildProcess()) {
} else if (hasActiveChildProcess() || server.in_exec) {
server.aof_rewrite_scheduled = 1;
addReplyStatus(c,"Background append only file rewriting scheduled");
} else if (rewriteAppendOnlyFileBackground() == C_OK) {

8
src/commands.c
View File

@ -6612,17 +6612,17 @@ struct redisCommand redisCommandTable[] = {
{"memory","A container for memory diagnostics commands","Depends on subcommand.","4.0.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_SERVER,MEMORY_History,MEMORY_Hints,NULL,-2,0,0,.subcommands=MEMORY_Subcommands},
{"module","A container for module commands","Depends on subcommand.","4.0.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_SERVER,MODULE_History,MODULE_Hints,NULL,-2,0,0,.subcommands=MODULE_Subcommands},
{"monitor","Listen for all requests received by the server in real time",NULL,"1.0.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_SERVER,MONITOR_History,MONITOR_Hints,monitorCommand,1,CMD_ADMIN|CMD_NOSCRIPT|CMD_LOADING|CMD_STALE,0},
{"psync","Internal command used for replication",NULL,"2.8.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_SERVER,PSYNC_History,PSYNC_Hints,syncCommand,-3,CMD_NO_ASYNC_LOADING|CMD_ADMIN|CMD_NOSCRIPT,0,.args=PSYNC_Args},
{"psync","Internal command used for replication",NULL,"2.8.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_SERVER,PSYNC_History,PSYNC_Hints,syncCommand,-3,CMD_NO_ASYNC_LOADING|CMD_ADMIN|CMD_NO_MULTI|CMD_NOSCRIPT,0,.args=PSYNC_Args},
{"replconf","An internal command for configuring the replication stream","O(1)","3.0.0",CMD_DOC_SYSCMD,NULL,NULL,COMMAND_GROUP_SERVER,REPLCONF_History,REPLCONF_Hints,replconfCommand,-1,CMD_ADMIN|CMD_NOSCRIPT|CMD_LOADING|CMD_STALE,0},
{"replicaof","Make the server a replica of another instance, or promote it as master.","O(1)","5.0.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_SERVER,REPLICAOF_History,REPLICAOF_Hints,replicaofCommand,3,CMD_NO_ASYNC_LOADING|CMD_ADMIN|CMD_NOSCRIPT|CMD_STALE,0,.args=REPLICAOF_Args},
{"restore-asking","An internal command for migrating keys in a cluster","O(1) to create the new key and additional O(N*M) to reconstruct the serialized value, where N is the number of Redis objects composing the value and M their average size. For small string values the time complexity is thus O(1)+O(1*M) where M is small, so simply O(1). However for sorted set values the complexity is O(N*M*log(N)) because inserting values into sorted sets is O(log(N)).","3.0.0",CMD_DOC_SYSCMD,NULL,NULL,COMMAND_GROUP_SERVER,RESTORE_ASKING_History,RESTORE_ASKING_Hints,restoreCommand,-4,CMD_WRITE|CMD_DENYOOM|CMD_ASKING,ACL_CATEGORY_KEYSPACE|ACL_CATEGORY_DANGEROUS,{{CMD_KEY_WRITE,KSPEC_BS_INDEX,.bs.index={1},KSPEC_FK_RANGE,.fk.range={0,1,0}}}},
{"role","Return the role of the instance in the context of replication","O(1)","2.8.12",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_SERVER,ROLE_History,ROLE_Hints,roleCommand,1,CMD_NOSCRIPT|CMD_LOADING|CMD_STALE|CMD_FAST|CMD_SENTINEL,ACL_CATEGORY_ADMIN|ACL_CATEGORY_DANGEROUS},
{"save","Synchronously save the dataset to disk","O(N) where N is the total number of keys in all databases","1.0.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_SERVER,SAVE_History,SAVE_Hints,saveCommand,1,CMD_NO_ASYNC_LOADING|CMD_ADMIN|CMD_NOSCRIPT,0},
{"shutdown","Synchronously save the dataset to disk and then shut down the server","O(N) when saving, where N is the total number of keys in all databases when saving data, otherwise O(1)","1.0.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_SERVER,SHUTDOWN_History,SHUTDOWN_Hints,shutdownCommand,-1,CMD_ADMIN|CMD_NOSCRIPT|CMD_LOADING|CMD_STALE|CMD_SENTINEL,0,.args=SHUTDOWN_Args},
{"save","Synchronously save the dataset to disk","O(N) where N is the total number of keys in all databases","1.0.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_SERVER,SAVE_History,SAVE_Hints,saveCommand,1,CMD_NO_ASYNC_LOADING|CMD_ADMIN|CMD_NOSCRIPT|CMD_NO_MULTI,0},
{"shutdown","Synchronously save the dataset to disk and then shut down the server","O(N) when saving, where N is the total number of keys in all databases when saving data, otherwise O(1)","1.0.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_SERVER,SHUTDOWN_History,SHUTDOWN_Hints,shutdownCommand,-1,CMD_ADMIN|CMD_NOSCRIPT|CMD_LOADING|CMD_STALE|CMD_NO_MULTI|CMD_SENTINEL,0,.args=SHUTDOWN_Args},
{"slaveof","Make the server a replica of another instance, or promote it as master. Deprecated starting with Redis 5. Use REPLICAOF instead.","O(1)","1.0.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_SERVER,SLAVEOF_History,SLAVEOF_Hints,replicaofCommand,3,CMD_NO_ASYNC_LOADING|CMD_ADMIN|CMD_NOSCRIPT|CMD_STALE,0,.args=SLAVEOF_Args},
{"slowlog","A container for slow log commands","Depends on subcommand.","2.2.12",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_SERVER,SLOWLOG_History,SLOWLOG_Hints,NULL,-2,0,0,.subcommands=SLOWLOG_Subcommands},
{"swapdb","Swaps two Redis databases","O(N) where N is the count of clients watching or blocking on keys from both databases.","4.0.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_SERVER,SWAPDB_History,SWAPDB_Hints,swapdbCommand,3,CMD_WRITE|CMD_FAST,ACL_CATEGORY_KEYSPACE|ACL_CATEGORY_DANGEROUS,.args=SWAPDB_Args},
{"sync","Internal command used for replication",NULL,"1.0.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_SERVER,SYNC_History,SYNC_Hints,syncCommand,1,CMD_NO_ASYNC_LOADING|CMD_ADMIN|CMD_NOSCRIPT,0},
{"sync","Internal command used for replication",NULL,"1.0.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_SERVER,SYNC_History,SYNC_Hints,syncCommand,1,CMD_NO_ASYNC_LOADING|CMD_ADMIN|CMD_NO_MULTI|CMD_NOSCRIPT,0},
{"time","Return the current server time","O(1)","2.6.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_SERVER,TIME_History,TIME_Hints,timeCommand,1,CMD_RANDOM|CMD_LOADING|CMD_STALE|CMD_FAST,0},
/* set */
{"sadd","Add one or more members to a set","O(1) for each element added, so O(N) to add N elements when the command is called with multiple arguments.","1.0.0",CMD_DOC_NONE,NULL,NULL,COMMAND_GROUP_SET,SADD_History,SADD_Hints,saddCommand,-3,CMD_WRITE|CMD_DENYOOM|CMD_FAST,ACL_CATEGORY_SET,{{CMD_KEY_WRITE,KSPEC_BS_INDEX,.bs.index={1},KSPEC_FK_RANGE,.fk.range={0,1,0}}},.args=SADD_Args},

1
src/commands/psync.json
View File

@ -8,6 +8,7 @@
"command_flags": [
"NO_ASYNC_LOADING",
"ADMIN",
"NO_MULTI",
"NOSCRIPT"
],
"arguments": [

3
src/commands/save.json
View File

@ -9,7 +9,8 @@
"command_flags": [
"NO_ASYNC_LOADING",
"ADMIN",
"NOSCRIPT"
"NOSCRIPT",
"NO_MULTI"
]
}
}

1
src/commands/shutdown.json
View File

@ -17,6 +17,7 @@
"NOSCRIPT",
"LOADING",
"STALE",
"NO_MULTI",
"SENTINEL"
],
"arguments": [

1
src/commands/sync.json
View File

@ -8,6 +8,7 @@
"command_flags": [
"NO_ASYNC_LOADING",
"ADMIN",
"NO_MULTI",
"NOSCRIPT"
]
}

4
src/rdb.c
View File

@ -3432,8 +3432,8 @@ void bgsaveCommand(client *c) {
if (server.child_type == CHILD_TYPE_RDB) {
addReplyError(c,"Background save already in progress");
} else if (hasActiveChildProcess()) {
if (schedule) {
} else if (hasActiveChildProcess() || server.in_exec) {
if (schedule || server.in_exec) {
server.rdb_bgsave_scheduled = 1;
addReplyStatus(c,"Background saving scheduled");
} else {

9
src/server.c
View File

@ -3350,6 +3350,11 @@ int processCommand(client *c) {
}
}
if (c->flags & CLIENT_MULTI && c->cmd->flags & CMD_NO_MULTI) {
rejectCommandFormat(c,"Command not allowed inside a transaction");
return C_OK;
}
/* Check if the user can run this command according to the current
* ACLs. */
int acl_errpos;
@ -3997,6 +4002,10 @@ void addReplyFlagsForCommand(client *c, struct redisCommand *cmd) {
flagcount += addReplyCommandFlag(c,cmd->flags,CMD_NO_AUTH, "no_auth");
flagcount += addReplyCommandFlag(c,cmd->flags,CMD_MAY_REPLICATE, "may_replicate");
flagcount += addReplyCommandFlag(c,cmd->flags,CMD_NO_MANDATORY_KEYS, "no_mandatory_keys");
flagcount += addReplyCommandFlag(c,cmd->flags,CMD_PROTECTED, "protected");
flagcount += addReplyCommandFlag(c,cmd->flags,CMD_NO_ASYNC_LOADING, "no_async_loading");
flagcount += addReplyCommandFlag(c,cmd->flags,CMD_NO_MULTI, "no_multi");
/* "sentinel" and "only-sentinel" are hidden on purpose. */
if (cmd->movablekeys) {
addReplyStatus(c, "movablekeys");

5
src/server.h
View File

@ -206,6 +206,7 @@ extern int configOOMScoreAdjValuesDefaults[CONFIG_OOM_COUNT];
#define CMD_MODULE_GETKEYS (1ULL<<21) /* Use the modules getkeys interface. */
#define CMD_MODULE_NO_CLUSTER (1ULL<<22) /* Deny on Redis Cluster. */
#define CMD_NO_ASYNC_LOADING (1ULL<<23)
#define CMD_NO_MULTI (1ULL<<24)
/* Command flags that describe ACLs categories. */
#define ACL_CATEGORY_KEYSPACE (1ULL<<0)
@ -2081,7 +2082,7 @@ typedef int redisGetKeysProc(struct redisCommand *cmd, robj **argv, int argc, ge
* CMD_LOADING: Allow the command while loading the database.
*
* CMD_NO_ASYNC_LOADING: Deny during async loading (when a replica uses diskless
sync swapdb, and allows access to the old dataset)
* sync swapdb, and allows access to the old dataset)
*
* CMD_STALE: Allow the command while a slave has stale data but is not
* allowed to serve this data. Normally no command is accepted
@ -2115,6 +2116,8 @@ typedef int redisGetKeysProc(struct redisCommand *cmd, robj **argv, int argc, ge
*
* CMD_NO_MANDATORY_KEYS: This key arguments for this command are optional.
*
* CMD_NO_MULTI: The command is nt allowed inside a transaction
*
* The following additional flags are only used in order to put commands
* in a specific ACL category. Commands can have multiple ACL categories.
* See redis.conf for the exact meaning of each.

8
tests/unit/aofrw.tcl
View File

@ -61,10 +61,14 @@ start_server {tags {"aofrw external:skip"} overrides {aof-use-rdb-preamble no}}
test {Turning off AOF kills the background writing child if any} {
r config set appendonly yes
waitForBgrewriteaof r
r multi
# start a slow AOFRW
set k v
r config set rdb-key-save-delay 10000000
r bgrewriteaof
# disable AOF and wait for the child to be killed
r config set appendonly no
r exec
wait_for_condition 50 100 {
[string match {*Killing*AOF*child*} [exec tail -5 < [srv 0 stdout]]]
} else {

80
tests/unit/multi.tcl
View File

@ -743,4 +743,84 @@ start_server {tags {"multi"}} {
} {} {needs:repl}
}
foreach {cmd} {SAVE SHUTDOWN} {
test "MULTI with $cmd" {
r del foo
r multi
r set foo bar
catch {r $cmd} e1
catch {r exec} e2
assert_match {*Command not allowed inside a transaction*} $e1
assert_match {EXECABORT*} $e2
r get foo
} {}
}
test "MULTI with BGREWRITEAOF" {
set forks [s total_forks]
r multi
r set foo bar
r BGREWRITEAOF
set res [r exec]
assert_match "*rewriting scheduled*" [lindex $res 1]
wait_for_condition 50 100 {
[s total_forks] > $forks
} else {
fail "aofrw didn't start"
}
waitForBgrewriteaof r
} {} {external:skip}
test "MULTI with config set appendonly" {
set lines [count_log_lines 0]
set forks [s total_forks]
r multi
r set foo bar
r config set appendonly yes
r exec
verify_log_message 0 "*AOF background was scheduled*" $lines
wait_for_condition 50 100 {
[s total_forks] > $forks
} else {
fail "aofrw didn't start"
}
waitForBgrewriteaof r
} {} {external:skip}
test "MULTI with config error" {
r multi
r set foo bar
r config set maxmemory bla
# letting the redis parser read it, it'll throw an exception instead of
# reply with an array that contains an error, so we switch to reading
# raw RESP instead
r readraw 1
set res [r exec]
assert_equal $res "*2"
set res [r read]
assert_equal $res "+OK"
set res [r read]
r readraw 1
set _ $res
} {*CONFIG SET failed*}
}
start_server {overrides {appendonly {yes} appendfilename {appendonly.aof} appendfsync always} tags {external:skip}} {
test {MULTI with FLUSHALL and AOF} {
set aof [get_last_incr_aof_path r]
r multi
r set foo bar
r flushall
r exec
assert_aof_content $aof {
{select *}
{multi}
{set *}
{flushall}
{exec}
}
r get foo
} {}
}