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

Do command existence and arity checks when loading AOF to avoid crash (#1614)

Browse Source 
Do command existence and arity checks when loading AOF to avoid crash

Currently, loading commands such as `cluster` or `cluster slots xxx`
from AOF will cause the server to crash.
1. `cluster` is a container command, and executing proc will cause a
    crash because we do not check subcommand and arity.
2. `cluster slots xxx`, arity check fail, reply with an error from the
    AOF client and trigger a panic.

Of course, there are many other ways for a problematic AOF to cause the
panic, but it is still necessary do some basic checks before executing.
In this way, in these basic cases, we can print useful error messages
instead of crashing directly.

Signed-off-by: Binbin <binloveplay1314@qq.com>
This commit is contained in:
Binbin 2025-01-30 01:06:13 +08:00 committed by GitHub
parent d72a97edf6
commit 4b8f3ed9ac
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 48 additions and 8 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

10
src/aof.c
View File

@ -1532,10 +1532,11 @@ int loadSingleAppendOnlyFile(char *filename) {
}
/* Command lookup */
cmd = lookupCommand(argv, argc);
if (!cmd) {
serverLog(LL_WARNING, "Unknown command '%s' reading the append only file %s", (char *)argv[0]->ptr,
filename);
sds err = NULL;
fakeClient->cmd = fakeClient->lastcmd = cmd = lookupCommand(argv, argc);
if ((!cmd && !commandCheckExistence(fakeClient, &err)) || (cmd && !commandCheckArity(cmd, argc, &err))) {
serverLog(LL_WARNING, "Error reading the append only file %s, error: %s", filename, err);
sdsfree(err);
freeClientArgv(fakeClient);
ret = AOF_FAILED;
goto cleanup;
@ -1544,7 +1545,6 @@ int loadSingleAppendOnlyFile(char *filename) {
if (cmd->proc == multiCommand) valid_before_multi = valid_up_to;
/* Run the command in the context of a fake client */
fakeClient->cmd = fakeClient->lastcmd = cmd;
if (fakeClient->flag.multi && fakeClient->cmd->proc != execCommand) {
/* Note: we don't have to attempt calling evalGetCommandFlags,
* since this is AOF, the checks in processCommand are not made

3
src/server.c
View File

@ -3911,7 +3911,7 @@ void afterCommand(client *c) {
int commandCheckExistence(client *c, sds *err) {
if (c->cmd) return 1;
if (!err) return 0;
if (isContainerCommandBySds(c->argv[0]->ptr)) {
if (isContainerCommandBySds(c->argv[0]->ptr) && c->argc >= 2) {
/* If we can't find the command but argv[0] by itself is a command
* it means we're dealing with an invalid subcommand. Print Help. */
sds cmd = sdsnew((char *)c->argv[0]->ptr);
@ -4025,7 +4025,6 @@ int processCommand(client *c) {
return C_OK;
}
/* Check if the command is marked as protected and the relevant configuration allows it */
if (c->cmd->flags & CMD_PROTECTED) {
if ((c->cmd->proc == debugCommand && !allowProtectedAction(server.enable_debug_cmd, c)) ||

43
tests/integration/aof.tcl
View File

@ -259,7 +259,7 @@ tags {"aof external:skip"} {
start_server_aof_ex [list dir $server_path aof-load-truncated yes] [list wait_ready false] {
test "Unknown command: Server should have logged an error" {
wait_for_log_messages 0 {"*Unknown command 'bla' reading the append only file*"} 0 10 1000
wait_for_log_messages 0 {"*unknown command 'bla'*"} 0 10 1000
}
}
@ -693,6 +693,47 @@ tags {"aof cluster external:skip"} {
assert_equal [r ping] {PONG}
}
}
test {Test command check in aof won't crash} {
# cluster, wrong number of arguments for 'cluster' command
create_aof $aof_dirpath $aof_file { append_to_aof [formatCommand cluster] }
create_aof_manifest $aof_dirpath $aof_manifest_file { append_to_manifest "file appendonly.aof.1.incr.aof seq 1 type i\n" }
start_server_aof_ex [list dir $server_path] [list wait_ready false] {
wait_for_condition 100 50 {
! [is_alive [srv pid]]
} else {
fail "AOF loading didn't fail"
}
assert_equal 1 [count_message_lines $server_path/stdout "wrong number of arguments for 'cluster' command"]
}
clean_aof_persistence $aof_dirpath
# cluster slots-xxx, unknown subcommand 'slots-xxx'
create_aof $aof_dirpath $aof_file { append_to_aof [formatCommand cluster slots-xxx] }
create_aof_manifest $aof_dirpath $aof_manifest_file { append_to_manifest "file appendonly.aof.1.incr.aof seq 1 type i\n" }
start_server_aof_ex [list dir $server_path] [list wait_ready false] {
wait_for_condition 100 50 {
! [is_alive [srv pid]]
} else {
fail "AOF loading didn't fail"
}
assert_equal 1 [count_message_lines $server_path/stdout "unknown subcommand 'slots-xxx'"]
}
clean_aof_persistence $aof_dirpath
# cluster slots xxx, wrong number of arguments for 'cluster|slots' command
create_aof $aof_dirpath $aof_file { append_to_aof [formatCommand cluster slots xxx] }
create_aof_manifest $aof_dirpath $aof_manifest_file { append_to_manifest "file appendonly.aof.1.incr.aof seq 1 type i\n" }
start_server_aof_ex [list dir $server_path] [list wait_ready false] {
wait_for_condition 100 50 {
![is_alive [srv pid]]
} else {
fail "AOF loading didn't fail"
}
assert_equal 1 [count_message_lines $server_path/stdout "wrong number of arguments for 'cluster|slots' command"]
}
clean_aof_persistence $aof_dirpath
}
}
set ::singledb $old_singledb