Unverified Commit 14176484 authored by Oran Agra's avatar Oran Agra Committed by GitHub
Browse files

Prevent LCS from allocating temp memory over proto-max-bulk-len (#9817)

LCS can allocate immense amount of memory (sizes of two inputs multiplied by each other).
In the past this caused some possible security issues due to overflows, which we solved
and also added use of `trymalloc` to return "Insufficient memory" instead of OOM panic zmalloc.

But in case overcommit is enabled, it could be that we won't get the OOM panic, and zmalloc
will succeed, and then we can get OOM killed by the kernel.

The solution here is to prevent LCS from allocating transient memory that's bigger than
`proto-max-bulk-len` config.
This config is not directly related to transient memory, but using a hard coded value ad well as
introducing a specific config seems wrong.

This comes to solve an error in the corrupt-dump-fuzzer test that started in the daily CI see #9799
parent d4e7ffb3
...@@ -800,10 +800,15 @@ void lcsCommand(client *c) { ...@@ -800,10 +800,15 @@ void lcsCommand(client *c) {
unsigned long long lcssize = (unsigned long long)(alen+1)*(blen+1); /* Can't overflow due to the size limits above. */ unsigned long long lcssize = (unsigned long long)(alen+1)*(blen+1); /* Can't overflow due to the size limits above. */
unsigned long long lcsalloc = lcssize * sizeof(uint32_t); unsigned long long lcsalloc = lcssize * sizeof(uint32_t);
uint32_t *lcs = NULL; uint32_t *lcs = NULL;
if (lcsalloc < SIZE_MAX && lcsalloc / lcssize == sizeof(uint32_t)) if (lcsalloc < SIZE_MAX && lcsalloc / lcssize == sizeof(uint32_t)) {
if (lcsalloc > (size_t)server.proto_max_bulk_len) {
addReplyError(c, "Insufficient memory, transient memory for LCS exceeds proto-max-bulk-len");
goto cleanup;
}
lcs = ztrymalloc(lcsalloc); lcs = ztrymalloc(lcsalloc);
}
if (!lcs) { if (!lcs) {
addReplyError(c, "Insufficient memory"); addReplyError(c, "Insufficient memory, failed allocating transient memory for LCS");
goto cleanup; goto cleanup;
} }
......
...@@ -732,5 +732,14 @@ test {corrupt payload: fuzzer findings - stream double free listpack when insert ...@@ -732,5 +732,14 @@ test {corrupt payload: fuzzer findings - stream double free listpack when insert
} }
} }
test {corrupt payload: fuzzer findings - LCS OOM} {
start_server [list overrides [list loglevel verbose use-exit-on-panic yes crash-memcheck-enabled no] ] {
r SETRANGE _int 423324 1450173551
catch {r LCS _int _int} err
assert_match "*Insufficient memory*" $err
r ping
}
}
} ;# tags } ;# tags
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment