diff options
author | sundb <sundbcn@gmail.com> | 2023-04-11 01:38:40 +0800 |
---|---|---|
committer | GitHub <noreply@github.com> | 2023-04-10 20:38:40 +0300 |
commit | e0b378d22b8eb53a5ce87f7b8d36ee63aafa4131 (patch) | |
tree | 0e00a11926b6dca95c179627b90ff36b58757c6f /src/networking.c | |
parent | e55568edb58c1437d6a84e1baa103dcf5b6153ef (diff) | |
download | redis-e0b378d22b8eb53a5ce87f7b8d36ee63aafa4131.tar.gz |
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>
Diffstat (limited to 'src/networking.c')
-rw-r--r-- | src/networking.c | 13 |
1 files changed, 7 insertions, 6 deletions
diff --git a/src/networking.c b/src/networking.c index 4dcdf6bc9..7fa18af48 100644 --- a/src/networking.c +++ b/src/networking.c @@ -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; |