Allow users to replace allocator and handle OOM everywhere. #800

michael-grunder · 2020-05-07T22:46:09Z

This PR replaces every allocation and deallocation with hi_ variants that are defined in alloc.h

Additionally, it attempts to gracefully handle OOM everywhere.

To test the new logic, I created "fail allocators" which could be configured to fail after N allocations or randomly.

Then I ran our tests under Valgrind while incrementing this counter until I detected a Valgrind error (leak, invalid read/write, etc) or all the tests passed. We don't have async unit tests so I created some and repeated the process.

A couple points of interest:

When fuzz testing hiredis I found a bug where redisReadTask->elements was defined as an int but redisReply->elements was defined as a size_t, which could overflow (3e8835e).
I fixed a Unix socket reconnection leak
I made a small change to ssl.c to avoid a potential memory leak (8fe3dd7)

Cheers!

* Replace every allocation/free call with defined hiredis variants. This should make it trivial for users of the library to replace the allocator with one of their choosing. * Add logic to handle OOM everywhere in the library to allow for graceful exit in the event of OOM.

michael-grunder · 2020-05-08T01:55:05Z

One more thing before I forget.

I think we may want to impose a sane limit on multi-bulk elements. I needed to do that locally or fuzz testing would immediately break hiredis by sending something like:

*35184372088832\r\n

To redisReader. This isn't invalid and fits in a 64 bit integer, but it's an absurd count that ends up blocking hiredis for minutes. It seems unlikely we'd ever need to actually process that many elements.

To fix it locally for fuzz testing I just did this:

diff --git hiredis.c hiredis.c
index 6ce3049..37795d1 100644
--- hiredis.c
+++ hiredis.c
@@ -44,6 +44,9 @@
 #include "async.h"
 #include "win32.h"

+/* A sane value for maximum number of array elements */
+#define HIREDIS_MAX_ARRAY_ELEMENTS ((1LL<<32) - 1)
+
 static redisContextFuncs redisContextDefaultFuncs = {
     .free_privdata = NULL,
     .async_read = redisAsyncRead,
@@ -150,6 +153,9 @@ static void *createStringObject(const redisReadTask *task, char *str, size_t len
 static void *createArrayObject(const redisReadTask *task, size_t elements) {
     redisReply *r, *parent;

+    if (elements > HIREDIS_MAX_ARRAY_ELEMENTS)
+        return NULL;
+
     r = createReplyObject(task->type);
     if (r == NULL)
         return NULL;
@@ -171,6 +177,7 @@ static void *createArrayObject(const redisReadTask *task, size_t elements) {
                parent->type == REDIS_REPLY_SET);
         parent->element[task->idx] = r;
     }
+
     return r;
 }

yossigo · 2020-05-11T18:51:24Z

alloc.h

+#define hi_calloc calloc
+#define hi_realloc realloc
+#define hi_strdup strdup
+#define hi_free free


@michael-grunder What about pluggable heap functions? Do you prefer to have that in a separate PR?

Sure, that works for me. It would make it much simpler for end-users to plug in their own functions too.

Updated the logic to use a single pass-through struct with function pointers and just access the appropriate member depending on what kind of allocation it is.

I went with static inline functions here because it allowed me to have the allocation functions configurable without paying a performance price. Using non-inline functions defined in alloc.c cost between 8-15% on my machine for some workloads.

yossigo · 2020-05-11T19:07:47Z

net.c

@@ -328,6 +328,22 @@ int redisContextSetTimeout(redisContext *c, const struct timeval tv) {
    return REDIS_OK;
 }

+static int _redisContextUpdateTimeout(redisContext *c, const struct timeval *timeout) {
+    /* Same timeval struct, short circuit */
+    if (c->timeout == timeout)


@michael-grunder You probably meant to compare the actual value and not the pointer here.

yossigo · 2020-05-11T19:09:07Z

net.c

@@ -352,20 +368,18 @@ static int _redisContextConnectTcp(redisContext *c, const char *addr, int port,
     * This is a bit ugly, but atleast it works and doesn't leak memory.
     **/
    if (c->tcp.host != addr) {


Looks like another case of comparing the pointer rather than the value.

yossigo · 2020-05-11T19:11:49Z

net.c

@@ -375,10 +389,10 @@ static int _redisContextConnectTcp(redisContext *c, const char *addr, int port,
    }

    if (source_addr == NULL) {
-        free(c->tcp.source_addr);
+        hi_free(c->tcp.source_addr);
        c->tcp.source_addr = NULL;
    } else if (c->tcp.source_addr != source_addr) {


Same as above.

net.c

yossigo · 2020-05-11T19:24:49Z

@michael-grunder Another thing, regarding HIREDIS_MAX_ARRAY_ELEMENTS do you see any reason not to make this runtime configurable rather than a compile time define?

I think it's the kind of things that will need to be changed once in a million, but if that happens it's much easier to just do some runtime configuration rather than full library rebuild.

michael-grunder · 2020-05-11T20:57:28Z

Thanks for going through the code!

@michael-grunder Another thing, regarding HIREDIS_MAX_ARRAY_ELEMENTS do you see any reason not to make this runtime configurable rather than a compile time define?

I'm happy to add this as configurable similar to our reader maxbuf.

The pointer comparisons were intentional but this is what we're doing now. I think it was initially done this way to handle reconnections.

AFAIK it's safe to treat two pointers equal if they point to the same address and are of the same type. Do you mind clarifying what your concern is? It's possible I'm totally missing your point 😄

For reference, this logic has been in place for years (example, example).

I'll add HIREDIS_MAX_ARRAY_ELEMENTS as a configurable runtime setting, with a sane default.
I'll switch to pluggable heap allocation functions that will default to malloc, calloc, free, etc.

yossigo · 2020-05-12T07:58:21Z

@michael-grunder Yes I see the reconnect now! So this obviously works for reconnects, but if a user calls this function with the same value (but not same pointer of course) it will get reallocated. Anyway, I guess that's not related to this PR.

Rework allocator injection logic use non-inline pass-thru functions on Windows, so the feature is at least available.

Just some small changes that makes the new logic conform better to existing hiredis naming and style conventions.

michael-grunder · 2020-05-20T16:55:50Z

Hi @yossigo, would you like to see any changes in this PR?

yossigo

@michael-grunder Looks good to me! Just one small comment to consider above.

alloc.c

michael-grunder added 7 commits May 3, 2020 18:05

Merge branch 'master' into pluggable-allocation

ee652b6

Fix a potential memory leak in ssl.c

8fe3dd7

Update missed allocations to our wrapper define

a32192b

Fix redisReaderTask elements type

3e8835e

More robuse async OOM handling

8d59446

Fix two compilation errors

58e72a4

yossigo reviewed May 11, 2020

View reviewed changes

Fix potential memory leak

cbb7fe6

michael-grunder added 9 commits May 12, 2020 08:06

Allow for a configurable max multi-bulk elements setting

3e10335

Allow library users to specify allocator

4fe636a

Add extern C if we detect C++

7257a49

Fix windows compilation

19d4577

Disable pluggable allocation for Windows

8a7d6a8

Attempt cleaner Win32 logic

85f8746

Further simplify Win32 logic

fdb182e

Re-enable allocator injection in Windows

9efea93

Rework allocator injection logic use non-inline pass-thru functions on Windows, so the feature is at least available.

Minor refactor and name changes

9487d18

Just some small changes that makes the new logic conform better to existing hiredis naming and style conventions.

michael-grunder mentioned this pull request May 15, 2020

Hiredis v1.0.0 #807

Closed

7 tasks

michael-grunder added 2 commits May 18, 2020 20:14

Fix typo

2843337

Merge remote-tracking branch 'upstream/master' into pluggable-allocation

4e8fd61

yossigo reviewed May 22, 2020

View reviewed changes

alloc.c Outdated Show resolved Hide resolved

michael-grunder added 3 commits May 22, 2020 07:55

Remove piecemeal allocator override

324cadf

Fix memory leak introduced merging upstream/master

e5a1879

Merge branch 'master' into pluggable-allocation

06318f2

michael-grunder merged commit 8e0264c into redis:master May 22, 2020

This was referenced May 22, 2020

Enhanced error-handling (OOM) and more (redisAsync* family) #638

Closed

New SSL API to replace redisSecureConnection(). #821

Merged

michael-grunder deleted the pluggable-allocation branch October 15, 2022 05:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow users to replace allocator and handle OOM everywhere. #800

Allow users to replace allocator and handle OOM everywhere. #800

michael-grunder commented May 7, 2020 •

edited

Loading

michael-grunder commented May 8, 2020

yossigo May 11, 2020

michael-grunder May 11, 2020

michael-grunder May 13, 2020

yossigo May 11, 2020

yossigo May 11, 2020

yossigo May 11, 2020

yossigo commented May 11, 2020

michael-grunder commented May 11, 2020

yossigo commented May 12, 2020

michael-grunder commented May 20, 2020

yossigo left a comment

Allow users to replace allocator and handle OOM everywhere. #800

Allow users to replace allocator and handle OOM everywhere. #800

Conversation

michael-grunder commented May 7, 2020 • edited Loading

michael-grunder commented May 8, 2020

yossigo May 11, 2020

Choose a reason for hiding this comment

michael-grunder May 11, 2020

Choose a reason for hiding this comment

michael-grunder May 13, 2020

Choose a reason for hiding this comment

yossigo May 11, 2020

Choose a reason for hiding this comment

yossigo May 11, 2020

Choose a reason for hiding this comment

yossigo May 11, 2020

Choose a reason for hiding this comment

yossigo commented May 11, 2020

michael-grunder commented May 11, 2020

yossigo commented May 12, 2020

michael-grunder commented May 20, 2020

yossigo left a comment

Choose a reason for hiding this comment

michael-grunder commented May 7, 2020 •

edited

Loading