From 85a834bfa2a31cd7c5754fe1611cf059657c0fb4 Mon Sep 17 00:00:00 2001 From: "zhaozhao.zz" Date: Thu, 25 Jan 2024 17:17:39 +0800 Subject: [PATCH] Revert multi OOM limit and add multi buffer limit (#12961) Fix #9926 , and introduce an alternative method to prevent abuse of transactions: 1. revert #5454 (which was blocking read-only transactions in OOM state), and break the tie of MULTI state memory usage and the server OOM state. Meaning that we'll limit the total memory a single client can queue, and do that unconditionally regardless of the server being OOM or not. 2. to prevent abuse of transactions, we use the `client-query-buffer-limit` to restrict the size of the transaction. Because the commands cached in the MULTI/EXEC queue have not been executed yet, so they are also considered a part of the "query buffer" in a broader sense. In other words, the commands in the MULTI queue and the `querybuf` of the client together constitute the "query buffer". When they exceed the limit, the connection will be disconnected. The reasoning is that it's sensible to sends a single command with a huge (1GB) argument, and it's sensible to sends a transaction with many small commands, but it's probably not common to sends a long transaction with many huge arguments (will consume a lot of memory before even being executed). If anyone runs into that, they can simply increase the `client-query-buffer-limit` config. P.S. To prevent DDoS attacks, unauthenticated clients have a separate hard limit. Their query buffer should not exceed a maximum of 1MB. In other words, if the query buffer of an unauthenticated client exceeds 1MB or the `client-query-buffer-limit` (if it is set to a value smaller than 1MB,), the connection will be disconnected. --- redis.conf | 2 +- src/networking.c | 8 +++++++- src/server.c | 16 +--------------- 3 files changed, 9 insertions(+), 17 deletions(-) diff --git a/redis.conf b/redis.conf index ab9ba337d..333430ae7 100644 --- a/redis.conf +++ b/redis.conf @@ -2075,7 +2075,7 @@ client-output-buffer-limit pubsub 32mb 8mb 60 # amount by default in order to avoid that a protocol desynchronization (for # instance due to a bug in the client) will lead to unbound memory usage in # the query buffer. However you can configure it here if you have very special -# needs, such us huge multi/exec requests or alike. +# needs, such as a command with huge argument, or huge multi/exec requests or alike. # # client-query-buffer-limit 1gb diff --git a/src/networking.c b/src/networking.c index 0390092e4..f8e3c14ae 100644 --- a/src/networking.c +++ b/src/networking.c @@ -2720,7 +2720,13 @@ void readQueryFromClient(connection *conn) { atomicIncr(server.stat_net_input_bytes, nread); } - if (!(c->flags & CLIENT_MASTER) && sdslen(c->querybuf) > server.client_max_querybuf_len) { + if (!(c->flags & CLIENT_MASTER) && + /* The commands cached in the MULTI/EXEC queue have not been executed yet, + * so they are also considered a part of the query buffer in a broader sense. + * + * For unauthenticated clients, the query buffer cannot exceed 1MB at most. */ + (c->mstate.argv_len_sums + sdslen(c->querybuf) > server.client_max_querybuf_len || + (c->mstate.argv_len_sums + sdslen(c->querybuf) > 1024*1024*1024 && authRequired(c)))) { sds ci = catClientInfoString(sdsempty(),c), bytes = sdsempty(); bytes = sdscatrepr(bytes,c->querybuf,64); diff --git a/src/server.c b/src/server.c index bfabfa272..db4841940 100644 --- a/src/server.c +++ b/src/server.c @@ -4131,21 +4131,7 @@ int processCommand(client *c) { * in a slave, that may be the active client, to be freed. */ if (server.current_client == NULL) return C_ERR; - int reject_cmd_on_oom = is_denyoom_command; - /* If client is in MULTI/EXEC context, queuing may consume an unlimited - * amount of memory, so we want to stop that. - * However, we never want to reject DISCARD, or even EXEC (unless it - * contains denied commands, in which case is_denyoom_command is already - * set. */ - if (c->flags & CLIENT_MULTI && - c->cmd->proc != execCommand && - c->cmd->proc != discardCommand && - c->cmd->proc != quitCommand && - c->cmd->proc != resetCommand) { - reject_cmd_on_oom = 1; - } - - if (out_of_memory && reject_cmd_on_oom) { + if (out_of_memory && is_denyoom_command) { rejectCommand(c, shared.oomerr); return C_OK; }