Use dummy allocator to make accesses defined as per standard (#11982)

## 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 <oran@redislabs.com>
This commit is contained in:
sundb 2023-04-11 01:38:40 +08:00 committed by GitHub
parent e55568edb5
commit e0b378d22b
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
6 changed files with 118 additions and 37 deletions

View File

@ -520,7 +520,7 @@ NULL
restartServer(flags,delay);
addReplyError(c,"failed to restart the server. Check server logs.");
} else if (!strcasecmp(c->argv[1]->ptr,"oom")) {
void *ptr = zmalloc(ULONG_MAX); /* Should trigger an out of memory. */
void *ptr = zmalloc(SIZE_MAX/2); /* Should trigger an out of memory. */
zfree(ptr);
addReply(c,shared.ok);
} else if (!strcasecmp(c->argv[1]->ptr,"assert")) {

View File

@ -39,8 +39,11 @@
#ifndef LISTPACK_ALLOC_H
#define LISTPACK_ALLOC_H
#include "zmalloc.h"
#define lp_malloc zmalloc
#define lp_realloc zrealloc
/* We use zmalloc_usable/zrealloc_usable instead of zmalloc/zrealloc
* to ensure the safe invocation of 'zmalloc_usable_size().
* See comment in zmalloc_usable_size(). */
#define lp_malloc(sz) zmalloc_usable(sz,NULL)
#define lp_realloc(ptr,sz) zrealloc_usable(ptr,sz,NULL)
#define lp_free zfree
#define lp_malloc_size zmalloc_usable_size
#endif

View File

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

View File

@ -134,7 +134,7 @@ client *createClient(connection *conn) {
connSetReadHandler(conn, readQueryFromClient);
connSetPrivateData(conn, c);
}
c->buf = zmalloc(PROTO_REPLY_CHUNK_BYTES);
c->buf = zmalloc_usable(PROTO_REPLY_CHUNK_BYTES, &c->buf_usable_size);
selectDb(c,0);
uint64_t client_id;
atomicGetIncr(server.next_client_id, client_id, 1);
@ -150,7 +150,6 @@ client *createClient(connection *conn) {
c->lib_name = NULL;
c->lib_ver = NULL;
c->bufpos = 0;
c->buf_usable_size = zmalloc_usable_size(c->buf);
c->buf_peak = c->buf_usable_size;
c->buf_peak_last_reset_time = server.unixtime;
c->ref_repl_buf_node = NULL;
@ -701,11 +700,12 @@ void trimReplyUnusedTailSpace(client *c) {
if (tail->size - tail->used > tail->size / 4 &&
tail->used < PROTO_REPLY_CHUNK_BYTES)
{
size_t usable_size;
size_t old_size = tail->size;
tail = zrealloc(tail, tail->used + sizeof(clientReplyBlock));
tail = zrealloc_usable(tail, tail->used + sizeof(clientReplyBlock), &usable_size);
/* take over the allocation's internal fragmentation (at least for
* memory usage tracking) */
tail->size = zmalloc_usable_size(tail) - sizeof(clientReplyBlock);
tail->size = usable_size - sizeof(clientReplyBlock);
c->reply_bytes = c->reply_bytes + tail->size - old_size;
listNodeValue(ln) = tail;
}
@ -785,9 +785,10 @@ void setDeferredReply(client *c, void *node, const char *s, size_t length) {
listDelNode(c->reply,ln);
} else {
/* Create a new node */
clientReplyBlock *buf = zmalloc(length + sizeof(clientReplyBlock));
size_t usable_size;
clientReplyBlock *buf = zmalloc_usable(length + sizeof(clientReplyBlock), &usable_size);
/* Take over the allocation's internal fragmentation */
buf->size = zmalloc_usable_size(buf) - sizeof(clientReplyBlock);
buf->size = usable_size - sizeof(clientReplyBlock);
buf->used = length;
memcpy(buf->buf, s, length);
listNodeValue(ln) = buf;

View File

@ -102,9 +102,16 @@ static void zmalloc_default_oom(size_t size) {
static void (*zmalloc_oom_handler)(size_t) = zmalloc_default_oom;
#ifdef HAVE_MALLOC_SIZE
void *extend_to_usable(void *ptr, size_t size) {
UNUSED(size);
return ptr;
}
#endif
/* 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) {
static inline void *ztrymalloc_usable_internal(size_t size, size_t *usable) {
/* 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);
@ -123,24 +130,39 @@ void *ztrymalloc_usable(size_t size, size_t *usable) {
#endif
}
void *ztrymalloc_usable(size_t size, size_t *usable) {
size_t usable_size = 0;
void *ptr = ztrymalloc_usable_internal(size, &usable_size);
#ifdef HAVE_MALLOC_SIZE
ptr = extend_to_usable(ptr, usable_size);
#endif
if (usable) *usable = usable_size;
return ptr;
}
/* Allocate memory or panic */
void *zmalloc(size_t size) {
void *ptr = ztrymalloc_usable(size, NULL);
void *ptr = ztrymalloc_usable_internal(size, NULL);
if (!ptr) zmalloc_oom_handler(size);
return ptr;
}
/* Try allocating memory, and return NULL if failed. */
void *ztrymalloc(size_t size) {
void *ptr = ztrymalloc_usable(size, NULL);
void *ptr = ztrymalloc_usable_internal(size, NULL);
return ptr;
}
/* Allocate memory or panic.
* '*usable' is set to the usable size if non NULL. */
void *zmalloc_usable(size_t size, size_t *usable) {
void *ptr = ztrymalloc_usable(size, usable);
size_t usable_size = 0;
void *ptr = ztrymalloc_usable_internal(size, &usable_size);
if (!ptr) zmalloc_oom_handler(size);
#ifdef HAVE_MALLOC_SIZE
ptr = extend_to_usable(ptr, usable_size);
#endif
if (usable) *usable = usable_size;
return ptr;
}
@ -165,7 +187,7 @@ 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) {
static inline void *ztrycalloc_usable_internal(size_t size, size_t *usable) {
/* 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);
@ -184,6 +206,16 @@ void *ztrycalloc_usable(size_t size, size_t *usable) {
#endif
}
void *ztrycalloc_usable(size_t size, size_t *usable) {
size_t usable_size = 0;
void *ptr = ztrycalloc_usable_internal(size, &usable_size);
#ifdef HAVE_MALLOC_SIZE
ptr = extend_to_usable(ptr, usable_size);
#endif
if (usable) *usable = usable_size;
return ptr;
}
/* Allocate memory and zero it or panic.
* We need this wrapper to have a calloc compatible signature */
void *zcalloc_num(size_t num, size_t size) {
@ -193,35 +225,40 @@ void *zcalloc_num(size_t num, size_t size) {
zmalloc_oom_handler(SIZE_MAX);
return NULL;
}
void *ptr = ztrycalloc_usable(num*size, NULL);
void *ptr = ztrycalloc_usable_internal(num*size, NULL);
if (!ptr) zmalloc_oom_handler(num*size);
return ptr;
}
/* Allocate memory and zero it or panic */
void *zcalloc(size_t size) {
void *ptr = ztrycalloc_usable(size, NULL);
void *ptr = ztrycalloc_usable_internal(size, NULL);
if (!ptr) zmalloc_oom_handler(size);
return ptr;
}
/* Try allocating memory, and return NULL if failed. */
void *ztrycalloc(size_t size) {
void *ptr = ztrycalloc_usable(size, NULL);
void *ptr = ztrycalloc_usable_internal(size, NULL);
return ptr;
}
/* Allocate memory or panic.
* '*usable' is set to the usable size if non NULL. */
void *zcalloc_usable(size_t size, size_t *usable) {
void *ptr = ztrycalloc_usable(size, usable);
size_t usable_size = 0;
void *ptr = ztrycalloc_usable_internal(size, &usable_size);
if (!ptr) zmalloc_oom_handler(size);
#ifdef HAVE_MALLOC_SIZE
ptr = extend_to_usable(ptr, usable_size);
#endif
if (usable) *usable = usable_size;
return ptr;
}
/* 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) {
static inline void *ztryrealloc_usable_internal(void *ptr, size_t size, size_t *usable) {
#ifndef HAVE_MALLOC_SIZE
void *realptr;
#endif
@ -275,24 +312,39 @@ void *ztryrealloc_usable(void *ptr, size_t size, size_t *usable) {
#endif
}
void *ztryrealloc_usable(void *ptr, size_t size, size_t *usable) {
size_t usable_size = 0;
ptr = ztryrealloc_usable_internal(ptr, size, &usable_size);
#ifdef HAVE_MALLOC_SIZE
ptr = extend_to_usable(ptr, usable_size);
#endif
if (usable) *usable = usable_size;
return ptr;
}
/* Reallocate memory and zero it or panic */
void *zrealloc(void *ptr, size_t size) {
ptr = ztryrealloc_usable(ptr, size, NULL);
ptr = ztryrealloc_usable_internal(ptr, size, NULL);
if (!ptr && size != 0) zmalloc_oom_handler(size);
return ptr;
}
/* Try Reallocating memory, and return NULL if failed. */
void *ztryrealloc(void *ptr, size_t size) {
ptr = ztryrealloc_usable(ptr, size, NULL);
ptr = ztryrealloc_usable_internal(ptr, size, NULL);
return ptr;
}
/* Reallocate memory or panic.
* '*usable' is set to the usable size if non NULL. */
void *zrealloc_usable(void *ptr, size_t size, size_t *usable) {
ptr = ztryrealloc_usable(ptr, size, usable);
size_t usable_size = 0;
ptr = ztryrealloc_usable(ptr, size, &usable_size);
if (!ptr && size != 0) zmalloc_oom_handler(size);
#ifdef HAVE_MALLOC_SIZE
ptr = extend_to_usable(ptr, usable_size);
#endif
if (usable) *usable = usable_size;
return ptr;
}

View File

@ -96,20 +96,20 @@
#define HAVE_DEFRAG
#endif
__attribute__((malloc)) void *zmalloc(size_t size);
__attribute__((malloc)) void *zcalloc(size_t size);
__attribute__((malloc)) __attribute((alloc_size(1,2))) void *zcalloc_num(size_t num, size_t size);
__attribute__((malloc,alloc_size(1))) void *zmalloc(size_t size);
__attribute__((malloc,alloc_size(1))) void *zcalloc(size_t size);
__attribute__((malloc,alloc_size(1,2))) void *zcalloc_num(size_t num, size_t size);
__attribute__((alloc_size(2))) void *zrealloc(void *ptr, size_t size);
__attribute__((malloc)) void *ztrymalloc(size_t size);
__attribute__((malloc)) void *ztrycalloc(size_t size);
__attribute__((malloc,alloc_size(1))) void *ztrymalloc(size_t size);
__attribute__((malloc,alloc_size(1))) void *ztrycalloc(size_t size);
__attribute__((alloc_size(2))) void *ztryrealloc(void *ptr, size_t size);
void zfree(void *ptr);
__attribute__((malloc)) __attribute__((alloc_size(1))) void *zmalloc_usable(size_t size, size_t *usable);
__attribute__((malloc)) __attribute__((alloc_size(1))) void *zcalloc_usable(size_t size, size_t *usable);
__attribute__((alloc_size(2))) void *zrealloc_usable(void *ptr, size_t size, size_t *usable);
__attribute__((malloc)) __attribute__((alloc_size(1))) void *ztrymalloc_usable(size_t size, size_t *usable);
__attribute__((malloc)) __attribute__((alloc_size(1))) void *ztrycalloc_usable(size_t size, size_t *usable);
__attribute__((alloc_size(2))) void *ztryrealloc_usable(void *ptr, size_t size, size_t *usable);
void *zmalloc_usable(size_t size, size_t *usable);
void *zcalloc_usable(size_t size, size_t *usable);
void *zrealloc_usable(void *ptr, size_t size, size_t *usable);
void *ztrymalloc_usable(size_t size, size_t *usable);
void *ztrycalloc_usable(size_t size, size_t *usable);
void *ztryrealloc_usable(void *ptr, size_t size, size_t *usable);
void zfree_usable(void *ptr, size_t *usable);
__attribute__((malloc)) char *zstrdup(const char *s);
size_t zmalloc_used_memory(void);
@ -133,7 +133,22 @@ __attribute__((malloc)) void *zmalloc_no_tcache(size_t size);
size_t zmalloc_size(void *ptr);
size_t zmalloc_usable_size(void *ptr);
#else
/* If we use 'zmalloc_usable_size()' to obtain additional available memory size
* and manipulate it, we need to call 'extend_to_usable()' afterwards to ensure
* the compiler recognizes this extra memory. However, if we use the pointer
* obtained from z[*]_usable() family functions, there is no need for this step. */
#define zmalloc_usable_size(p) zmalloc_size(p)
/* derived from https://github.com/systemd/systemd/pull/25688
* 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.
* We use a dummy allocator function to tell the compiler that the new size of ptr is newsize.
* The implementation returns the pointer as is; the only reason for its existence is as a conduit for the
* alloc_size attribute. This cannot be a static inline because gcc then loses the attributes on the function.
* See: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96503 */
__attribute__((alloc_size(2),noinline)) void *extend_to_usable(void *ptr, size_t size);
#endif
int get_proc_stat_ll(int i, long long *res);