mirror of
https://github.com/valkey-io/valkey.git
synced 2026-05-06 05:26:42 -04:00
fix: validate key count before allocating result in keyspec (#3598)
In `getKeysUsingKeySpecs`, when extracting keys based on the `KSPEC_FK_KEYNUM `spec (like in the `EVAL` command), the server read the number of keys from the arguments and calculated the expected end index. However, it called `getKeysPrepareResult` to allocate memory for the result array before validating whether last was within the bounds of the actual arguments provided. If a client sent a command with a huge declared number of keys (e.g., `COMMAND GETKEYS EVAL "return 1" 2147483647 key1`), the server would allocate a massive amount of memory. Since `vm.overcommit_memory` is recommended, this allocation would NOT normally have triggered OOM (we never wrote to it so there is no physical memory allocated), but if you disable overcommit, this could trigger an OOM. You can reproduce it with: ``` $ prlimit --as=1073741824 src/valkey-server --save "" ... 384270:M 30 Apr 2026 04:27:24.456 * Ready to accept connections tcp ... <in valkey-cli> 127.0.0.1:6379> command getkeys eval "return 1" 2147483647 key1 ... <in server log> 384270:M 30 Apr 2026 04:29:26.950 # Out Of Memory allocating 17179869176 bytes! ``` ## Solution * Moved the bounds check `if (last >= argc || last < first || first >= argc)` to execute before the call to `getKeysPrepareResult`, preventing the large allocation on invalid input. * To further catch issues like this, protected against integer overflow during the calculation of last by using a long long temporary variable. If it exceeds INT_MAX or falls below INT_MIN, the spec is marked invalid immediately. Signed-off-by: Jacob Murphy <jkmurphy@google.com>
This commit is contained in:
@@ -2374,20 +2374,22 @@ int getKeysUsingKeySpecs(struct serverCommand *cmd, robj **argv, int argc, int s
|
||||
}
|
||||
|
||||
first += spec->fk.keynum.firstkey;
|
||||
last = first + ((int)numkeys - 1) * step;
|
||||
long long temp_last = first + (numkeys - 1) * step;
|
||||
if (temp_last > INT_MAX || temp_last < INT_MIN) goto invalid_spec;
|
||||
last = (int)temp_last;
|
||||
} else {
|
||||
/* unknown spec */
|
||||
goto invalid_spec;
|
||||
}
|
||||
|
||||
int count = ((last - first) + 1);
|
||||
keys = getKeysPrepareResult(result, result->numkeys + count);
|
||||
|
||||
/* First or last is out of bounds, which indicates a syntax error */
|
||||
if (last >= argc || last < first || first >= argc) {
|
||||
goto invalid_spec;
|
||||
}
|
||||
|
||||
int count = ((last - first) + 1);
|
||||
keys = getKeysPrepareResult(result, result->numkeys + count);
|
||||
|
||||
for (i = first; i <= last; i += step) {
|
||||
if (i >= argc || i < first) {
|
||||
/* Modules commands, and standard commands with a not fixed number
|
||||
|
||||
@@ -162,6 +162,10 @@ start_server {tags {"introspection"}} {
|
||||
assert_equal {} [r command getkeys eval "return 1" 0]
|
||||
}
|
||||
|
||||
test {COMMAND GETKEYS EVAL with huge numkeys} {
|
||||
assert_equal {} [r command getkeys eval "return 1" 2147483647 key1]
|
||||
}
|
||||
|
||||
test {COMMAND GETKEYS LCS} {
|
||||
assert_equal {key1 key2} [r command getkeys lcs key1 key2]
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user