slipenkomk/valkey
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

fix scripts access wrong slot if they disagree with pre-declared keys (#12906)

Browse Source 
Regarding how to obtain the hash slot of a key, there is an optimization
in `getKeySlot()`, it is used to avoid redundant hash calculations for
keys: when the current client is in the process of executing a command,
it can directly use the slot of the current client because the slot to
access has already been calculated in advance in `processCommand()`.

However, scripts are a special case where, in default mode or with
`allow-cross-slot-keys` enabled, they are allowed to access keys beyond
the pre-declared range. This means that the keys they operate on may not
belong to the slot of the pre-declared keys. Currently, when the
commands in a script are executed, the slot of the original client
(i.e., the current client) is not correctly updated, leading to
subsequent access to the wrong slot.

This PR fixes the above issue. When checking the cluster constraints in
a script, the slot to be accessed by the current command is set for the
original client (i.e., the current client). This ensures that
`getKeySlot()` gets the correct slot cache.

Additionally, the following modifications are made:

1. The 'sort' and 'sort_ro' commands use `getKeySlot()` instead of
`c->slot` because the client could be an engine client in a script and
can lead to potential bug.
2. `getKeySlot()` is also used in pubsub to obtain the slot for the
channel, standardizing the way slots are retrieved.
This commit is contained in:
zhaozhao.zz 2024-01-15 09:57:12 +08:00 committed by GitHub
parent 284ef21ea0
commit bb2b6e2927
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
5 changed files with 24 additions and 7 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
src/pubsub.c
View File

@ -290,7 +290,7 @@ int pubsubSubscribeChannel(client *c, robj *channel, pubsubtype type) {
incrRefCount(channel);
/* Add the client to the channel -> list of clients hash table */
if (server.cluster_enabled && type.shard) {
slot = c->slot;
slot = getKeySlot(channel->ptr);
}
d_ptr = type.serverPubSubChannels(slot);
if (*d_ptr == NULL) {
@ -332,7 +332,7 @@ int pubsubUnsubscribeChannel(client *c, robj *channel, int notify, pubsubtype ty
retval = 1;
/* Remove the client from the channel -> clients list hash table */
if (server.cluster_enabled && type.shard) {
slot = c->slot != -1 ? c->slot : (int)keyHashSlot(channel->ptr, sdslen(channel->ptr));
slot = getKeySlot(channel->ptr);
}
d = *type.serverPubSubChannels(slot);
serverAssertWithInfo(c,NULL,d != NULL);

13
src/script.c
View File

@ -209,6 +209,7 @@ int scriptPrepareForRun(scriptRunCtx *run_ctx, client *engine_client, client *ca
run_ctx->c = engine_client;
run_ctx->original_client = caller;
run_ctx->funcname = funcname;
run_ctx->slot = caller->slot;
client *script_client = run_ctx->c;
client *curr_client = run_ctx->original_client;
@ -262,6 +263,8 @@ void scriptResetRun(scriptRunCtx *run_ctx) {
unprotectClient(run_ctx->original_client);
}
run_ctx->slot = -1;
preventCommandPropagation(run_ctx->original_client);
/* unset curr_run_ctx so we will know there is no running script */
@ -463,14 +466,18 @@ static int scriptVerifyClusterState(scriptRunCtx *run_ctx, client *c, client *or
* already been thrown. This is only checking for cross slot keys being accessed
* that weren't pre-declared. */
if (hashslot != -1 && !(run_ctx->flags & SCRIPT_ALLOW_CROSS_SLOT)) {
if (original_c->slot == -1) {
original_c->slot = hashslot;
} else if (original_c->slot != hashslot) {
if (run_ctx->slot == -1) {
run_ctx->slot = hashslot;
} else if (run_ctx->slot != hashslot) {
*err = sdsnew("Script attempted to access keys that do not hash to "
"the same slot");
return C_ERR;
}
}
c->slot = hashslot;
original_c->slot = hashslot;
return C_OK;
}

1
src/script.h
View File

@ -74,6 +74,7 @@ struct scriptRunCtx {
int flags;
int repl_flags;
monotime start_time;
int slot;
};
/* Scripts flags */

4
src/sort.c
View File

@ -239,7 +239,7 @@ void sortCommandGeneric(client *c, int readonly) {
/* If BY is specified with a real pattern, we can't accept it in cluster mode,
* unless we can make sure the keys formed by the pattern are in the same slot
* as the key to sort. */
if (server.cluster_enabled && patternHashSlot(sortby->ptr, sdslen(sortby->ptr)) != c->slot) {
if (server.cluster_enabled && patternHashSlot(sortby->ptr, sdslen(sortby->ptr)) != getKeySlot(c->argv[1]->ptr)) {
addReplyError(c, "BY option of SORT denied in Cluster mode when "
"keys formed by the pattern may be in different slots.");
syntax_error++;
@ -258,7 +258,7 @@ void sortCommandGeneric(client *c, int readonly) {
/* If GET is specified with a real pattern, we can't accept it in cluster mode,
* unless we can make sure the keys formed by the pattern are in the same slot
* as the key to sort. */
if (server.cluster_enabled && patternHashSlot(c->argv[j+1]->ptr, sdslen(c->argv[j+1]->ptr)) != c->slot) {
if (server.cluster_enabled && patternHashSlot(c->argv[j+1]->ptr, sdslen(c->argv[j+1]->ptr)) != getKeySlot(c->argv[1]->ptr)) {
addReplyError(c, "GET option of SORT denied in Cluster mode when "
"keys formed by the pattern may be in different slots.");
syntax_error++;

9
tests/unit/cluster/scripting.tcl
View File

@ -62,6 +62,15 @@ start_cluster 1 0 {tags {external:skip cluster}} {
} 1 bar}
}
test {Cross slot commands are allowed by default if they disagree with pre-declared keys} {
r 0 flushall
r 0 eval "redis.call('set', 'foo', 'bar')" 1 bar
# Make sure the script writes to the right slot
assert_equal 1 [r 0 cluster COUNTKEYSINSLOT 12182] ;# foo slot
assert_equal 0 [r 0 cluster COUNTKEYSINSLOT 5061] ;# bar slot
}
test "Function no-cluster flag" {
R 0 function load {#!lua name=test
redis.register_function{function_name='f1', callback=function() return 'hello' end, flags={'no-cluster'}}