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.
This commit is contained in:
zhaozhao.zz 2024-01-25 17:17:39 +08:00 committed by GitHub
parent 07b292af5e
commit 85a834bfa2
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
3 changed files with 9 additions and 17 deletions

View File

@ -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

View File

@ -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);

View File

@ -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;
}