diff options
author | Oran Agra <oran@redislabs.com> | 2022-07-13 09:14:38 +0300 |
---|---|---|
committer | GitHub <noreply@github.com> | 2022-07-13 09:14:38 +0300 |
commit | 599e59ebc57283f52c60a8de56ec5f44d053109a (patch) | |
tree | 6745143949ea51b00b884c5cf2282f0363882a12 | |
parent | 20af95a99f333d242e13ffae86e89d6c1167d36f (diff) | |
download | redis-599e59ebc57283f52c60a8de56ec5f44d053109a.tar.gz |
Avoid valgrind fishy value warning on corrupt restore payloads (#10937)
The corrupt dump fuzzer uncovered a valgrind warning saying:
```
==76370== Argument 'size' of function malloc has a fishy (possibly negative) value: -3744781444216323815
```
This allocation would have failed (returning NULL) and being handled properly by redis (even before this change), but we also want to silence the valgrind warnings (which are checking that casting to ssize_t produces a non-negative value).
The solution i opted for is to explicitly fail these allocations (returning NULL), before even reaching `malloc` (which would have failed and return NULL too).
The implication is that we will not be able to support a single allocation of more than 2GB on a 32bit system (which i don't think is a realistic scenario).
i.e. i do think we could be facing cases were redis consumes more than 2gb on a 32bit system, but not in a single allocation.
The byproduct of this, is that i dropped the overflow assertions, since these will now lead to the same OOM panic we have for failed allocations.
-rw-r--r-- | src/zmalloc.c | 18 | ||||
-rw-r--r-- | tests/integration/corrupt-dump.tcl | 11 |
2 files changed, 23 insertions, 6 deletions
diff --git a/src/zmalloc.c b/src/zmalloc.c index 857baa8ac..dc9d7cbc4 100644 --- a/src/zmalloc.c +++ b/src/zmalloc.c @@ -59,14 +59,12 @@ void zlibc_free(void *ptr) { #ifdef HAVE_MALLOC_SIZE #define PREFIX_SIZE (0) -#define ASSERT_NO_SIZE_OVERFLOW(sz) #else #if defined(__sun) || defined(__sparc) || defined(__sparc__) #define PREFIX_SIZE (sizeof(long long)) #else #define PREFIX_SIZE (sizeof(size_t)) #endif -#define ASSERT_NO_SIZE_OVERFLOW(sz) assert((sz) + PREFIX_SIZE > (sz)) #endif /* When using the libc allocator, use a minimum allocation size to match the @@ -106,7 +104,8 @@ static void (*zmalloc_oom_handler)(size_t) = zmalloc_default_oom; /* Try allocating memory, and return NULL if failed. * '*usable' is set to the usable size if non NULL. */ void *ztrymalloc_usable(size_t size, size_t *usable) { - ASSERT_NO_SIZE_OVERFLOW(size); + /* Possible overflow, return NULL, so that the caller can panic or handle a failed allocation. */ + if (size >= SIZE_MAX/2) return NULL; void *ptr = malloc(MALLOC_MIN_SIZE(size)+PREFIX_SIZE); if (!ptr) return NULL; @@ -149,7 +148,7 @@ void *zmalloc_usable(size_t size, size_t *usable) { * Currently implemented only for jemalloc. Used for online defragmentation. */ #ifdef HAVE_DEFRAG void *zmalloc_no_tcache(size_t size) { - ASSERT_NO_SIZE_OVERFLOW(size); + if (size >= SIZE_MAX/2) zmalloc_oom_handler(size); void *ptr = mallocx(size+PREFIX_SIZE, MALLOCX_TCACHE_NONE); if (!ptr) zmalloc_oom_handler(size); update_zmalloc_stat_alloc(zmalloc_size(ptr)); @@ -166,7 +165,8 @@ void zfree_no_tcache(void *ptr) { /* Try allocating memory and zero it, and return NULL if failed. * '*usable' is set to the usable size if non NULL. */ void *ztrycalloc_usable(size_t size, size_t *usable) { - ASSERT_NO_SIZE_OVERFLOW(size); + /* Possible overflow, return NULL, so that the caller can panic or handle a failed allocation. */ + if (size >= SIZE_MAX/2) return NULL; void *ptr = calloc(1, MALLOC_MIN_SIZE(size)+PREFIX_SIZE); if (ptr == NULL) return NULL; @@ -221,7 +221,6 @@ void *zcalloc_usable(size_t size, size_t *usable) { /* Try reallocating memory, and return NULL if failed. * '*usable' is set to the usable size if non NULL. */ void *ztryrealloc_usable(void *ptr, size_t size, size_t *usable) { - ASSERT_NO_SIZE_OVERFLOW(size); #ifndef HAVE_MALLOC_SIZE void *realptr; #endif @@ -238,6 +237,13 @@ void *ztryrealloc_usable(void *ptr, size_t size, size_t *usable) { if (ptr == NULL) return ztrymalloc_usable(size, usable); + /* Possible overflow, return NULL, so that the caller can panic or handle a failed allocation. */ + if (size >= SIZE_MAX/2) { + zfree(ptr); + if (usable) *usable = 0; + return NULL; + } + #ifdef HAVE_MALLOC_SIZE oldsize = zmalloc_size(ptr); newptr = realloc(ptr,size); diff --git a/tests/integration/corrupt-dump.tcl b/tests/integration/corrupt-dump.tcl index 1e54f26a1..1b9be676b 100644 --- a/tests/integration/corrupt-dump.tcl +++ b/tests/integration/corrupt-dump.tcl @@ -790,5 +790,16 @@ test {corrupt payload: fuzzer findings - streamLastValidID panic} { } } +test {corrupt payload: fuzzer findings - valgrind fishy value warning} { + start_server [list overrides [list loglevel verbose use-exit-on-panic yes crash-memcheck-enabled no] ] { + r config set sanitize-dump-payload yes + r debug set-skip-checksum-validation 1 + catch {r restore _key 0 "\x13\x01\x10\x00\x00\x01\x81\xCC\x07\xDC\xF2\x00\x00\x00\x00\x00\x00\x00\x00\x40\x42\x42\x00\x00\x00\x18\x00\x02\x01\x01\x01\x02\x01\x84\x69\x74\x65\x6D\x05\x85\x76\x61\x6C\x75\x65\x06\x00\x01\x02\x01\x00\x01\x00\x01\x01\x01\x00\x01\x05\x01\x03\x01\x2C\x01\x00\x01\x01\x01\x82\x5F\x31\x03\x05\x01\x02\x01\x3C\x01\x00\x01\x01\x01\x02\x01\x05\x01\xFF\x02\xD0\x00\x00\x01\x81\xCC\x07\xDD\x2E\x00\x81\x00\x00\x01\x81\xCC\x07\xDC\xF2\x00\x81\x00\x00\x01\x81\xCC\x07\xDD\x1E\x00\x03\x01\x07\x6D\x79\x67\x72\x6F\x75\x70\x81\x00\x00\x01\x81\xCC\x07\xDD\x1E\x00\x02\x01\x00\x00\x01\x81\xCC\x07\xDD\x1E\x00\x00\x00\x00\x00\x00\x00\x00\x71\xDD\x07\xCC\x81\x01\x00\x00\x01\x01\x05\x41\x6C\x69\x63\x65\x58\xDD\x07\xCC\x81\x01\x00\x00\x01\x00\x00\x01\x81\xCC\x07\xDD\x1E\x00\x00\x00\x00\x00\x00\x00\x00\x0A\x00\x2F\xB0\xD1\x15\x0A\x97\x87\x6B"} err + assert_match "*Bad data format*" $err + r ping + } +} + + } ;# tags |