From e0b378d22b8eb53a5ce87f7b8d36ee63aafa4131 Mon Sep 17 00:00:00 2001 From: sundb Date: Tue, 11 Apr 2023 01:38:40 +0800 Subject: Use dummy allocator to make accesses defined as per standard (#11982) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Issue When we use GCC-12 later or clang 9.0 later to build with `-D_FORTIFY_SOURCE=3`, we can see the following buffer overflow: ``` === REDIS BUG REPORT START: Cut & paste starting from here === 6263:M 06 Apr 2023 08:59:12.915 # Redis 255.255.255 crashed by signal: 6, si_code: -6 6263:M 06 Apr 2023 08:59:12.915 # Crashed running the instruction at: 0x7f03d59efa7c ------ STACK TRACE ------ EIP: /lib/x86_64-linux-gnu/libc.so.6(pthread_kill+0x12c)[0x7f03d59efa7c] Backtrace: /lib/x86_64-linux-gnu/libc.so.6(+0x42520)[0x7f03d599b520] /lib/x86_64-linux-gnu/libc.so.6(pthread_kill+0x12c)[0x7f03d59efa7c] /lib/x86_64-linux-gnu/libc.so.6(raise+0x16)[0x7f03d599b476] /lib/x86_64-linux-gnu/libc.so.6(abort+0xd3)[0x7f03d59817f3] /lib/x86_64-linux-gnu/libc.so.6(+0x896f6)[0x7f03d59e26f6] /lib/x86_64-linux-gnu/libc.so.6(__fortify_fail+0x2a)[0x7f03d5a8f76a] /lib/x86_64-linux-gnu/libc.so.6(+0x1350c6)[0x7f03d5a8e0c6] src/redis-server 127.0.0.1:25111(+0xd5e80)[0x557cddd3be80] src/redis-server 127.0.0.1:25111(feedReplicationBufferWithObject+0x78)[0x557cddd3c768] src/redis-server 127.0.0.1:25111(replicationFeedSlaves+0x1a4)[0x557cddd3cbc4] src/redis-server 127.0.0.1:25111(+0x8721a)[0x557cddced21a] src/redis-server 127.0.0.1:25111(call+0x47a)[0x557cddcf38ea] src/redis-server 127.0.0.1:25111(processCommand+0xbf4)[0x557cddcf4aa4] src/redis-server 127.0.0.1:25111(processInputBuffer+0xe6)[0x557cddd22216] src/redis-server 127.0.0.1:25111(readQueryFromClient+0x3a8)[0x557cddd22898] src/redis-server 127.0.0.1:25111(+0x1b9134)[0x557cdde1f134] src/redis-server 127.0.0.1:25111(aeMain+0x119)[0x557cddce5349] src/redis-server 127.0.0.1:25111(main+0x466)[0x557cddcd6716] /lib/x86_64-linux-gnu/libc.so.6(+0x29d90)[0x7f03d5982d90] /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0x80)[0x7f03d5982e40] src/redis-server 127.0.0.1:25111(_start+0x25)[0x557cddcd7025] ``` The main reason is that when FORTIFY_SOURCE is enabled, GCC or clang will enhance some common functions, such as `strcpy`, `memcpy`, `fgets`, etc, so that they can detect buffer overflow errors and stop program execution, thus improving the safety of the program. We use `zmalloc_usable_size()` everywhere to use memory blocks, but that is an abuse since the malloc_usable_size() isn't meant for this kind of use, it is for diagnostics only. That is also why the behavior is flaky when built with _FORTIFY_SOURCE, the compiler can sense that we reach outside the allocated block and SIGABRT. ### Solution If we need to use the additional memory we got, we need to use a dummy realloc with `alloc_size` attribute and no inlining, (see `extend_to_usable`) to let the compiler see the large of memory we need to use. This can either be an implicit call inside `z*usable` that returns the size, so that the caller doesn't have any other worry, or it can be a normal zmalloc call which means that if the caller wants to use zmalloc_usable_size it must also use extend_to_usable. ### Changes This PR does the following: 1) rename the current z[try]malloc_usable family to z[try]malloc_internal and don't expose them to users outside zmalloc.c, 2) expose a new set of `z[*]_usable` family that use z[*]_internal and `extend_to_usable()` implicitly, the caller gets the size of the allocation and it is safe to use. 3) go over all the users of `zmalloc_usable_size` and convert them to use the `z[*]_usable` family if possible. 4) in the places where the caller can't use `z[*]_usable` and store the real size, and must still rely on zmalloc_usable_size, we still make sure that the allocation used `z[*]_usable` (which has a call to `extend_to_usable()`) and ignores the returning size, this way a later call to `zmalloc_usable_size` is still safe. [4] was done for module.c and listpack.c, all the others places (sds, reply proto list, replication backlog, client->buf) are using [3]. Co-authored-by: Oran Agra --- src/module.c | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) (limited to 'src/module.c') diff --git a/src/module.c b/src/module.c index ae75b3dc2..e29277956 100644 --- a/src/module.c +++ b/src/module.c @@ -516,13 +516,20 @@ void moduleCreateContext(RedisModuleCtx *out_ctx, RedisModule *module, int ctx_f * You should avoid using malloc(). * This function panics if unable to allocate enough memory. */ void *RM_Alloc(size_t bytes) { - return zmalloc(bytes); + /* Use 'zmalloc_usable()' instead of 'zmalloc()' to allow the compiler + * to recognize the additional memory size, which means that modules can + * use the memory reported by 'RM_MallocUsableSize()' safely. In theory this + * isn't really needed since this API can't be inlined (not even for embedded + * modules like TLS (we use function pointers for module APIs), and the API doesn't + * have the malloc_size attribute, but it's hard to predict how smart future compilers + * will be, so better safe than sorry. */ + return zmalloc_usable(bytes,NULL); } /* Similar to RM_Alloc, but returns NULL in case of allocation failure, instead * of panicking. */ void *RM_TryAlloc(size_t bytes) { - return ztrymalloc(bytes); + return ztrymalloc_usable(bytes,NULL); } /* Use like calloc(). Memory allocated with this function is reported in @@ -530,12 +537,12 @@ void *RM_TryAlloc(size_t bytes) { * and in general is taken into account as memory allocated by Redis. * You should avoid using calloc() directly. */ void *RM_Calloc(size_t nmemb, size_t size) { - return zcalloc(nmemb*size); + return zcalloc_usable(nmemb*size,NULL); } /* Use like realloc() for memory obtained with RedisModule_Alloc(). */ void* RM_Realloc(void *ptr, size_t bytes) { - return zrealloc(ptr,bytes); + return zrealloc_usable(ptr,bytes,NULL); } /* Use like free() for memory obtained by RedisModule_Alloc() and @@ -10704,6 +10711,9 @@ size_t RM_MallocSize(void* ptr) { /* Similar to RM_MallocSize, the difference is that RM_MallocUsableSize * returns the usable size of memory by the module. */ size_t RM_MallocUsableSize(void *ptr) { + /* It is safe to use 'zmalloc_usable_size()' to manipulate additional + * memory space, as we guarantee that the compiler can recognize this + * after 'RM_Alloc', 'RM_TryAlloc', 'RM_Realloc', or 'RM_Calloc'. */ return zmalloc_usable_size(ptr); } -- cgit v1.2.1