From e67a1235c90fdbe6cdc476d6c9d1d5e6a8ed5938 Mon Sep 17 00:00:00 2001 From: Bjorn Svensson Date: Thu, 3 Dec 2020 17:18:51 +0100 Subject: [PATCH 01/14] Use hi_free() from hiredis Hiredis uses its own allocator-functions which by default uses libc. By using the same design in hiredis-cluster a user can replace the allocator for both hiredis and hiredis-cluster at the same time. Own allocator functions can be enabled by the API: `hiredisAllocFuncs hiredisSetAllocators(hiredisAllocFuncs *ha)` This commit only covers `free()` and cleanups of freeing data. --- adlist.c | 3 +- command.c | 8 ++++-- dict.c | 17 ++++++----- hiarray.c | 6 ++-- hircluster.c | 80 ++++++++++++++++++++-------------------------------- hiutil.c | 19 ++++--------- hiutil.h | 7 ----- 7 files changed, 56 insertions(+), 84 deletions(-) diff --git a/adlist.c b/adlist.c index 607d7fa..e0e451c 100644 --- a/adlist.c +++ b/adlist.c @@ -27,10 +27,11 @@ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE * POSSIBILITY OF SUCH DAMAGE. */ +#include +#include #include "adlist.h" #include "hiutil.h" -#include /* Create a new list. The created list can be freed with * AlFreeList(), but private value of every node need to be freed diff --git a/command.c b/command.c index 74f5540..0a43e97 100644 --- a/command.c +++ b/command.c @@ -1,5 +1,6 @@ #include #include +#include #include "command.h" #include "hiarray.h" @@ -1655,15 +1656,18 @@ void command_destroy(struct cmd *command) { if (command->cmd != NULL) { hi_free(command->cmd); + command->cmd = NULL; } if (command->errstr != NULL) { hi_free(command->errstr); + command->errstr = NULL; } if (command->keys != NULL) { command->keys->nelem = 0; hiarray_destroy(command->keys); + command->keys = NULL; } if (command->frag_seq != NULL) { @@ -1671,9 +1675,7 @@ void command_destroy(struct cmd *command) { command->frag_seq = NULL; } - if (command->reply != NULL) { - freeReplyObject(command->reply); - } + freeReplyObject(command->reply); if (command->sub_commands != NULL) { listRelease(command->sub_commands); diff --git a/dict.c b/dict.c index 92c4b49..8178f4a 100644 --- a/dict.c +++ b/dict.c @@ -32,12 +32,12 @@ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE * POSSIBILITY OF SUCH DAMAGE. */ - -#include "dict.h" #include #include #include +#include "dict.h" + /* -------------------------- private prototypes ---------------------------- */ static int _dictExpandIfNeeded(dict *ht); @@ -124,7 +124,8 @@ static int dictExpand(dict *ht, unsigned long size) { } } assert(ht->used == 0); - free(ht->table); + hi_free(ht->table); + ht->table = NULL; /* Remap the new hashtable in the old */ *ht = n; @@ -167,13 +168,15 @@ static int _dictClear(dict *ht) { nextHe = he->next; dictFreeEntryKey(ht, he); dictFreeEntryVal(ht, he); - free(he); + hi_free(he); ht->used--; he = nextHe; } } /* Free the table and the allocated cache structure */ - free(ht->table); + hi_free(ht->table); + ht->table = NULL; + /* Re-initialize the table */ _dictReset(ht); return DICT_OK; /* never fails */ @@ -182,7 +185,7 @@ static int _dictClear(dict *ht) { /* Clear & Release the hash table */ static void dictRelease(dict *ht) { _dictClear(ht); - free(ht); + hi_free(ht); } static dictEntry *dictFind(dict *ht, const void *key) { @@ -231,7 +234,7 @@ static dictEntry *dictNext(dictIterator *iter) { return NULL; } -static void dictReleaseIterator(dictIterator *iter) { free(iter); } +static void dictReleaseIterator(dictIterator *iter) { hi_free(iter); } /* ------------------------- private functions ------------------------------ */ diff --git a/hiarray.c b/hiarray.c index dbc6adb..60d6f42 100644 --- a/hiarray.c +++ b/hiarray.c @@ -1,3 +1,4 @@ +#include #include #include "hiarray.h" @@ -49,9 +50,8 @@ int hiarray_init(struct hiarray *a, uint32_t n, size_t size) { void hiarray_deinit(struct hiarray *a) { ASSERT(a->nelem == 0); - if (a->elem != NULL) { - hi_free(a->elem); - } + hi_free(a->elem); + a->elem = NULL; } uint32_t hiarray_idx(struct hiarray *a, void *elem) { diff --git a/hircluster.c b/hircluster.c index fafd557..d5a8187 100644 --- a/hircluster.c +++ b/hircluster.c @@ -1,6 +1,7 @@ #include #include +#include #include #include #include @@ -283,9 +284,8 @@ static void cluster_node_deinit(cluster_node *node) { node->role = REDIS_ROLE_NULL; node->myself = 0; - if (node->con != NULL) { - redisFree(node->con); - } + redisFree(node->con); + node->con = NULL; if (node->acon != NULL) { redisAsyncFree(node->acon); @@ -453,10 +453,8 @@ static int authenticate(redisClusterContext *cc, redisContext *c) { return REDIS_OK; error: - if (reply != NULL) { - freeReplyObject(reply); - reply = NULL; - } + freeReplyObject(reply); + return REDIS_ERR; } @@ -505,6 +503,7 @@ static cluster_node *node_get_with_slots(redisClusterContext *cc, node->slots = listCreate(); if (node->slots == NULL) { hi_free(node); + node = NULL; __redisClusterSetError(cc, REDIS_ERR_OTHER, "slots for node listCreate error"); goto error; @@ -525,9 +524,7 @@ static cluster_node *node_get_with_slots(redisClusterContext *cc, error: - if (node != NULL) { - hi_free(node); - } + hi_free(node); return NULL; } @@ -1447,9 +1444,7 @@ static int cluster_update_route_by_addr(redisClusterContext *cc, const char *ip, freeReplyObject(reply); - if (c != NULL) { - redisFree(c); - } + redisFree(c); return REDIS_OK; @@ -1480,14 +1475,9 @@ static int cluster_update_route_by_addr(redisClusterContext *cc, const char *ip, dictRelease(nodes); } - if (reply != NULL) { - freeReplyObject(reply); - reply = NULL; - } + freeReplyObject(reply); - if (c != NULL) { - redisFree(c); - } + redisFree(c); return REDIS_ERR; } @@ -1635,11 +1625,13 @@ void redisClusterFree(redisClusterContext *cc) { return; if (cc->connect_timeout) { - free(cc->connect_timeout); + hi_free(cc->connect_timeout); + cc->connect_timeout = NULL; } if (cc->command_timeout) { - free(cc->command_timeout); + hi_free(cc->command_timeout); + cc->command_timeout = NULL; } memset(cc->table, 0, REDIS_CLUSTER_SLOTS * sizeof(cluster_node *)); @@ -1658,7 +1650,7 @@ void redisClusterFree(redisClusterContext *cc) { listRelease(cc->requests); } - free(cc); + hi_free(cc); } /* Connect to a Redis cluster. On error the field error in the returned @@ -2155,7 +2147,7 @@ static cluster_node *node_get_which_connected(redisClusterContext *cc) { dictReleaseIterator(di); return node; - } else if (reply != NULL) { + } else { freeReplyObject(reply); reply = NULL; } @@ -2236,17 +2228,13 @@ static char *cluster_config_get(redisClusterContext *cc, *config_value_len = sub_reply->len; sub_reply->str = NULL; - if (reply != NULL) { - freeReplyObject(reply); - } + freeReplyObject(reply); return config_value; error: - if (reply != NULL) { - freeReplyObject(reply); - } + freeReplyObject(reply); return NULL; } @@ -3168,7 +3156,7 @@ void *redisClustervCommand(redisClusterContext *cc, const char *format, reply = redisClusterFormattedCommand(cc, cmd, len); - free(cmd); + hi_free(cmd); return reply; } @@ -3198,7 +3186,7 @@ void *redisClusterCommandArgv(redisClusterContext *cc, int argc, reply = redisClusterFormattedCommand(cc, cmd, len); - free(cmd); + hi_free(cmd); return reply; } @@ -3332,7 +3320,7 @@ int redisClustervAppendCommand(redisClusterContext *cc, const char *format, ret = redisClusterAppendFormattedCommand(cc, cmd, len); - free(cmd); + hi_free(cmd); return ret; } @@ -3368,7 +3356,7 @@ int redisClusterAppendCommandArgv(redisClusterContext *cc, int argc, ret = redisClusterAppendFormattedCommand(cc, cmd, len); - free(cmd); + hi_free(cmd); return ret; } @@ -3655,12 +3643,9 @@ static void cluster_async_data_free(cluster_async_data *cad) { return; } - if (cad->command != NULL) { - command_destroy(cad->command); - } + command_destroy(cad->command); hi_free(cad); - cad = NULL; } static void unlinkAsyncContextAndNode(void *data) { @@ -3931,7 +3916,8 @@ static void redisClusterAsyncCallback(redisAsyncContext *ac, void *r, cluster_timeout = hi_atoi(cluster_timeout_str, cluster_timeout_str_len); - free(cluster_timeout_str); + hi_free(cluster_timeout_str); + if (cluster_timeout <= 0) { __redisClusterAsyncSetError( acc, REDIS_ERR_OTHER, @@ -4032,9 +4018,7 @@ static void redisClusterAsyncCallback(redisAsyncContext *ac, void *r, memset(acc->errstr, '\0', strlen(acc->errstr)); } - if (cad != NULL) { - cluster_async_data_free(cad); - } + cluster_async_data_free(cad); return; @@ -4050,9 +4034,7 @@ static void redisClusterAsyncCallback(redisAsyncContext *ac, void *r, error: - if (cad != NULL) { - cluster_async_data_free(cad); - } + cluster_async_data_free(cad); } int redisClusterAsyncFormattedCommand(redisClusterAsyncContext *acc, @@ -4169,9 +4151,7 @@ int redisClusterAsyncFormattedCommand(redisClusterAsyncContext *acc, error: - if (command != NULL) { - command_destroy(command); - } + command_destroy(command); if (commands != NULL) { listRelease(commands); @@ -4203,7 +4183,7 @@ int redisClustervAsyncCommand(redisClusterAsyncContext *acc, ret = redisClusterAsyncFormattedCommand(acc, fn, privdata, cmd, len); - free(cmd); + hi_free(cmd); return ret; } @@ -4237,7 +4217,7 @@ int redisClusterAsyncCommandArgv(redisClusterAsyncContext *acc, ret = redisClusterAsyncFormattedCommand(acc, fn, privdata, cmd, len); - free(cmd); + hi_free(cmd); return ret; } diff --git a/hiutil.c b/hiutil.c index f8e9908..05bd2cf 100644 --- a/hiutil.c +++ b/hiutil.c @@ -1,23 +1,25 @@ -#include "win32.h" #include #include +#include #include #include #include #include +#include #ifndef WIN32 #include #include #include #endif -#include "hiutil.h" -#include #ifdef HI_HAVE_BACKTRACE #include #endif +#include "hiutil.h" +#include "win32.h" + #ifndef WIN32 int hi_set_blocking(int sd) { int flags; @@ -269,15 +271,6 @@ void *_hi_realloc(void *ptr, size_t size, const char *name, int line) { return p; } -void _hi_free(void *ptr, const char *name, int line) { - ASSERT(ptr != NULL); - - if (name == NULL && line == 1) { - } - - free(ptr); -} - void hi_stacktrace(int skip_count) { if (skip_count > 0) { } @@ -299,7 +292,7 @@ void hi_stacktrace(int skip_count) { printf("[%d] %s\n", j, symbols[i]); } - free(symbols); + hi_free(symbols); #endif } diff --git a/hiutil.h b/hiutil.h index a7e8b0f..1d14d8f 100644 --- a/hiutil.h +++ b/hiutil.h @@ -168,17 +168,10 @@ int _uint_len(uint32_t num); #define hi_realloc(_p, _s) _hi_realloc(_p, (size_t)(_s), __FILE__, __LINE__) -#define hi_free(_p) \ - do { \ - _hi_free(_p, __FILE__, __LINE__); \ - (_p) = NULL; \ - } while (0) - void *_hi_alloc(size_t size, const char *name, int line); void *_hi_zalloc(size_t size, const char *name, int line); void *_hi_calloc(size_t nmemb, size_t size, const char *name, int line); void *_hi_realloc(void *ptr, size_t size, const char *name, int line); -void _hi_free(void *ptr, const char *name, int line); #define hi_strndup(_s, _n) strndup((char *)(_s), (size_t)(_n)); From 076369784bb0814ffb14505fa4c2d08a18c28db9 Mon Sep 17 00:00:00 2001 From: Bjorn Svensson Date: Thu, 3 Dec 2020 22:02:31 +0100 Subject: [PATCH 02/14] Use hi_realloc() from hiredis Hiredis uses its own allocator-functions which by default uses libc. By using the same design in hiredis-cluster a user can replace the allocator for both hiredis and hiredis-cluster at the same time. Own allocator functions can be enabled by the API: `hiredisAllocFuncs hiredisSetAllocators(hiredisAllocFuncs *ha)` - This commit only covers `realloc()` - Making sure that a realloc returning NULL is handled --- hiutil.c | 13 ------------- hiutil.h | 3 --- 2 files changed, 16 deletions(-) diff --git a/hiutil.c b/hiutil.c index 05bd2cf..4ea61ef 100644 --- a/hiutil.c +++ b/hiutil.c @@ -258,19 +258,6 @@ void *_hi_calloc(size_t nmemb, size_t size, const char *name, int line) { return _hi_zalloc(nmemb * size, name, line); } -void *_hi_realloc(void *ptr, size_t size, const char *name, int line) { - void *p; - - ASSERT(size != 0); - - p = realloc(ptr, size); - - if (name == NULL && line == 1) { - } - - return p; -} - void hi_stacktrace(int skip_count) { if (skip_count > 0) { } diff --git a/hiutil.h b/hiutil.h index 1d14d8f..462d16d 100644 --- a/hiutil.h +++ b/hiutil.h @@ -166,12 +166,9 @@ int _uint_len(uint32_t num); #define hi_calloc(_n, _s) \ _hi_calloc((size_t)(_n), (size_t)(_s), __FILE__, __LINE__) -#define hi_realloc(_p, _s) _hi_realloc(_p, (size_t)(_s), __FILE__, __LINE__) - void *_hi_alloc(size_t size, const char *name, int line); void *_hi_zalloc(size_t size, const char *name, int line); void *_hi_calloc(size_t nmemb, size_t size, const char *name, int line); -void *_hi_realloc(void *ptr, size_t size, const char *name, int line); #define hi_strndup(_s, _n) strndup((char *)(_s), (size_t)(_n)); From eeeb0524eba80587738d9470204daa143e63452c Mon Sep 17 00:00:00 2001 From: Bjorn Svensson Date: Thu, 3 Dec 2020 22:43:31 +0100 Subject: [PATCH 03/14] Use hi_calloc() from hiredis Hiredis uses its own allocator-functions which by default uses libc. By using the same design in hiredis-cluster a user can replace the allocator for both hiredis and hiredis-cluster at the same time. Own allocator functions can be enabled by the API: `hiredisAllocFuncs hiredisSetAllocators(hiredisAllocFuncs *ha)` - This commit only covers `calloc()` - Making sure that a calloc() returning NULL is handled --- dict.c | 4 +++- hircluster.c | 3 +-- hiutil.c | 4 ---- hiutil.h | 4 ---- 4 files changed, 4 insertions(+), 11 deletions(-) diff --git a/dict.c b/dict.c index 8178f4a..36315e9 100644 --- a/dict.c +++ b/dict.c @@ -96,7 +96,9 @@ static int dictExpand(dict *ht, unsigned long size) { _dictInit(&n, ht->type, ht->privdata); n.size = realsize; n.sizemask = realsize - 1; - n.table = calloc(realsize, sizeof(dictEntry *)); + n.table = hi_calloc(realsize, sizeof(dictEntry *)); + if (n.table == NULL) + return DICT_ERR; /* Copy all the elements from the old to the new table: * note that if the old hash table is empty ht->size is zero, diff --git a/hircluster.c b/hircluster.c index d5a8187..e2f9d8a 100644 --- a/hircluster.c +++ b/hircluster.c @@ -1588,7 +1588,7 @@ int test_cluster_update_route(redisClusterContext *cc) { redisClusterContext *redisClusterContextInit(void) { redisClusterContext *cc; - cc = calloc(1, sizeof(redisClusterContext)); + cc = hi_calloc(1, sizeof(redisClusterContext)); if (cc == NULL) return NULL; @@ -2898,7 +2898,6 @@ static void *command_post_fragment(redisClusterContext *cc, struct cmd *command, listReleaseIterator(list_iter); reply = hi_calloc(1, sizeof(*reply)); - if (reply == NULL) { __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); return NULL; diff --git a/hiutil.c b/hiutil.c index 4ea61ef..3d765b8 100644 --- a/hiutil.c +++ b/hiutil.c @@ -254,10 +254,6 @@ void *_hi_zalloc(size_t size, const char *name, int line) { return p; } -void *_hi_calloc(size_t nmemb, size_t size, const char *name, int line) { - return _hi_zalloc(nmemb * size, name, line); -} - void hi_stacktrace(int skip_count) { if (skip_count > 0) { } diff --git a/hiutil.h b/hiutil.h index 462d16d..1e11834 100644 --- a/hiutil.h +++ b/hiutil.h @@ -163,12 +163,8 @@ int _uint_len(uint32_t num); #define hi_zalloc(_s) _hi_zalloc((size_t)(_s), __FILE__, __LINE__) -#define hi_calloc(_n, _s) \ - _hi_calloc((size_t)(_n), (size_t)(_s), __FILE__, __LINE__) - void *_hi_alloc(size_t size, const char *name, int line); void *_hi_zalloc(size_t size, const char *name, int line); -void *_hi_calloc(size_t nmemb, size_t size, const char *name, int line); #define hi_strndup(_s, _n) strndup((char *)(_s), (size_t)(_n)); From e6dcd6091820d9d6d1f24e3f8ccc1b50cfff33a3 Mon Sep 17 00:00:00 2001 From: Bjorn Svensson Date: Thu, 3 Dec 2020 22:57:59 +0100 Subject: [PATCH 04/14] Remove unused string duplication macro and define There is no usage of strdup(), and therefor no need for hi_strdup() --- hiutil.h | 2 -- win32.h | 4 ---- 2 files changed, 6 deletions(-) diff --git a/hiutil.h b/hiutil.h index 1e11834..17de191 100644 --- a/hiutil.h +++ b/hiutil.h @@ -166,8 +166,6 @@ int _uint_len(uint32_t num); void *_hi_alloc(size_t size, const char *name, int line); void *_hi_zalloc(size_t size, const char *name, int line); -#define hi_strndup(_s, _n) strndup((char *)(_s), (size_t)(_n)); - #ifndef WIN32 /* * Wrappers to send or receive n byte message on a blocking diff --git a/win32.h b/win32.h index 7f7aea8..1d24450 100644 --- a/win32.h +++ b/win32.h @@ -8,10 +8,6 @@ #define inline __inline #endif -#ifndef strdup -#define strdup _strdup -#endif - #ifndef strcasecmp #define strcasecmp _stricmp #endif From e94a6729654e6e4bf440d3cf604daa7a5c189a5c Mon Sep 17 00:00:00 2001 From: Bjorn Svensson Date: Thu, 3 Dec 2020 23:34:57 +0100 Subject: [PATCH 05/14] Use hi_malloc() from hiredis Hiredis uses its own allocator-functions which by default uses libc. By using the same design in hiredis-cluster a user can replace the allocator for both hiredis and hiredis-cluster at the same time. Own allocator functions can be enabled by the API: `hiredisAllocFuncs hiredisSetAllocators(hiredisAllocFuncs *ha)` - This commit only covers `malloc()` - Making sure that a malloc() returning NULL is handled --- adlist.c | 10 +++++----- command.c | 6 ++++-- dict.c | 12 +++++++++--- hiarray.c | 6 +++--- hircluster.c | 37 +++++++++++++++++++++++-------------- hiutil.c | 17 ++--------------- hiutil.h | 12 +++--------- 7 files changed, 49 insertions(+), 51 deletions(-) diff --git a/adlist.c b/adlist.c index e0e451c..2c0e173 100644 --- a/adlist.c +++ b/adlist.c @@ -41,7 +41,7 @@ hilist *listCreate(void) { struct hilist *list; - if ((list = hi_alloc(sizeof(*list))) == NULL) + if ((list = hi_malloc(sizeof(*list))) == NULL) return NULL; list->head = list->tail = NULL; list->len = 0; @@ -79,7 +79,7 @@ void listRelease(hilist *list) { hilist *listAddNodeHead(hilist *list, void *value) { listNode *node; - if ((node = hi_alloc(sizeof(*node))) == NULL) + if ((node = hi_malloc(sizeof(*node))) == NULL) return NULL; node->value = value; if (list->len == 0) { @@ -104,7 +104,7 @@ hilist *listAddNodeHead(hilist *list, void *value) { hilist *listAddNodeTail(hilist *list, void *value) { listNode *node; - if ((node = hi_alloc(sizeof(*node))) == NULL) + if ((node = hi_malloc(sizeof(*node))) == NULL) return NULL; node->value = value; if (list->len == 0) { @@ -124,7 +124,7 @@ hilist *listInsertNode(hilist *list, listNode *old_node, void *value, int after) { listNode *node; - if ((node = hi_alloc(sizeof(*node))) == NULL) + if ((node = hi_malloc(sizeof(*node))) == NULL) return NULL; node->value = value; if (after) { @@ -176,7 +176,7 @@ void listDelNode(hilist *list, listNode *node) { listIter *listGetIterator(hilist *list, int direction) { listIter *iter; - if ((iter = hi_alloc(sizeof(*iter))) == NULL) + if ((iter = hi_malloc(sizeof(*iter))) == NULL) return NULL; if (direction == AL_START_HEAD) iter->next = list->head; diff --git a/command.c b/command.c index 0a43e97..79447a0 100644 --- a/command.c +++ b/command.c @@ -1606,7 +1606,9 @@ void redis_parse_cmd(struct cmd *r) { r->result = CMD_PARSE_ERROR; errno = EINVAL; if (r->errstr == NULL) { - r->errstr = hi_alloc(100 * sizeof(*r->errstr)); + r->errstr = hi_malloc(100 * sizeof(*r->errstr)); + if (r->errstr == NULL) + return; } len = _scnprintf( @@ -1618,7 +1620,7 @@ void redis_parse_cmd(struct cmd *r) { struct cmd *command_get() { struct cmd *command; - command = hi_alloc(sizeof(struct cmd)); + command = hi_malloc(sizeof(struct cmd)); if (command == NULL) { return NULL; } diff --git a/dict.c b/dict.c index 36315e9..377e17b 100644 --- a/dict.c +++ b/dict.c @@ -70,7 +70,9 @@ static void _dictReset(dict *ht) { /* Create a new hash table */ static dict *dictCreate(dictType *type, void *privDataPtr) { - dict *ht = malloc(sizeof(*ht)); + dict *ht = hi_malloc(sizeof(*ht)); + if (ht == NULL) + return NULL; _dictInit(ht, type, privDataPtr); return ht; } @@ -145,7 +147,9 @@ static int dictAdd(dict *ht, void *key, void *val) { return DICT_ERR; /* Allocates the memory and stores key */ - entry = malloc(sizeof(*entry)); + entry = hi_malloc(sizeof(*entry)); + if (entry == NULL) + return DICT_ERR; entry->next = ht->table[index]; ht->table[index] = entry; @@ -207,7 +211,9 @@ static dictEntry *dictFind(dict *ht, const void *key) { } static dictIterator *dictGetIterator(dict *ht) { - dictIterator *iter = malloc(sizeof(*iter)); + dictIterator *iter = hi_malloc(sizeof(*iter)); + if (iter == NULL) + return NULL; iter->ht = ht; iter->index = -1; diff --git a/hiarray.c b/hiarray.c index 60d6f42..0cf9583 100644 --- a/hiarray.c +++ b/hiarray.c @@ -9,12 +9,12 @@ struct hiarray *hiarray_create(uint32_t n, size_t size) { ASSERT(n != 0 && size != 0); - a = hi_alloc(sizeof(*a)); + a = hi_malloc(sizeof(*a)); if (a == NULL) { return NULL; } - a->elem = hi_alloc(n * size); + a->elem = hi_malloc(n * size); if (a->elem == NULL) { hi_free(a); return NULL; @@ -35,7 +35,7 @@ void hiarray_destroy(struct hiarray *a) { int hiarray_init(struct hiarray *a, uint32_t n, size_t size) { ASSERT(n != 0 && size != 0); - a->elem = hi_alloc(n * size); + a->elem = hi_malloc(n * size); if (a->elem == NULL) { return HI_ENOMEM; } diff --git a/hircluster.c b/hircluster.c index e2f9d8a..403be41 100644 --- a/hircluster.c +++ b/hircluster.c @@ -331,7 +331,7 @@ static int cluster_slot_init(cluster_slot *slot, cluster_node *node) { static cluster_slot *cluster_slot_create(cluster_node *node) { cluster_slot *slot; - slot = hi_alloc(sizeof(*slot)); + slot = hi_malloc(sizeof(*slot)); if (slot == NULL) { return NULL; } @@ -393,7 +393,7 @@ static copen_slot *cluster_open_slot_create(uint32_t slot_num, int migrate, cluster_node *node) { copen_slot *oslot; - oslot = hi_alloc(sizeof(*oslot)); + oslot = hi_malloc(sizeof(*oslot)); if (oslot == NULL) { return NULL; } @@ -491,7 +491,7 @@ static cluster_node *node_get_with_slots(redisClusterContext *cc, goto error; } - node = hi_alloc(sizeof(cluster_node)); + node = hi_malloc(sizeof(cluster_node)); if (node == NULL) { __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); goto error; @@ -543,7 +543,7 @@ static cluster_node *node_get_with_nodes(redisClusterContext *cc, return NULL; } - node = hi_alloc(sizeof(cluster_node)); + node = hi_malloc(sizeof(cluster_node)); if (node == NULL) { __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); return NULL; @@ -1719,7 +1719,10 @@ redisClusterContext *redisClusterConnectWithTimeout(const char *addrs, } if (cc->connect_timeout == NULL) { - cc->connect_timeout = malloc(sizeof(struct timeval)); + cc->connect_timeout = hi_malloc(sizeof(struct timeval)); + if (cc->connect_timeout == NULL) { + return NULL; + } } memcpy(cc->connect_timeout, &tv, sizeof(struct timeval)); @@ -1801,7 +1804,7 @@ int redisClusterSetOptionAddNode(redisClusterContext *cc, const char *addr) { return REDIS_ERR; } - node = hi_alloc(sizeof(cluster_node)); + node = hi_malloc(sizeof(cluster_node)); if (node == NULL) { sdsfree(ip); __redisClusterSetError(cc, REDIS_ERR_OTHER, @@ -1945,7 +1948,10 @@ int redisClusterSetOptionConnectTimeout(redisClusterContext *cc, } if (cc->connect_timeout == NULL) { - cc->connect_timeout = malloc(sizeof(struct timeval)); + cc->connect_timeout = hi_malloc(sizeof(struct timeval)); + if (cc->connect_timeout == NULL) { + return REDIS_ERR; + } } memcpy(cc->connect_timeout, &tv, sizeof(struct timeval)); @@ -1961,7 +1967,10 @@ int redisClusterSetOptionTimeout(redisClusterContext *cc, } if (cc->command_timeout == NULL) { - cc->command_timeout = malloc(sizeof(struct timeval)); + cc->command_timeout = hi_malloc(sizeof(struct timeval)); + if (cc->command_timeout == NULL) { + return REDIS_ERR; + } memcpy(cc->command_timeout, &tv, sizeof(struct timeval)); } else if (cc->command_timeout->tv_sec != tv.tv_sec || cc->command_timeout->tv_usec != tv.tv_usec) { @@ -2349,7 +2358,7 @@ static cluster_node *node_get_by_ask_error_reply(redisClusterContext *cc, if (ip_port != NULL && ip_port_len == 2) { de = dictFind(cc->nodes, part[2]); if (de == NULL) { - node = hi_alloc(sizeof(cluster_node)); + node = hi_malloc(sizeof(cluster_node)); if (node == NULL) { __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); @@ -2561,7 +2570,7 @@ static int command_pre_fragment(redisClusterContext *cc, struct cmd *command, goto done; } - command->frag_seq = hi_alloc(key_count * sizeof(*command->frag_seq)); + command->frag_seq = hi_malloc(key_count * sizeof(*command->frag_seq)); if (command->frag_seq == NULL) { __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); goto done; @@ -2951,7 +2960,7 @@ static void *command_post_fragment(redisClusterContext *cc, struct cmd *command, } else if (command->type == CMD_REQ_REDIS_MSET) { reply->type = REDIS_REPLY_STATUS; uint32_t str_len = strlen(REDIS_STATUS_OK); - reply->str = hi_alloc((str_len + 1) * sizeof(char *)); + reply->str = hi_malloc((str_len + 1) * sizeof(char *)); if (reply->str == NULL) { freeReplyObject(reply); __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); @@ -3598,7 +3607,7 @@ redisClusterAsyncInitialize(redisClusterContext *cc) { return NULL; } - acc = hi_alloc(sizeof(redisClusterAsyncContext)); + acc = hi_malloc(sizeof(redisClusterAsyncContext)); if (acc == NULL) return NULL; @@ -3623,7 +3632,7 @@ redisClusterAsyncInitialize(redisClusterContext *cc) { static cluster_async_data *cluster_async_data_get(void) { cluster_async_data *cad; - cad = hi_alloc(sizeof(cluster_async_data)); + cad = hi_malloc(sizeof(cluster_async_data)); if (cad == NULL) { return NULL; } @@ -4071,7 +4080,7 @@ int redisClusterAsyncFormattedCommand(redisClusterAsyncContext *acc, goto error; } - command->cmd = malloc(len * sizeof(*command->cmd)); + command->cmd = hi_malloc(len * sizeof(*command->cmd)); if (command->cmd == NULL) { __redisClusterAsyncSetError(acc, REDIS_ERR_OOM, "Out of memory"); goto error; diff --git a/hiutil.c b/hiutil.c index 3d765b8..fd85cfb 100644 --- a/hiutil.c +++ b/hiutil.c @@ -230,23 +230,10 @@ int _uint_len(uint32_t num) { return n; } -void *_hi_alloc(size_t size, const char *name, int line) { +void *_hi_zalloc(size_t size) { void *p; - ASSERT(size != 0); - - p = malloc(size); - - if (name == NULL && line == 1) { - } - - return p; -} - -void *_hi_zalloc(size_t size, const char *name, int line) { - void *p; - - p = _hi_alloc(size, name, line); + p = hi_malloc(size); if (p != NULL) { memset(p, 0, size); } diff --git a/hiutil.h b/hiutil.h index 17de191..6c3f0e5 100644 --- a/hiutil.h +++ b/hiutil.h @@ -154,17 +154,11 @@ int hi_valid_port(int n); int _uint_len(uint32_t num); /* - * Memory allocation and free wrappers. - * - * These wrappers enables us to loosely detect double free, dangling - * pointer access and zero-byte alloc. + * Memory allocation wrapper. */ -#define hi_alloc(_s) _hi_alloc((size_t)(_s), __FILE__, __LINE__) - -#define hi_zalloc(_s) _hi_zalloc((size_t)(_s), __FILE__, __LINE__) +#define hi_zalloc(_s) _hi_zalloc((size_t)(_s)) -void *_hi_alloc(size_t size, const char *name, int line); -void *_hi_zalloc(size_t size, const char *name, int line); +void *_hi_zalloc(size_t size); #ifndef WIN32 /* From 36dec32db7db76893ececdba4bffb869a69eaabd Mon Sep 17 00:00:00 2001 From: Bjorn Svensson Date: Thu, 3 Dec 2020 23:48:40 +0100 Subject: [PATCH 06/14] Remove hi_zalloc() To have a common ground with hiredis regarding allocators the not so commonly used hi_zalloc() is replaced with a hi_malloc() and a memset(). Also making sure that hi_malloc() returning NULL is handled. --- hircluster.c | 19 ++++++++++++++----- hiutil.c | 11 ----------- hiutil.h | 7 ------- 3 files changed, 14 insertions(+), 23 deletions(-) diff --git a/hircluster.c b/hircluster.c index 403be41..55aad0a 100644 --- a/hircluster.c +++ b/hircluster.c @@ -2564,11 +2564,12 @@ static int command_pre_fragment(redisClusterContext *cc, struct cmd *command, key_count = hiarray_n(command->keys); - sub_commands = hi_zalloc(REDIS_CLUSTER_SLOTS * sizeof(*sub_commands)); + sub_commands = hi_malloc(REDIS_CLUSTER_SLOTS * sizeof(*sub_commands)); if (sub_commands == NULL) { __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); goto done; } + memset(sub_commands, 0, REDIS_CLUSTER_SLOTS * sizeof(*sub_commands)); command->frag_seq = hi_malloc(key_count * sizeof(*command->frag_seq)); if (command->frag_seq == NULL) { @@ -2662,12 +2663,14 @@ static int command_pre_fragment(redisClusterContext *cc, struct cmd *command, sub_command->clen += 13 + num_str_len; sub_command->cmd = - hi_zalloc(sub_command->clen * sizeof(*sub_command->cmd)); + hi_malloc(sub_command->clen * sizeof(*sub_command->cmd)); if (sub_command->cmd == NULL) { __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); slot_num = -1; goto done; } + memset(sub_command->cmd, 0, + sub_command->clen * sizeof(*sub_command->cmd)); sub_command->cmd[idx++] = '*'; memcpy(sub_command->cmd + idx, num_str, num_str_len); @@ -2704,12 +2707,14 @@ static int command_pre_fragment(redisClusterContext *cc, struct cmd *command, sub_command->clen += 12 + num_str_len; sub_command->cmd = - hi_zalloc(sub_command->clen * sizeof(*sub_command->cmd)); + hi_malloc(sub_command->clen * sizeof(*sub_command->cmd)); if (sub_command->cmd == NULL) { __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); slot_num = -1; goto done; } + memset(sub_command->cmd, 0, + sub_command->clen * sizeof(*sub_command->cmd)); sub_command->cmd[idx++] = '*'; memcpy(sub_command->cmd + idx, num_str, num_str_len); @@ -2746,12 +2751,14 @@ static int command_pre_fragment(redisClusterContext *cc, struct cmd *command, sub_command->clen += 15 + num_str_len; sub_command->cmd = - hi_zalloc(sub_command->clen * sizeof(*sub_command->cmd)); + hi_malloc(sub_command->clen * sizeof(*sub_command->cmd)); if (sub_command->cmd == NULL) { __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); slot_num = -1; goto done; } + memset(sub_command->cmd, 0, + sub_command->clen * sizeof(*sub_command->cmd)); sub_command->cmd[idx++] = '*'; memcpy(sub_command->cmd + idx, num_str, num_str_len); @@ -2790,12 +2797,14 @@ static int command_pre_fragment(redisClusterContext *cc, struct cmd *command, sub_command->clen += 13 + num_str_len; sub_command->cmd = - hi_zalloc(sub_command->clen * sizeof(*sub_command->cmd)); + hi_malloc(sub_command->clen * sizeof(*sub_command->cmd)); if (sub_command->cmd == NULL) { __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); slot_num = -1; goto done; } + memset(sub_command->cmd, 0, + sub_command->clen * sizeof(*sub_command->cmd)); sub_command->cmd[idx++] = '*'; memcpy(sub_command->cmd + idx, num_str, num_str_len); diff --git a/hiutil.c b/hiutil.c index fd85cfb..fae6f6f 100644 --- a/hiutil.c +++ b/hiutil.c @@ -230,17 +230,6 @@ int _uint_len(uint32_t num) { return n; } -void *_hi_zalloc(size_t size) { - void *p; - - p = hi_malloc(size); - if (p != NULL) { - memset(p, 0, size); - } - - return p; -} - void hi_stacktrace(int skip_count) { if (skip_count > 0) { } diff --git a/hiutil.h b/hiutil.h index 6c3f0e5..c4ba6f4 100644 --- a/hiutil.h +++ b/hiutil.h @@ -153,13 +153,6 @@ int hi_valid_port(int n); int _uint_len(uint32_t num); -/* - * Memory allocation wrapper. - */ -#define hi_zalloc(_s) _hi_zalloc((size_t)(_s)) - -void *_hi_zalloc(size_t size); - #ifndef WIN32 /* * Wrappers to send or receive n byte message on a blocking From c2857addcb3b900af621083f79470fdf19845baa Mon Sep 17 00:00:00 2001 From: Bjorn Svensson Date: Fri, 4 Dec 2020 15:30:10 +0100 Subject: [PATCH 07/14] Handle OOM in creations of list and dict iterators The iterators performs a malloc under the hood that might fail due to out of memory. This avoids using a null iterator. --- hircluster.c | 151 ++++++++++++++++++++++++++++++++++----------------- 1 file changed, 102 insertions(+), 49 deletions(-) diff --git a/hircluster.c b/hircluster.c index 55aad0a..d5f4231 100644 --- a/hircluster.c +++ b/hircluster.c @@ -615,6 +615,10 @@ static void cluster_nodes_swap_ctx(dict *nodes_f, dict *nodes_t) { } di = dictGetIterator(nodes_t); + if (di == NULL) { + return; + } + while ((de_t = dictNext(di)) != NULL) { node_t = dictGetEntryVal(de_t); if (node_t == NULL) { @@ -643,7 +647,6 @@ static void cluster_nodes_swap_ctx(dict *nodes_f, dict *nodes_t) { node_f->acon->data = node_f; } } - dictReleaseIterator(di); } @@ -1369,9 +1372,7 @@ static int cluster_update_route_by_addr(redisClusterContext *cc, const char *ip, dit = dictGetIterator(nodes); if (dit == NULL) { - __redisClusterSetError(cc, REDIS_ERR_OOM, - "Dict get iterator failed: out of memory"); - goto error; + goto oom; } while ((den = dictNext(dit))) { @@ -1388,9 +1389,7 @@ static int cluster_update_route_by_addr(redisClusterContext *cc, const char *ip, lit = listGetIterator(master->slots, AL_START_HEAD); if (lit == NULL) { - __redisClusterSetError(cc, REDIS_ERR_OOM, - "List get iterator failed: out of memory"); - goto error; + goto oom; } while ((lnode = listNext(lit))) { @@ -1448,15 +1447,14 @@ static int cluster_update_route_by_addr(redisClusterContext *cc, const char *ip, return REDIS_OK; -error: +oom: + __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); + // passthrough - if (dit != NULL) { - dictReleaseIterator(dit); - } +error: - if (lit != NULL) { - listReleaseIterator(lit); - } + dictReleaseIterator(dit); + listReleaseIterator(lit); if (slots != NULL) { if (slots == cc->slots) { @@ -1502,6 +1500,11 @@ int cluster_update_route(redisClusterContext *cc) { } it = dictGetIterator(cc->nodes); + if (it == NULL) { + __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); + return REDIS_ERR; + } + while ((de = dictNext(it)) != NULL) { node = dictGetEntryVal(de); if (node == NULL || node->host == NULL || node->port < 0) { @@ -1533,9 +1536,9 @@ int cluster_update_route(redisClusterContext *cc) { #ifdef DEBUG static void print_cluster_node_list(redisClusterContext *cc) { - dictIterator *di = NULL; + dictIterator *dit = NULL; dictEntry *de; - listIter *it; + listIter *lit; listNode *ln; cluster_node *master, *slave; hilist *slaves; @@ -1544,11 +1547,15 @@ static void print_cluster_node_list(redisClusterContext *cc) { return; } - di = dictGetIterator(cc->nodes); + dit = dictGetIterator(cc->nodes); + if (dit == NULL) { + __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); + return; + } printf("name\taddress\trole\tslaves\n"); - while ((de = dictNext(di)) != NULL) { + while ((de = dictNext(dit)) != NULL) { master = dictGetEntryVal(de); printf("%s\t%s\t%d\t%s\n", master->name, master->addr, master->role, @@ -1559,17 +1566,22 @@ static void print_cluster_node_list(redisClusterContext *cc) { continue; } - it = listGetIterator(slaves, AL_START_HEAD); - while ((ln = listNext(it)) != NULL) { + lit = listGetIterator(slaves, AL_START_HEAD); + if (lit == NULL) { + __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); + return; + } + while ((ln = listNext(lit)) != NULL) { slave = listNodeValue(ln); printf("%s\t%s\t%d\t%s\n", slave->name, slave->addr, slave->role, slave->slaves ? "hava" : "null"); } - listReleaseIterator(it); + listReleaseIterator(lit); printf("\n"); } + dictReleaseIterator(dit); } #endif /* DEBUG */ @@ -1962,6 +1974,8 @@ int redisClusterSetOptionConnectTimeout(redisClusterContext *cc, int redisClusterSetOptionTimeout(redisClusterContext *cc, const struct timeval tv) { + dictIterator *di = NULL; + if (cc == NULL) { return REDIS_ERR; } @@ -1978,10 +1992,12 @@ int redisClusterSetOptionTimeout(redisClusterContext *cc, if (cc->nodes && dictSize(cc->nodes) > 0) { dictEntry *de; - dictIterator *di; cluster_node *node; di = dictGetIterator(cc->nodes); + if (di == NULL) { + goto oom; + } while ((de = dictNext(di)) != NULL) { node = dictGetEntryVal(de); @@ -1996,6 +2012,10 @@ int redisClusterSetOptionTimeout(redisClusterContext *cc, listNode *ln; li = listGetIterator(node->slaves, AL_START_HEAD); + if (li == NULL) { + goto oom; + } + while ((ln = listNext(li)) != NULL) { slave = listNodeValue(ln); if (slave->con && slave->con->flags & REDIS_CONNECTED && @@ -2003,16 +2023,19 @@ int redisClusterSetOptionTimeout(redisClusterContext *cc, redisSetTimeout(slave->con, tv); } } - listReleaseIterator(li); } } - dictReleaseIterator(di); } } return REDIS_OK; + +oom: + dictReleaseIterator(di); + __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); + return REDIS_ERR; } int redisClusterSetOptionMaxRedirect(redisClusterContext *cc, @@ -2136,6 +2159,11 @@ static cluster_node *node_get_which_connected(redisClusterContext *cc) { } di = dictGetIterator(cc->nodes); + if (di == NULL) { + __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); + return NULL; + } + while ((de = dictNext(di)) != NULL) { node = dictGetEntryVal(de); if (node == NULL) { @@ -2163,7 +2191,6 @@ static cluster_node *node_get_which_connected(redisClusterContext *cc) { } dictReleaseIterator(di); - return NULL; } @@ -2865,10 +2892,13 @@ static void *command_post_fragment(redisClusterContext *cc, struct cmd *command, struct cmd *sub_command; listNode *list_node; listIter *list_iter; - redisReply *reply, *sub_reply; + redisReply *reply = NULL, *sub_reply; long long count = 0; list_iter = listGetIterator(commands, AL_START_HEAD); + if (list_iter == NULL) { + goto oom; + } while ((list_node = listNext(list_iter)) != NULL) { sub_command = list_node->value; reply = sub_command->reply; @@ -2984,6 +3014,11 @@ static void *command_post_fragment(redisClusterContext *cc, struct cmd *command, } return reply; + +oom: + freeReplyObject(reply); + __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); + return NULL; } /* @@ -3099,6 +3134,10 @@ void *redisClusterFormattedCommand(redisClusterContext *cc, char *cmd, ASSERT(listLength(commands) != 1); list_iter = listGetIterator(commands, AL_START_HEAD); + if (list_iter == NULL) { + goto oom; + } + while ((list_node = listNext(list_iter)) != NULL) { sub_command = list_node->value; @@ -3123,14 +3162,14 @@ void *redisClusterFormattedCommand(redisClusterContext *cc, char *cmd, listRelease(commands); } - if (list_iter != NULL) { - listReleaseIterator(list_iter); - } - + listReleaseIterator(list_iter); cc->retry_count = 0; - return reply; +oom: + __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); + // passthrough + error: if (command != NULL) { @@ -3142,12 +3181,8 @@ void *redisClusterFormattedCommand(redisClusterContext *cc, char *cmd, listRelease(commands); } - if (list_iter != NULL) { - listReleaseIterator(list_iter); - } - + listReleaseIterator(list_iter); cc->retry_count = 0; - return NULL; } @@ -3264,6 +3299,10 @@ int redisClusterAppendFormattedCommand(redisClusterContext *cc, char *cmd, ASSERT(listLength(commands) != 1); list_iter = listGetIterator(commands, AL_START_HEAD); + if (list_iter == NULL) { + goto oom; + } + while ((list_node = listNext(list_iter)) != NULL) { sub_command = list_node->value; @@ -3290,28 +3329,25 @@ int redisClusterAppendFormattedCommand(redisClusterContext *cc, char *cmd, } } - if (list_iter != NULL) { - listReleaseIterator(list_iter); - } - + listReleaseIterator(list_iter); listAddNodeTail(cc->requests, command); return REDIS_OK; +oom: + __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); + // passthrough + error: if (command != NULL) { command->cmd = NULL; command_destroy(command); } - if (commands != NULL) { listRelease(commands); } - - if (list_iter != NULL) { - listReleaseIterator(list_iter); - } + listReleaseIterator(list_iter); /* Attention: mybe here we must pop the sub_commands that had append to the nodes. @@ -3390,6 +3426,11 @@ static int redisClusterSendAll(redisClusterContext *cc) { } di = dictGetIterator(cc->nodes); + if (di == NULL) { + __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); + return REDIS_ERR; + } + while ((de = dictNext(di)) != NULL) { node = dictGetEntryVal(de); if (node == NULL) { @@ -3435,7 +3476,13 @@ static int redisClusterClearAll(redisClusterContext *cc) { if (cc->nodes == NULL) { return REDIS_ERR; } + di = dictGetIterator(cc->nodes); + if (di == NULL) { + __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); + return REDIS_ERR; + } + while ((de = dictNext(di)) != NULL) { node = dictGetEntryVal(de); if (node == NULL) { @@ -3507,6 +3554,11 @@ int redisClusterGetReply(redisClusterContext *cc, void **reply) { ASSERT(listLength(commands) != 1); list_iter = listGetIterator(commands, AL_START_HEAD); + if (list_iter == NULL) { + __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); + goto error; + } + while ((list_sub_command = listNext(list_iter)) != NULL) { sub_command = list_sub_command->value; if (sub_command == NULL) { @@ -3539,10 +3591,7 @@ int redisClusterGetReply(redisClusterContext *cc, void **reply) { error: - if (list_iter != NULL) { - listReleaseIterator(list_iter); - } - + listReleaseIterator(list_iter); listDelNode(cc->requests, list_command); return REDIS_ERR; } @@ -4261,6 +4310,10 @@ void redisClusterAsyncDisconnect(redisClusterAsyncContext *acc) { } di = dictGetIterator(nodes); + if (di == NULL) { + __redisClusterAsyncSetError(acc, REDIS_ERR_OOM, "Out of memory"); + return; + } while ((de = dictNext(di)) != NULL) { node = dictGetEntryVal(de); From e42ab36b3e96e4927e47ebcb90d17f9cbdca8cb5 Mon Sep 17 00:00:00 2001 From: Bjorn Svensson Date: Fri, 4 Dec 2020 15:38:19 +0100 Subject: [PATCH 08/14] Handle oom commonly in each function Making sure to set the error string when there is an out of memory and cleanup the temporary allocated data. --- dict.c | 6 +-- hircluster.c | 102 ++++++++++++++++++++------------------------------- 2 files changed, 43 insertions(+), 65 deletions(-) diff --git a/dict.c b/dict.c index 377e17b..5b41240 100644 --- a/dict.c +++ b/dict.c @@ -32,6 +32,7 @@ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE * POSSIBILITY OF SUCH DAMAGE. */ + #include #include #include @@ -73,6 +74,7 @@ static dict *dictCreate(dictType *type, void *privDataPtr) { dict *ht = hi_malloc(sizeof(*ht)); if (ht == NULL) return NULL; + _dictInit(ht, type, privDataPtr); return ht; } @@ -129,7 +131,6 @@ static int dictExpand(dict *ht, unsigned long size) { } assert(ht->used == 0); hi_free(ht->table); - ht->table = NULL; /* Remap the new hashtable in the old */ *ht = n; @@ -150,6 +151,7 @@ static int dictAdd(dict *ht, void *key, void *val) { entry = hi_malloc(sizeof(*entry)); if (entry == NULL) return DICT_ERR; + entry->next = ht->table[index]; ht->table[index] = entry; @@ -181,8 +183,6 @@ static int _dictClear(dict *ht) { } /* Free the table and the allocated cache structure */ hi_free(ht->table); - ht->table = NULL; - /* Re-initialize the table */ _dictReset(ht); return DICT_OK; /* never fails */ diff --git a/hircluster.c b/hircluster.c index d5f4231..b36afd8 100644 --- a/hircluster.c +++ b/hircluster.c @@ -700,8 +700,7 @@ static int cluster_master_slave_mapping_with_name(redisClusterContext *cc, if (node->slaves == NULL) { node->slaves = listCreate(); if (node->slaves == NULL) { - __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); - return REDIS_ERR; + goto oom; } node->slaves->free = listClusterNodeDestructor; @@ -725,8 +724,7 @@ static int cluster_master_slave_mapping_with_name(redisClusterContext *cc, if (node_old->slaves == NULL) { node_old->slaves = listCreate(); if (node_old->slaves == NULL) { - __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); - return REDIS_ERR; + goto oom; } node_old->slaves->free = listClusterNodeDestructor; @@ -739,6 +737,10 @@ static int cluster_master_slave_mapping_with_name(redisClusterContext *cc, } return REDIS_OK; + +oom: + __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); + return REDIS_ERR; } /** @@ -764,8 +766,7 @@ dict *parse_cluster_slots(redisClusterContext *cc, redisReply *reply, nodes = dictCreate(&clusterNodesDictType, NULL); if (nodes == NULL) { - __redisClusterSetError(cc, REDIS_ERR_OOM, "out of memory"); - goto error; + goto oom; } if (reply->type != REDIS_REPLY_ARRAY || reply->elements <= 0) { @@ -786,9 +787,7 @@ dict *parse_cluster_slots(redisClusterContext *cc, redisReply *reply, slot = cluster_slot_create(NULL); if (slot == NULL) { - __redisClusterSetError(cc, REDIS_ERR_OOM, - "Slot create failed: out of memory."); - goto error; + goto oom; } // one slots region @@ -859,10 +858,7 @@ dict *parse_cluster_slots(redisClusterContext *cc, redisReply *reply, master = dictGetEntryVal(den); ret = cluster_slot_ref_node(slot, master); if (ret != REDIS_OK) { - __redisClusterSetError( - cc, REDIS_ERR_OOM, - "Slot ref node failed: out of memory."); - goto error; + goto oom; } slot = NULL; @@ -890,10 +886,7 @@ dict *parse_cluster_slots(redisClusterContext *cc, redisReply *reply, ret = cluster_slot_ref_node(slot, master); if (ret != REDIS_OK) { - __redisClusterSetError( - cc, REDIS_ERR_OOM, - "Slot ref node failed: out of memory."); - goto error; + goto oom; } slot = NULL; @@ -907,10 +900,8 @@ dict *parse_cluster_slots(redisClusterContext *cc, redisReply *reply, if (master->slaves == NULL) { master->slaves = listCreate(); if (master->slaves == NULL) { - __redisClusterSetError(cc, REDIS_ERR_OOM, - "Out of memory"); cluster_node_deinit(slave); - goto error; + goto oom; } master->slaves->free = listClusterNodeDestructor; @@ -924,6 +915,10 @@ dict *parse_cluster_slots(redisClusterContext *cc, redisReply *reply, return nodes; +oom: + __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); + // passthrough + error: if (nodes != NULL) { @@ -959,8 +954,7 @@ dict *parse_cluster_nodes(redisClusterContext *cc, char *str, int str_len, nodes = dictCreate(&clusterNodesDictType, NULL); if (nodes == NULL) { - __redisClusterSetError(cc, REDIS_ERR_OOM, "out of memory"); - goto error; + goto oom; } start = str; @@ -1168,9 +1162,7 @@ dict *parse_cluster_nodes(redisClusterContext *cc, char *str, int str_len, slot = cluster_slot_create(master); if (slot == NULL) { - __redisClusterSetError(cc, REDIS_ERR_OOM, - "Out of memory"); - goto error; + goto oom; } slot->start = (uint32_t)slot_start; @@ -1219,6 +1211,10 @@ dict *parse_cluster_nodes(redisClusterContext *cc, char *str, int str_len, return nodes; +oom: + __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); + // passthrough + error: if (part != NULL) { @@ -1365,9 +1361,7 @@ static int cluster_update_route_by_addr(redisClusterContext *cc, const char *ip, slots = hiarray_create(dictSize(nodes), sizeof(cluster_slot *)); if (slots == NULL) { - __redisClusterSetError(cc, REDIS_ERR_OTHER, - "Slots array create failed: out of memory"); - goto error; + goto oom; } dit = dictGetIterator(nodes); @@ -1378,7 +1372,7 @@ static int cluster_update_route_by_addr(redisClusterContext *cc, const char *ip, while ((den = dictNext(dit))) { master = dictGetEntryVal(den); if (master->role != REDIS_ROLE_MASTER) { - __redisClusterSetError(cc, REDIS_ERR_OOM, + __redisClusterSetError(cc, REDIS_ERR_OTHER, "Node role must be master"); goto error; } @@ -1424,6 +1418,7 @@ static int cluster_update_route_by_addr(redisClusterContext *cc, const char *ip, } } + // Move all hiredis contexts in cc->nodes to nodes cluster_nodes_swap_ctx(cc->nodes, nodes); if (cc->nodes != NULL) { dictRelease(cc->nodes); @@ -1469,12 +1464,10 @@ static int cluster_update_route_by_addr(redisClusterContext *cc, const char *ip, if (nodes == cc->nodes) { cc->nodes = NULL; } - dictRelease(nodes); } freeReplyObject(reply); - redisFree(c); return REDIS_ERR; @@ -1983,7 +1976,7 @@ int redisClusterSetOptionTimeout(redisClusterContext *cc, if (cc->command_timeout == NULL) { cc->command_timeout = hi_malloc(sizeof(struct timeval)); if (cc->command_timeout == NULL) { - return REDIS_ERR; + goto oom; } memcpy(cc->command_timeout, &tv, sizeof(struct timeval)); } else if (cc->command_timeout->tv_sec != tv.tv_sec || @@ -2388,7 +2381,6 @@ static cluster_node *node_get_by_ask_error_reply(redisClusterContext *cc, node = hi_malloc(sizeof(cluster_node)); if (node == NULL) { __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); - goto done; } @@ -2922,7 +2914,6 @@ static void *command_post_fragment(redisClusterContext *cc, struct cmd *command, listReleaseIterator(list_iter); return NULL; } - count += reply->integer; } else if (command->type == CMD_REQ_REDIS_EXISTS) { if (reply->type != REDIS_REPLY_INTEGER) { @@ -2930,7 +2921,6 @@ static void *command_post_fragment(redisClusterContext *cc, struct cmd *command, listReleaseIterator(list_iter); return NULL; } - count += reply->integer; } else if (command->type == CMD_REQ_REDIS_MSET) { if (reply->type != REDIS_REPLY_STATUS || reply->len != 2 || @@ -2947,8 +2937,7 @@ static void *command_post_fragment(redisClusterContext *cc, struct cmd *command, reply = hi_calloc(1, sizeof(*reply)); if (reply == NULL) { - __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); - return NULL; + goto oom; } if (command->type == CMD_REQ_REDIS_MGET) { @@ -2962,9 +2951,7 @@ static void *command_post_fragment(redisClusterContext *cc, struct cmd *command, reply->elements = key_count; reply->element = hi_calloc(key_count, sizeof(*reply)); if (reply->element == NULL) { - freeReplyObject(reply); - __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); - return NULL; + goto oom; } for (i = key_count - 1; i >= 0; i--) { /* for each key */ @@ -3001,9 +2988,7 @@ static void *command_post_fragment(redisClusterContext *cc, struct cmd *command, uint32_t str_len = strlen(REDIS_STATUS_OK); reply->str = hi_malloc((str_len + 1) * sizeof(char *)); if (reply->str == NULL) { - freeReplyObject(reply); - __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); - return NULL; + goto oom; } reply->len = str_len; @@ -3101,8 +3086,7 @@ void *redisClusterFormattedCommand(redisClusterContext *cc, char *cmd, command = command_get(); if (command == NULL) { - __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); - return NULL; + goto oom; } command->cmd = cmd; @@ -3110,8 +3094,7 @@ void *redisClusterFormattedCommand(redisClusterContext *cc, char *cmd, commands = listCreate(); if (commands == NULL) { - __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); - goto error; + goto oom; } commands->free = listCommandFree; @@ -3254,17 +3237,14 @@ int redisClusterAppendFormattedCommand(redisClusterContext *cc, char *cmd, if (cc->requests == NULL) { cc->requests = listCreate(); if (cc->requests == NULL) { - __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); - goto error; + goto oom; } - cc->requests->free = listCommandFree; } command = command_get(); if (command == NULL) { - __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); - goto error; + goto oom; } command->cmd = cmd; @@ -3272,8 +3252,7 @@ int redisClusterAppendFormattedCommand(redisClusterContext *cc, char *cmd, commands = listCreate(); if (commands == NULL) { - __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); - goto error; + goto oom; } commands->free = listCommandFree; @@ -3352,7 +3331,6 @@ int redisClusterAppendFormattedCommand(redisClusterContext *cc, char *cmd, /* Attention: mybe here we must pop the sub_commands that had append to the nodes. But now we do not handle it. */ - return REDIS_ERR; } @@ -4134,22 +4112,19 @@ int redisClusterAsyncFormattedCommand(redisClusterAsyncContext *acc, command = command_get(); if (command == NULL) { - __redisClusterAsyncSetError(acc, REDIS_ERR_OOM, "Out of memory"); - goto error; + goto oom; } command->cmd = hi_malloc(len * sizeof(*command->cmd)); if (command->cmd == NULL) { - __redisClusterAsyncSetError(acc, REDIS_ERR_OOM, "Out of memory"); - goto error; + goto oom; } memcpy(command->cmd, cmd, len); command->clen = len; commands = listCreate(); if (commands == NULL) { - __redisClusterAsyncSetError(acc, REDIS_ERR_OOM, "Out of memory"); - goto error; + goto oom; } commands->free = listCommandFree; @@ -4194,7 +4169,6 @@ int redisClusterAsyncFormattedCommand(redisClusterAsyncContext *acc, cad = cluster_async_data_get(); if (cad == NULL) { - __redisClusterAsyncSetError(acc, REDIS_ERR_OOM, "Out of memory"); goto error; } @@ -4215,6 +4189,10 @@ int redisClusterAsyncFormattedCommand(redisClusterAsyncContext *acc, return REDIS_OK; +oom: + __redisClusterAsyncSetError(acc, REDIS_ERR_OOM, "Out of memory"); + // passthrough + error: command_destroy(command); From 3d4db9009a5ba6a3e49994e75344d7b5ea2e0d28 Mon Sep 17 00:00:00 2001 From: Bjorn Svensson Date: Fri, 4 Dec 2020 16:23:49 +0100 Subject: [PATCH 09/14] Added initial out-of-memory handling tests The tests are very static and only verifies the handling of the first allocation in the API functions. Next step could be to randomize when an allocation fail to make sure that all allocation failures are handled. --- README.md | 2 +- hircluster.c | 8 +- tests/CMakeLists.txt | 5 + tests/ct_out_of_memory_handling.c | 216 ++++++++++++++++++++++++++++++ tests/test_utils.h | 6 + 5 files changed, 233 insertions(+), 4 deletions(-) create mode 100644 tests/ct_out_of_memory_handling.c diff --git a/README.md b/README.md index a490085..5f6fe84 100644 --- a/README.md +++ b/README.md @@ -15,7 +15,7 @@ Hiredis-cluster is a fork of Hiredis-vip, with the following improvements: * Using CMake as build system * Code style guide (using clang-format) * Improved testing -* Memory leak corrections +* Memory leak corrections and allocation failure handling ## Features diff --git a/hircluster.c b/hircluster.c index b36afd8..5d28f96 100644 --- a/hircluster.c +++ b/hircluster.c @@ -858,7 +858,7 @@ dict *parse_cluster_slots(redisClusterContext *cc, redisReply *reply, master = dictGetEntryVal(den); ret = cluster_slot_ref_node(slot, master); if (ret != REDIS_OK) { - goto oom; + goto error; } slot = NULL; @@ -886,7 +886,7 @@ dict *parse_cluster_slots(redisClusterContext *cc, redisReply *reply, ret = cluster_slot_ref_node(slot, master); if (ret != REDIS_OK) { - goto oom; + goto error; } slot = NULL; @@ -1767,6 +1767,7 @@ int redisClusterSetOptionAddNode(redisClusterContext *cc, const char *addr) { if (cc->nodes == NULL) { cc->nodes = dictCreate(&clusterNodesDictType, NULL); if (cc->nodes == NULL) { + __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); return REDIS_ERR; } } @@ -1955,6 +1956,7 @@ int redisClusterSetOptionConnectTimeout(redisClusterContext *cc, if (cc->connect_timeout == NULL) { cc->connect_timeout = hi_malloc(sizeof(struct timeval)); if (cc->connect_timeout == NULL) { + __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); return REDIS_ERR; } } @@ -4169,7 +4171,7 @@ int redisClusterAsyncFormattedCommand(redisClusterAsyncContext *acc, cad = cluster_async_data_get(); if (cad == NULL) { - goto error; + goto oom; } cad->acc = acc; diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 28a23a4..683f1b5 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -78,6 +78,11 @@ if(ENABLE_IPV6_TESTS) set_tests_properties(ct_connection_ipv6 PROPERTIES LABELS "CT") endif() +add_executable(ct_out_of_memory_handling ct_out_of_memory_handling.c) +target_link_libraries(ct_out_of_memory_handling hiredis_cluster hiredis ${SSL_LIBRARY}) +add_test(NAME ct_out_of_memory_handling COMMAND "$") +set_tests_properties(ct_out_of_memory_handling PROPERTIES LABELS "CT") + if(ENABLE_SSL) # Executable: tls add_executable(example_tls main_tls.c) diff --git a/tests/ct_out_of_memory_handling.c b/tests/ct_out_of_memory_handling.c new file mode 100644 index 0000000..b8b999c --- /dev/null +++ b/tests/ct_out_of_memory_handling.c @@ -0,0 +1,216 @@ +#include "hircluster.h" +#include "test_utils.h" +#include +#include +#include +#include + +#define CLUSTER_NODE "127.0.0.1:7000" + +// A OOM failing malloc() +static void *hi_malloc_fail(size_t size) { + UNUSED(size); + return NULL; +} + +// A OOM failing calloc() +static void *hi_calloc_fail(size_t nmemb, size_t size) { + UNUSED(nmemb); + UNUSED(size); + return NULL; +} + +// A OOM failing realloc() +static void *hi_realloc_fail(void *ptr, size_t size) { + UNUSED(ptr); + UNUSED(size); + return NULL; +} + +// Reset the error string (between tests) +void reset_cluster_errors(redisClusterContext *cc) { + cc->err = 0; + memset(cc->errstr, '\0', strlen(cc->errstr)); +} + +// Test OOM handling in first allocation in: +// redisClusterContextInit(); +void test_context_init() { + hiredisAllocFuncs ha = { + .mallocFn = hi_malloc_fail, + .callocFn = hi_calloc_fail, + .reallocFn = hi_realloc_fail, + .strdupFn = strdup, + .freeFn = free, + }; + + // Override allocators + hiredisSetAllocators(&ha); + { + redisClusterContext *cc = redisClusterContextInit(); + assert(cc == NULL); + } + hiredisResetAllocators(); +} + +// Test OOM handling in first allocation in: +// redisClusterSetOptionXXXX +void test_setoptions() { + hiredisAllocFuncs ha = { + .mallocFn = hi_malloc_fail, + .callocFn = hi_calloc_fail, + .reallocFn = hi_realloc_fail, + .strdupFn = strdup, + .freeFn = free, + }; + + redisClusterContext *cc = redisClusterContextInit(); + assert(cc); + ASSERT_STR_EQ(cc->errstr, ""); + + // Override allocators + hiredisSetAllocators(&ha); + { + int result; + result = redisClusterSetOptionAddNodes(cc, CLUSTER_NODE); + assert(result == REDIS_ERR); + ASSERT_STR_STARTS_WITH(cc->errstr, "servers address is error"); + + reset_cluster_errors(cc); + + result = redisClusterSetOptionAddNode(cc, CLUSTER_NODE); + assert(result == REDIS_ERR); + ASSERT_STR_EQ(cc->errstr, "Out of memory"); + + reset_cluster_errors(cc); + + struct timeval timeout = {0, 500000}; + result = redisClusterSetOptionConnectTimeout(cc, timeout); + assert(result == REDIS_ERR); + ASSERT_STR_EQ(cc->errstr, "Out of memory"); + + reset_cluster_errors(cc); + + result = redisClusterSetOptionTimeout(cc, timeout); + assert(result == REDIS_ERR); + ASSERT_STR_EQ(cc->errstr, "Out of memory"); + } + hiredisResetAllocators(); + + redisClusterFree(cc); +} + +// Test OOM handling in first allocation in: +// redisClusterConnect2() +void test_cluster_connect() { + hiredisAllocFuncs ha = { + .mallocFn = hi_malloc_fail, + .callocFn = hi_calloc_fail, + .reallocFn = hi_realloc_fail, + .strdupFn = strdup, + .freeFn = free, + }; + struct timeval timeout = {0, 500000}; + + redisClusterContext *cc = redisClusterContextInit(); + assert(cc); + redisClusterSetOptionAddNodes(cc, CLUSTER_NODE); + redisClusterSetOptionConnectTimeout(cc, timeout); + ASSERT_STR_EQ(cc->errstr, ""); + + // Override allocators + hiredisSetAllocators(&ha); + { + int result; + result = redisClusterConnect2(cc); + assert(result == REDIS_ERR); + ASSERT_STR_EQ(cc->errstr, "Out of memory"); + } + hiredisResetAllocators(); + + redisClusterFree(cc); +} + +// Test OOM handling in first allocation in: +// redisClusterCommand() +void test_cluster_command() { + hiredisAllocFuncs ha = { + .mallocFn = hi_malloc_fail, + .callocFn = hi_calloc_fail, + .reallocFn = hi_realloc_fail, + .strdupFn = strdup, + .freeFn = free, + }; + + struct timeval timeout = {0, 500000}; + + redisClusterContext *cc = redisClusterContextInit(); + assert(cc); + redisClusterSetOptionAddNodes(cc, CLUSTER_NODE); + redisClusterSetOptionConnectTimeout(cc, timeout); + + int result; + result = redisClusterConnect2(cc); + ASSERT_MSG(result == REDIS_OK, cc->errstr); + ASSERT_STR_EQ(cc->errstr, ""); + + // Override allocators + hiredisSetAllocators(&ha); + { + redisReply *reply; + reply = (redisReply *)redisClusterCommand(cc, "SET key value"); + assert(reply == NULL); + + ASSERT_STR_EQ(cc->errstr, "Out of memory"); + } + hiredisResetAllocators(); + + redisClusterFree(cc); +} + +// Test OOM handling in first allocation in: +// redisClusterAppendCommand() +void test_cluster_append_command() { + hiredisAllocFuncs ha = { + .mallocFn = hi_malloc_fail, + .callocFn = hi_calloc_fail, + .reallocFn = hi_realloc_fail, + .strdupFn = strdup, + .freeFn = free, + }; + + struct timeval timeout = {0, 500000}; + + redisClusterContext *cc = redisClusterContextInit(); + assert(cc); + redisClusterSetOptionAddNodes(cc, CLUSTER_NODE); + redisClusterSetOptionConnectTimeout(cc, timeout); + + int result; + result = redisClusterConnect2(cc); + ASSERT_MSG(result == REDIS_OK, cc->errstr); + ASSERT_STR_EQ(cc->errstr, ""); + + // Override allocators + hiredisSetAllocators(&ha); + { + result = redisClusterAppendCommand(cc, "SET foo one"); + assert(result == REDIS_ERR); + ASSERT_STR_EQ(cc->errstr, "Out of memory"); + } + hiredisResetAllocators(); + + redisClusterFree(cc); +} + +int main() { + // Test the handling of an out-of-memory situation + // in the first allocation done in the API functions. + test_context_init(); + test_setoptions(); + test_cluster_connect(); + test_cluster_command(); + test_cluster_append_command(); + + return 0; +} diff --git a/tests/test_utils.h b/tests/test_utils.h index bbb48ff..654d1c2 100644 --- a/tests/test_utils.h +++ b/tests/test_utils.h @@ -57,4 +57,10 @@ _ctx->errstr); \ } +#define ASSERT_STR_EQ(_s1, _s2) \ + { assert(strcmp(_s1, _s2) == 0); } + +#define ASSERT_STR_STARTS_WITH(_s1, _s2) \ + { assert(strncmp(_s1, _s2, strlen(_s2)) == 0); } + #endif From 43692661dc75a121963a5b6e628d07f39c3703fc Mon Sep 17 00:00:00 2001 From: Bjorn Svensson Date: Sun, 6 Dec 2020 12:57:48 +0100 Subject: [PATCH 10/14] Handle out of memory in command parsing --- command.c | 11 +++++------ hircluster.c | 3 +-- 2 files changed, 6 insertions(+), 8 deletions(-) diff --git a/command.c b/command.c index 79447a0..af8ff7e 100644 --- a/command.c +++ b/command.c @@ -1100,7 +1100,7 @@ void redis_parse_cmd(struct cmd *r) { kpos = hiarray_push(r->keys); if (kpos == NULL) { - goto enomem; + goto oom; } kpos->start = m; kpos->end = p; @@ -1590,15 +1590,12 @@ void redis_parse_cmd(struct cmd *r) { done: ASSERT(r->type > CMD_UNKNOWN && r->type < CMD_SENTINEL); - r->result = CMD_PARSE_OK; - return; -enomem: +oom: r->result = CMD_PARSE_ENOMEM; - return; error: @@ -1607,8 +1604,10 @@ void redis_parse_cmd(struct cmd *r) { errno = EINVAL; if (r->errstr == NULL) { r->errstr = hi_malloc(100 * sizeof(*r->errstr)); - if (r->errstr == NULL) + if (r->errstr == NULL) { + r->result = CMD_PARSE_ENOMEM; return; + } } len = _scnprintf( diff --git a/hircluster.c b/hircluster.c index 5d28f96..dcccf20 100644 --- a/hircluster.c +++ b/hircluster.c @@ -3029,8 +3029,7 @@ static int command_format_by_slot(redisClusterContext *cc, struct cmd *command, redis_parse_cmd(command); if (command->result == CMD_PARSE_ENOMEM) { - __redisClusterSetError(cc, REDIS_ERR_PROTOCOL, - "Parse command error: out of memory"); + __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); goto done; } else if (command->result != CMD_PARSE_OK) { __redisClusterSetError(cc, REDIS_ERR_PROTOCOL, command->errstr); From c324347d77b9e0b660271504376251f9f835ef7b Mon Sep 17 00:00:00 2001 From: Bjorn Svensson Date: Sun, 6 Dec 2020 13:34:42 +0100 Subject: [PATCH 11/14] Cleanups of oom handling where hiarray is involved --- hiarray.c | 15 ------------ hiarray.h | 1 - hircluster.c | 65 +++++++++++++++++----------------------------------- hiutil.h | 1 - 4 files changed, 21 insertions(+), 61 deletions(-) diff --git a/hiarray.c b/hiarray.c index 0cf9583..adac732 100644 --- a/hiarray.c +++ b/hiarray.c @@ -32,21 +32,6 @@ void hiarray_destroy(struct hiarray *a) { hi_free(a); } -int hiarray_init(struct hiarray *a, uint32_t n, size_t size) { - ASSERT(n != 0 && size != 0); - - a->elem = hi_malloc(n * size); - if (a->elem == NULL) { - return HI_ENOMEM; - } - - a->nelem = 0; - a->size = size; - a->nalloc = n; - - return HI_OK; -} - void hiarray_deinit(struct hiarray *a) { ASSERT(a->nelem == 0); diff --git a/hiarray.h b/hiarray.h index 81f2695..8460042 100644 --- a/hiarray.h +++ b/hiarray.h @@ -35,7 +35,6 @@ static inline uint32_t hiarray_n(const struct hiarray *a) { return a->nelem; } struct hiarray *hiarray_create(uint32_t n, size_t size); void hiarray_destroy(struct hiarray *a); -int hiarray_init(struct hiarray *a, uint32_t n, size_t size); void hiarray_deinit(struct hiarray *a); uint32_t hiarray_idx(struct hiarray *a, void *elem); diff --git a/hircluster.c b/hircluster.c index dcccf20..304a6b7 100644 --- a/hircluster.c +++ b/hircluster.c @@ -1089,22 +1089,15 @@ dict *parse_cluster_nodes(redisClusterContext *cc, char *str, int str_len, master->migrating = hiarray_create(1, sizeof(oslot)); if (master->migrating == NULL) { - __redisClusterSetError( - cc, REDIS_ERR_OTHER, - "create migrating array error"); cluster_open_slot_destroy(oslot); - goto error; + goto oom; } } oslot_elem = hiarray_push(master->migrating); if (oslot_elem == NULL) { - __redisClusterSetError( - cc, REDIS_ERR_OTHER, - "Push migrating array error: out of " - "memory"); cluster_open_slot_destroy(oslot); - goto error; + goto oom; } *oslot_elem = oslot; @@ -1124,22 +1117,15 @@ dict *parse_cluster_nodes(redisClusterContext *cc, char *str, int str_len, master->importing = hiarray_create(1, sizeof(oslot)); if (master->importing == NULL) { - __redisClusterSetError( - cc, REDIS_ERR_OTHER, - "create migrating array error"); cluster_open_slot_destroy(oslot); - goto error; + goto oom; } } oslot_elem = hiarray_push(master->importing); if (oslot_elem == NULL) { - __redisClusterSetError( - cc, REDIS_ERR_OTHER, - "push migrating array error: out of " - "memory"); cluster_open_slot_destroy(oslot); - goto error; + goto oom; } *oslot_elem = oslot; @@ -1395,6 +1381,9 @@ static int cluster_update_route_by_addr(redisClusterContext *cc, const char *ip, } slot_elem = hiarray_push(slots); + if (slot_elem == NULL) { + goto oom; + } *slot_elem = slot; } @@ -2587,15 +2576,13 @@ static int command_pre_fragment(redisClusterContext *cc, struct cmd *command, sub_commands = hi_malloc(REDIS_CLUSTER_SLOTS * sizeof(*sub_commands)); if (sub_commands == NULL) { - __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); - goto done; + goto oom; } memset(sub_commands, 0, REDIS_CLUSTER_SLOTS * sizeof(*sub_commands)); command->frag_seq = hi_malloc(key_count * sizeof(*command->frag_seq)); if (command->frag_seq == NULL) { - __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); - goto done; + goto oom; } // Fill sub_command with key, slot and command length (clen, only keylength) @@ -2613,9 +2600,7 @@ static int command_pre_fragment(redisClusterContext *cc, struct cmd *command, if (sub_commands[slot_num] == NULL) { sub_commands[slot_num] = command_get(); if (sub_commands[slot_num] == NULL) { - __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); - slot_num = -1; - goto done; + goto oom; } } @@ -2625,9 +2610,7 @@ static int command_pre_fragment(redisClusterContext *cc, struct cmd *command, sub_kp = hiarray_push(sub_command->keys); if (sub_kp == NULL) { - __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); - slot_num = -1; - goto done; + goto oom; } sub_kp->start = kp->start; @@ -2686,9 +2669,7 @@ static int command_pre_fragment(redisClusterContext *cc, struct cmd *command, sub_command->cmd = hi_malloc(sub_command->clen * sizeof(*sub_command->cmd)); if (sub_command->cmd == NULL) { - __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); - slot_num = -1; - goto done; + goto oom; } memset(sub_command->cmd, 0, sub_command->clen * sizeof(*sub_command->cmd)); @@ -2730,9 +2711,7 @@ static int command_pre_fragment(redisClusterContext *cc, struct cmd *command, sub_command->cmd = hi_malloc(sub_command->clen * sizeof(*sub_command->cmd)); if (sub_command->cmd == NULL) { - __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); - slot_num = -1; - goto done; + goto oom; } memset(sub_command->cmd, 0, sub_command->clen * sizeof(*sub_command->cmd)); @@ -2774,9 +2753,7 @@ static int command_pre_fragment(redisClusterContext *cc, struct cmd *command, sub_command->cmd = hi_malloc(sub_command->clen * sizeof(*sub_command->cmd)); if (sub_command->cmd == NULL) { - __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); - slot_num = -1; - goto done; + goto oom; } memset(sub_command->cmd, 0, sub_command->clen * sizeof(*sub_command->cmd)); @@ -2820,9 +2797,7 @@ static int command_pre_fragment(redisClusterContext *cc, struct cmd *command, sub_command->cmd = hi_malloc(sub_command->clen * sizeof(*sub_command->cmd)); if (sub_command->cmd == NULL) { - __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); - slot_num = -1; - goto done; + goto oom; } memset(sub_command->cmd, 0, sub_command->clen * sizeof(*sub_command->cmd)); @@ -2863,9 +2838,7 @@ static int command_pre_fragment(redisClusterContext *cc, struct cmd *command, done: - if (sub_commands != NULL) { - hi_free(sub_commands); - } + hi_free(sub_commands); if (slot_num >= 0 && commands != NULL && listLength(commands) == 1) { listNode *list_node = listFirst(commands); @@ -2877,8 +2850,12 @@ static int command_pre_fragment(redisClusterContext *cc, struct cmd *command, command->slot_num = slot_num; } - return slot_num; + +oom: + __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); + hi_free(sub_commands); + return -1; // failing slot_num } static void *command_post_fragment(redisClusterContext *cc, struct cmd *command, diff --git a/hiutil.h b/hiutil.h index c4ba6f4..a05b590 100644 --- a/hiutil.h +++ b/hiutil.h @@ -12,7 +12,6 @@ typedef SSIZE_T ssize_t; #define HI_OK 0 #define HI_ERROR -1 #define HI_EAGAIN -2 -#define HI_ENOMEM -3 typedef int rstatus_t; /* return type */ From ed5eb618166bac364710c35ab7ee160ebc8873c8 Mon Sep 17 00:00:00 2001 From: Bjorn Svensson Date: Sun, 6 Dec 2020 21:12:57 +0100 Subject: [PATCH 12/14] Cleanups of OOM in list and dict cases --- adlist.c | 3 + hircluster.c | 164 +++++++++++++++++++++++++++++---------------------- 2 files changed, 96 insertions(+), 71 deletions(-) diff --git a/adlist.c b/adlist.c index 2c0e173..c05c9cd 100644 --- a/adlist.c +++ b/adlist.c @@ -281,6 +281,9 @@ listNode *listSearchKey(hilist *list, void *key) { listNode *node; iter = listGetIterator(list, AL_START_HEAD); + if (iter == NULL) { + return NULL; + } while ((node = listNext(iter)) != NULL) { if (list->match) { if (list->match(node->value, key)) { diff --git a/hircluster.c b/hircluster.c index 304a6b7..daae1b0 100644 --- a/hircluster.c +++ b/hircluster.c @@ -350,7 +350,10 @@ static cluster_slot *cluster_slot_create(cluster_node *node) { node->slots->free = listClusterSlotDestructor; } - listAddNodeTail(node->slots, slot); + if (listAddNodeTail(node->slots, slot) == NULL) { + cluster_slot_destroy(slot); + return NULL; + } } return slot; @@ -374,7 +377,9 @@ static int cluster_slot_ref_node(cluster_slot *slot, cluster_node *node) { node->slots->free = listClusterSlotDestructor; } - listAddNodeTail(node->slots, slot); + if (listAddNodeTail(node->slots, slot) == NULL) { + return REDIS_ERR; + } slot->node = node; return REDIS_OK; @@ -493,8 +498,7 @@ static cluster_node *node_get_with_slots(redisClusterContext *cc, node = hi_malloc(sizeof(cluster_node)); if (node == NULL) { - __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); - goto error; + goto oom; } cluster_node_init(node); @@ -502,11 +506,7 @@ static cluster_node *node_get_with_slots(redisClusterContext *cc, if (role == REDIS_ROLE_MASTER) { node->slots = listCreate(); if (node->slots == NULL) { - hi_free(node); - node = NULL; - __redisClusterSetError(cc, REDIS_ERR_OTHER, - "slots for node listCreate error"); - goto error; + goto oom; } node->slots->free = listClusterSlotDestructor; @@ -522,10 +522,12 @@ static cluster_node *node_get_with_slots(redisClusterContext *cc, return node; -error: +oom: + __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); + // passthrough +error: hi_free(node); - return NULL; } @@ -545,17 +547,14 @@ static cluster_node *node_get_with_nodes(redisClusterContext *cc, node = hi_malloc(sizeof(cluster_node)); if (node == NULL) { - __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); - return NULL; + goto oom; } cluster_node_init(node); if (role == REDIS_ROLE_MASTER) { node->slots = listCreate(); if (node->slots == NULL) { - __redisClusterSetError(cc, REDIS_ERR_OTHER, - "slots for node listCreate error"); - goto error; + goto oom; } node->slots->free = listClusterSlotDestructor; @@ -591,15 +590,20 @@ static cluster_node *node_get_with_nodes(redisClusterContext *cc, return node; -error: +oom: + __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); + // passthrough - if (node->slots) { - listRelease(node->slots); - } - if (node->host) { - sdsfree(node->host); +error: + if (node) { + if (node->slots) { + listRelease(node->slots); + } + if (node->host) { + sdsfree(node->host); + } + hi_free(node); } - hi_free(node); return NULL; } @@ -671,6 +675,9 @@ static int cluster_master_slave_mapping_with_name(redisClusterContext *cc, if (*nodes == NULL) { *nodes = dictCreate(&clusterNodesRefDictType, NULL); + if (*nodes == NULL) { + goto oom; + } } di = dictFind(*nodes, master_name); @@ -678,9 +685,7 @@ static int cluster_master_slave_mapping_with_name(redisClusterContext *cc, ret = dictAdd(*nodes, sdsnewlen(master_name, sdslen(master_name)), node); if (ret != DICT_OK) { - __redisClusterSetError(cc, REDIS_ERR_OTHER, - "the address already exists in the nodes"); - return REDIS_ERR; + goto oom; } } else { @@ -710,16 +715,20 @@ static int cluster_master_slave_mapping_with_name(redisClusterContext *cc, node_old->slaves->free = NULL; while (listLength(node_old->slaves) > 0) { lnode = listFirst(node_old->slaves); - listAddNodeHead(node->slaves, lnode->value); + if (listAddNodeHead(node->slaves, lnode->value) == NULL) { + goto oom; + } listDelNode(node_old->slaves, lnode); } listRelease(node_old->slaves); node_old->slaves = NULL; } - listAddNodeHead(node->slaves, node_old); - + if (listAddNodeHead(node->slaves, node_old) == NULL) { + goto oom; + } dictSetHashVal(*nodes, di, node); + } else if (node->role == REDIS_ROLE_SLAVE) { if (node_old->slaves == NULL) { node_old->slaves = listCreate(); @@ -729,8 +738,10 @@ static int cluster_master_slave_mapping_with_name(redisClusterContext *cc, node_old->slaves->free = listClusterNodeDestructor; } + if (listAddNodeTail(node_old->slaves, node) == NULL) { + goto oom; + } - listAddNodeTail(node_old->slaves, node); } else { NOT_REACHED(); } @@ -851,14 +862,14 @@ dict *parse_cluster_slots(redisClusterContext *cc, redisReply *reply, address = sdscatfmt(address, ":%i", elem_port->integer); den = dictFind(nodes, address); - // master already exits, break to the next slots region. + // master already exists, break to the next slots region. if (den != NULL) { sdsfree(address); master = dictGetEntryVal(den); ret = cluster_slot_ref_node(slot, master); if (ret != REDIS_OK) { - goto error; + goto error; // or possibly OOM } slot = NULL; @@ -876,17 +887,14 @@ dict *parse_cluster_slots(redisClusterContext *cc, redisReply *reply, sdsnewlen(master->addr, sdslen(master->addr)), master); if (ret != DICT_OK) { - __redisClusterSetError( - cc, REDIS_ERR_OTHER, - "The address already exists in the nodes"); cluster_node_deinit(master); hi_free(master); - goto error; + goto oom; } ret = cluster_slot_ref_node(slot, master); if (ret != REDIS_OK) { - goto error; + goto error; // or possibly OOM } slot = NULL; @@ -901,13 +909,18 @@ dict *parse_cluster_slots(redisClusterContext *cc, redisReply *reply, master->slaves = listCreate(); if (master->slaves == NULL) { cluster_node_deinit(slave); + hi_free(slave); goto oom; } master->slaves->free = listClusterNodeDestructor; } - listAddNodeTail(master->slaves, slave); + if (listAddNodeTail(master->slaves, slave) == NULL) { + cluster_node_deinit(slave); + hi_free(slave); + goto oom; + } } } } @@ -1016,6 +1029,7 @@ dict *parse_cluster_nodes(redisClusterContext *cc, char *str, int str_len, sdsnewlen(master->addr, sdslen(master->addr)), master); if (ret != DICT_OK) { + // Key already exists, but possibly an OOM error __redisClusterSetError( cc, REDIS_ERR_OTHER, "The address already exists in the nodes"); @@ -1746,7 +1760,7 @@ int redisClusterSetOptionAddNode(redisClusterContext *cc, const char *addr) { dictEntry *node_entry; cluster_node *node; sds ip; - int port; + int port, ret; sds addr_sds = NULL; if (cc == NULL) { @@ -1756,8 +1770,7 @@ int redisClusterSetOptionAddNode(redisClusterContext *cc, const char *addr) { if (cc->nodes == NULL) { cc->nodes = dictCreate(&clusterNodesDictType, NULL); if (cc->nodes == NULL) { - __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); - return REDIS_ERR; + goto oom; } } @@ -1820,11 +1833,18 @@ int redisClusterSetOptionAddNode(redisClusterContext *cc, const char *addr) { node->host = ip; node->port = port; - - dictAdd(cc->nodes, sdsnewlen(node->addr, sdslen(node->addr)), node); + ret = + dictAdd(cc->nodes, sdsnewlen(node->addr, sdslen(node->addr)), node); + if (ret != DICT_OK) { + goto oom; + } } return REDIS_OK; + +oom: + __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); + return REDIS_ERR; } int redisClusterSetOptionAddNodes(redisClusterContext *cc, const char *addrs) { @@ -2348,7 +2368,7 @@ static int __redisClusterGetReply(redisClusterContext *cc, int slot_num, static cluster_node *node_get_by_ask_error_reply(redisClusterContext *cc, redisReply *reply) { sds *part = NULL, *ip_port = NULL; - int part_len = 0, ip_port_len; + int part_len = 0, ip_port_len, ret; dictEntry *de; cluster_node *node = NULL; @@ -2381,8 +2401,12 @@ static cluster_node *node_get_by_ask_error_reply(redisClusterContext *cc, node->port = hi_atoi(ip_port[1], sdslen(ip_port[1])); node->role = REDIS_ROLE_MASTER; - dictAdd(cc->nodes, sdsnewlen(node->addr, sdslen(node->addr)), - node); + ret = dictAdd(cc->nodes, + sdsnewlen(node->addr, sdslen(node->addr)), node); + if (ret != DICT_OK) { + __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); + goto done; + } part[1] = NULL; ip_port[0] = NULL; @@ -2827,13 +2851,11 @@ static int command_pre_fragment(redisClusterContext *cc, struct cmd *command, NOT_REACHED(); } - // printf("len : %d\n", sub_command->clen); - // print_string_with_length_fix_CRLF(sub_command->cmd, - // sub_command->clen); - sub_command->type = command->type; - listAddNodeTail(commands, sub_command); + if (listAddNodeTail(commands, sub_command) == NULL) { + goto oom; + } } done: @@ -3244,34 +3266,30 @@ int redisClusterAppendFormattedCommand(redisClusterContext *cc, char *cmd, goto error; } - // all keys belong to one slot + // Append command(s) if (listLength(commands) == 0) { - if (__redisClusterAppendCommand(cc, command) == REDIS_OK) { - goto done; - } else { + // All keys belong to one slot + if (__redisClusterAppendCommand(cc, command) != REDIS_OK) { goto error; } - } - - ASSERT(listLength(commands) != 1); + } else { + // Keys belongs to different slots + ASSERT(listLength(commands) != 1); - list_iter = listGetIterator(commands, AL_START_HEAD); - if (list_iter == NULL) { - goto oom; - } + list_iter = listGetIterator(commands, AL_START_HEAD); + if (list_iter == NULL) { + goto oom; + } - while ((list_node = listNext(list_iter)) != NULL) { - sub_command = list_node->value; + while ((list_node = listNext(list_iter)) != NULL) { + sub_command = list_node->value; - if (__redisClusterAppendCommand(cc, sub_command) == REDIS_OK) { - continue; - } else { - goto error; + if (__redisClusterAppendCommand(cc, sub_command) != REDIS_OK) { + goto error; + } } } -done: - if (command->cmd != NULL) { command->cmd = NULL; } else { @@ -3283,12 +3301,16 @@ int redisClusterAppendFormattedCommand(redisClusterContext *cc, char *cmd, command->sub_commands = commands; } else { listRelease(commands); + commands = NULL; } } listReleaseIterator(list_iter); - listAddNodeTail(cc->requests, command); + list_iter = NULL; + if (listAddNodeTail(cc->requests, command) == NULL) { + goto oom; + } return REDIS_OK; oom: From 81a86afc70446f0c8613beb53d152144e1b4730c Mon Sep 17 00:00:00 2001 From: Bjorn Svensson Date: Mon, 7 Dec 2020 11:49:58 +0100 Subject: [PATCH 13/14] Handle allocation failures of sds strings --- command.c | 14 ++-- hircluster.c | 189 ++++++++++++++++++++++++++------------------------- 2 files changed, 103 insertions(+), 100 deletions(-) diff --git a/command.c b/command.c index af8ff7e..8853563 100644 --- a/command.c +++ b/command.c @@ -1588,25 +1588,17 @@ void redis_parse_cmd(struct cmd *r) { return; done: - ASSERT(r->type > CMD_UNKNOWN && r->type < CMD_SENTINEL); r->result = CMD_PARSE_OK; return; -oom: - - r->result = CMD_PARSE_ENOMEM; - return; - error: - r->result = CMD_PARSE_ERROR; errno = EINVAL; if (r->errstr == NULL) { r->errstr = hi_malloc(100 * sizeof(*r->errstr)); if (r->errstr == NULL) { - r->result = CMD_PARSE_ENOMEM; - return; + goto oom; } } @@ -1615,6 +1607,10 @@ void redis_parse_cmd(struct cmd *r) { "Parse command error. Cmd type: %d, state: %d, break position: %d.", r->type, state, (int)(p - r->cmd)); r->errstr[len] = '\0'; + return; + +oom: + r->result = CMD_PARSE_ENOMEM; } struct cmd *command_get() { diff --git a/hircluster.c b/hircluster.c index daae1b0..be4957a 100644 --- a/hircluster.c +++ b/hircluster.c @@ -412,6 +412,10 @@ static copen_slot *cluster_open_slot_create(uint32_t slot_num, int migrate, oslot->migrate = migrate; oslot->node = node; oslot->remote_name = sdsdup(remote_name); + if (oslot->remote_name == NULL) { + hi_free(oslot); + return NULL; + } return oslot; } @@ -420,12 +424,8 @@ static void cluster_open_slot_destroy(copen_slot *oslot) { oslot->slot_num = 0; oslot->migrate = 0; oslot->node = NULL; - - if (oslot->remote_name != NULL) { - sdsfree(oslot->remote_name); - oslot->remote_name = NULL; - } - + sdsfree(oslot->remote_name); + oslot->remote_name = NULL; hi_free(oslot); } @@ -512,11 +512,19 @@ static cluster_node *node_get_with_slots(redisClusterContext *cc, node->slots->free = listClusterSlotDestructor; } - node->name = NULL; node->addr = sdsnewlen(host_elem->str, host_elem->len); + if (node->addr == NULL) { + goto oom; + } node->addr = sdscatfmt(node->addr, ":%i", port_elem->integer); - + if (node->addr == NULL) { + goto oom; + } node->host = sdsnewlen(host_elem->str, host_elem->len); + if (node->host == NULL) { + goto oom; + } + node->name = NULL; node->port = (int)port_elem->integer; node->role = role; @@ -527,7 +535,11 @@ static cluster_node *node_get_with_slots(redisClusterContext *cc, // passthrough error: - hi_free(node); + if (node != NULL) { + sdsfree(node->addr); + sdsfree(node->host); + hi_free(node); + } return NULL; } @@ -539,7 +551,7 @@ static cluster_node *node_get_with_nodes(redisClusterContext *cc, uint8_t role) { char *p = NULL; char *port = NULL; - cluster_node *node; + cluster_node *node = NULL; if (info_count < 8) { return NULL; @@ -573,6 +585,9 @@ static cluster_node *node_get_with_nodes(redisClusterContext *cc, } node->host = sdsnewlen(node_infos[1], p - node_infos[1]); + if (node->host == NULL) { + goto oom; + } p++; // remove found separator character // Strip away cport if given by redis @@ -599,9 +614,7 @@ static cluster_node *node_get_with_nodes(redisClusterContext *cc, if (node->slots) { listRelease(node->slots); } - if (node->host) { - sdsfree(node->host); - } + sdsfree(node->host); hi_free(node); } return NULL; @@ -682,9 +695,13 @@ static int cluster_master_slave_mapping_with_name(redisClusterContext *cc, di = dictFind(*nodes, master_name); if (di == NULL) { - ret = - dictAdd(*nodes, sdsnewlen(master_name, sdslen(master_name)), node); + sds key = sdsnewlen(master_name, sdslen(master_name)); + if (key == NULL) { + goto oom; + } + ret = dictAdd(*nodes, key, node); if (ret != DICT_OK) { + sdsfree(key); goto oom; } @@ -768,7 +785,6 @@ dict *parse_cluster_slots(redisClusterContext *cc, redisReply *reply, redisReply *elem_nodes; redisReply *elem_ip, *elem_port; cluster_node *master = NULL, *slave; - sds address; uint32_t i, idx; if (reply == NULL) { @@ -858,13 +874,19 @@ dict *parse_cluster_slots(redisClusterContext *cc, redisReply *reply, // this is master. if (idx == 2) { - address = sdsnewlen(elem_ip->str, elem_ip->len); + sds address = sdsnewlen(elem_ip->str, elem_ip->len); + if (address == NULL) { + goto oom; + } address = sdscatfmt(address, ":%i", elem_port->integer); + if (address == NULL) { + goto oom; + } den = dictFind(nodes, address); + sdsfree(address); // master already exists, break to the next slots region. if (den != NULL) { - sdsfree(address); master = dictGetEntryVal(den); ret = cluster_slot_ref_node(slot, master); @@ -876,17 +898,20 @@ dict *parse_cluster_slots(redisClusterContext *cc, redisReply *reply, break; } - sdsfree(address); master = node_get_with_slots(cc, elem_ip, elem_port, REDIS_ROLE_MASTER); if (master == NULL) { goto error; } - ret = dictAdd(nodes, - sdsnewlen(master->addr, sdslen(master->addr)), - master); + sds key = sdsnewlen(master->addr, sdslen(master->addr)); + if (key == NULL) { + goto oom; + } + + ret = dictAdd(nodes, key, master); if (ret != DICT_OK) { + sdsfree(key); cluster_node_deinit(master); hi_free(master); goto oom; @@ -933,15 +958,12 @@ dict *parse_cluster_slots(redisClusterContext *cc, redisReply *reply, // passthrough error: - if (nodes != NULL) { dictRelease(nodes); } - if (slot != NULL) { cluster_slot_destroy(slot); } - return NULL; } @@ -1025,14 +1047,18 @@ dict *parse_cluster_nodes(redisClusterContext *cc, char *str, int str_len, goto error; } - ret = dictAdd(nodes, - sdsnewlen(master->addr, sdslen(master->addr)), - master); + sds key = sdsnewlen(master->addr, sdslen(master->addr)); + if (key == NULL) { + goto oom; + } + + ret = dictAdd(nodes, key, master); if (ret != DICT_OK) { // Key already exists, but possibly an OOM error __redisClusterSetError( cc, REDIS_ERR_OTHER, "The address already exists in the nodes"); + sdsfree(key); cluster_node_deinit(master); hi_free(master); goto error; @@ -1216,27 +1242,14 @@ dict *parse_cluster_nodes(redisClusterContext *cc, char *str, int str_len, // passthrough error: - - if (part != NULL) { - sdsfreesplitres(part, count_part); - count_part = 0; - part = NULL; - } - - if (slot_start_end != NULL) { - sdsfreesplitres(slot_start_end, count_slot_start_end); - count_slot_start_end = 0; - slot_start_end = NULL; - } - + sdsfreesplitres(part, count_part); + sdsfreesplitres(slot_start_end, count_slot_start_end); if (nodes != NULL) { dictRelease(nodes); } - if (nodes_name != NULL) { dictRelease(nodes_name); } - return NULL; } @@ -1450,10 +1463,8 @@ static int cluster_update_route_by_addr(redisClusterContext *cc, const char *ip, // passthrough error: - dictReleaseIterator(dit); listReleaseIterator(lit); - if (slots != NULL) { if (slots == cc->slots) { cc->slots = NULL; @@ -1462,17 +1473,14 @@ static int cluster_update_route_by_addr(redisClusterContext *cc, const char *ip, slots->nelem = 0; hiarray_destroy(slots); } - if (nodes != NULL) { if (nodes == cc->nodes) { cc->nodes = NULL; } dictRelease(nodes); } - freeReplyObject(reply); redisFree(c); - return REDIS_ERR; } @@ -1758,10 +1766,9 @@ redisClusterContext *redisClusterConnectNonBlock(const char *addrs, int flags) { int redisClusterSetOptionAddNode(redisClusterContext *cc, const char *addr) { dictEntry *node_entry; - cluster_node *node; - sds ip; + cluster_node *node = NULL; int port, ret; - sds addr_sds = NULL; + sds ip = NULL; if (cc == NULL) { return REDIS_ERR; @@ -1774,7 +1781,10 @@ int redisClusterSetOptionAddNode(redisClusterContext *cc, const char *addr) { } } - addr_sds = sdsnew(addr); + sds addr_sds = sdsnew(addr); + if (addr_sds == NULL) { + goto oom; + } node_entry = dictFind(cc->nodes, addr_sds); sdsfree(addr_sds); if (node_entry == NULL) { @@ -1794,8 +1804,11 @@ int redisClusterSetOptionAddNode(redisClusterContext *cc, const char *addr) { "server address is incorrect, address part missing."); return REDIS_ERR; } - ip = sdsnewlen(addr, p - addr); + ip = sdsnewlen(addr, p - addr); + if (ip == NULL) { + goto oom; + } p++; // remove separator character if (strlen(p) <= 0) { @@ -1814,28 +1827,26 @@ int redisClusterSetOptionAddNode(redisClusterContext *cc, const char *addr) { node = hi_malloc(sizeof(cluster_node)); if (node == NULL) { - sdsfree(ip); - __redisClusterSetError(cc, REDIS_ERR_OTHER, - "alloc cluster node error"); - return REDIS_ERR; + goto oom; } cluster_node_init(node); node->addr = sdsnew(addr); if (node->addr == NULL) { - sdsfree(ip); - hi_free(node); - __redisClusterSetError(cc, REDIS_ERR_OTHER, - "new node address error"); - return REDIS_ERR; + goto oom; } node->host = ip; node->port = port; - ret = - dictAdd(cc->nodes, sdsnewlen(node->addr, sdslen(node->addr)), node); + + sds key = sdsnewlen(node->addr, sdslen(node->addr)); + if (key == NULL) { + goto oom; + } + ret = dictAdd(cc->nodes, key, node); if (ret != DICT_OK) { + sdsfree(key); goto oom; } } @@ -1844,6 +1855,11 @@ int redisClusterSetOptionAddNode(redisClusterContext *cc, const char *addr) { oom: __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); + sdsfree(ip); + if (node != NULL) { + sdsfree(node->addr); + hi_free(node); + } return REDIS_ERR; } @@ -2368,7 +2384,7 @@ static int __redisClusterGetReply(redisClusterContext *cc, int slot_num, static cluster_node *node_get_by_ask_error_reply(redisClusterContext *cc, redisReply *reply) { sds *part = NULL, *ip_port = NULL; - int part_len = 0, ip_port_len, ret; + int part_len = 0, ip_port_len = 0, ret; dictEntry *de; cluster_node *node = NULL; @@ -2391,8 +2407,7 @@ static cluster_node *node_get_by_ask_error_reply(redisClusterContext *cc, if (de == NULL) { node = hi_malloc(sizeof(cluster_node)); if (node == NULL) { - __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); - goto done; + goto oom; } cluster_node_init(node); @@ -2401,18 +2416,21 @@ static cluster_node *node_get_by_ask_error_reply(redisClusterContext *cc, node->port = hi_atoi(ip_port[1], sdslen(ip_port[1])); node->role = REDIS_ROLE_MASTER; - ret = dictAdd(cc->nodes, - sdsnewlen(node->addr, sdslen(node->addr)), node); + sds key = sdsnewlen(node->addr, sdslen(node->addr)); + if (key == NULL) { + goto oom; + } + + ret = dictAdd(cc->nodes, key, node); if (ret != DICT_OK) { - __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); - goto done; + sdsfree(key); + goto oom; } part[1] = NULL; ip_port[0] = NULL; } else { node = de->val; - goto done; } } else { @@ -2430,18 +2448,15 @@ static cluster_node *node_get_by_ask_error_reply(redisClusterContext *cc, } done: - - if (part != NULL) { - sdsfreesplitres(part, part_len); - part = NULL; - } - - if (ip_port != NULL) { - sdsfreesplitres(ip_port, ip_port_len); - ip_port = NULL; - } - + sdsfreesplitres(part, part_len); + sdsfreesplitres(ip_port, ip_port_len); return node; + +oom: + __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); + sdsfreesplitres(part, part_len); + sdsfreesplitres(ip_port, ip_port_len); + return NULL; } static void *redis_cluster_command_execute(redisClusterContext *cc, @@ -2859,7 +2874,6 @@ static int command_pre_fragment(redisClusterContext *cc, struct cmd *command, } done: - hi_free(sub_commands); if (slot_num >= 0 && commands != NULL && listLength(commands) == 1) { @@ -3154,16 +3168,13 @@ void *redisClusterFormattedCommand(redisClusterContext *cc, char *cmd, // passthrough error: - if (command != NULL) { command->cmd = NULL; command_destroy(command); } - if (commands != NULL) { listRelease(commands); } - listReleaseIterator(list_iter); cc->retry_count = 0; return NULL; @@ -3318,7 +3329,6 @@ int redisClusterAppendFormattedCommand(redisClusterContext *cc, char *cmd, // passthrough error: - if (command != NULL) { command->cmd = NULL; command_destroy(command); @@ -4194,13 +4204,10 @@ int redisClusterAsyncFormattedCommand(redisClusterAsyncContext *acc, // passthrough error: - command_destroy(command); - if (commands != NULL) { listRelease(commands); } - return REDIS_ERR; } From afc84ed97744599e4a4ef51f2c4b8779e50e620e Mon Sep 17 00:00:00 2001 From: Bjorn Svensson Date: Mon, 7 Dec 2020 15:14:28 +0100 Subject: [PATCH 14/14] Improve allocation handling testcase Also found some issues during the testcases regarding OOM handling, like a leak during OOM. Also fixed the order of setting timeout when reconnecting. --- hircluster.c | 74 ++++++---- tests/ct_out_of_memory_handling.c | 236 +++++++++++------------------- 2 files changed, 136 insertions(+), 174 deletions(-) diff --git a/hircluster.c b/hircluster.c index be4957a..0c954ed 100644 --- a/hircluster.c +++ b/hircluster.c @@ -891,7 +891,7 @@ dict *parse_cluster_slots(redisClusterContext *cc, redisReply *reply, master = dictGetEntryVal(den); ret = cluster_slot_ref_node(slot, master); if (ret != REDIS_OK) { - goto error; // or possibly OOM + goto oom; } slot = NULL; @@ -906,6 +906,8 @@ dict *parse_cluster_slots(redisClusterContext *cc, redisReply *reply, sds key = sdsnewlen(master->addr, sdslen(master->addr)); if (key == NULL) { + cluster_node_deinit(master); + hi_free(master); goto oom; } @@ -919,7 +921,7 @@ dict *parse_cluster_slots(redisClusterContext *cc, redisReply *reply, ret = cluster_slot_ref_node(slot, master); if (ret != REDIS_OK) { - goto error; // or possibly OOM + goto oom; } slot = NULL; @@ -1003,8 +1005,11 @@ dict *parse_cluster_nodes(redisClusterContext *cc, char *str, int str_len, len = line_end - line_start; part = sdssplitlen(line_start, len + 1, " ", 1, &count_part); + if (part == NULL) { + goto oom; + } - if (part == NULL || count_part < 8) { + if (count_part < 8) { __redisClusterSetError(cc, REDIS_ERR_OTHER, "split cluster nodes error"); goto error; @@ -1049,6 +1054,8 @@ dict *parse_cluster_nodes(redisClusterContext *cc, char *str, int str_len, sds key = sdsnewlen(master->addr, sdslen(master->addr)); if (key == NULL) { + cluster_node_deinit(master); + hi_free(master); goto oom; } @@ -1080,13 +1087,11 @@ dict *parse_cluster_nodes(redisClusterContext *cc, char *str, int str_len, for (k = 8; k < count_part; k++) { slot_start_end = sdssplitlen(part[k], sdslen(part[k]), "-", 1, &count_slot_start_end); - if (slot_start_end == NULL) { - __redisClusterSetError( - cc, REDIS_ERR_OTHER, - "split slot start end error(NULL)"); - goto error; - } else if (count_slot_start_end == 1) { + goto oom; + } + + if (count_slot_start_end == 1) { slot_start = hi_atoi(slot_start_end[0], sdslen(slot_start_end[0])); slot_end = slot_start; @@ -1287,10 +1292,9 @@ static int cluster_update_route_by_addr(redisClusterContext *cc, const char *ip, } if (c == NULL) { - __redisClusterSetError(cc, REDIS_ERR_OTHER, - "Init redis context error(return NULL)"); - goto error; - } else if (c->err) { + goto oom; + } + if (c->err) { __redisClusterSetError(cc, c->err, c->errstr); goto error; } @@ -1875,7 +1879,12 @@ int redisClusterSetOptionAddNodes(redisClusterContext *cc, const char *addrs) { address = sdssplitlen(addrs, strlen(addrs), CLUSTER_ADDRESS_SEPARATOR, strlen(CLUSTER_ADDRESS_SEPARATOR), &address_count); - if (address == NULL || address_count <= 0) { + if (address == NULL) { + __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); + return REDIS_ERR; + } + + if (address_count <= 0) { __redisClusterSetError(cc, REDIS_ERR_OTHER, "servers address is error(correct is like: " "127.0.0.1:1234,127.0.0.2:5678)"); @@ -2109,11 +2118,11 @@ redisContext *ctx_get_by_node(redisClusterContext *cc, cluster_node *node) { } } #endif - authenticate(cc, c); // err and errstr handled in function - if (cc->command_timeout && c->err == 0) { redisSetTimeout(c, *cc->command_timeout); } + + authenticate(cc, c); // err and errstr handled in function } return c; @@ -2130,7 +2139,18 @@ redisContext *ctx_get_by_node(redisClusterContext *cc, cluster_node *node) { c = redisConnect(node->host, node->port); } - if (cc->command_timeout && c != NULL && c->err == 0) { + if (c == NULL) { + __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); + return NULL; + } + + if (c->err) { + __redisClusterSetError(cc, c->err, c->errstr); + redisFree(c); + return NULL; + } + + if (cc->command_timeout) { redisSetTimeout(c, *cc->command_timeout); } @@ -2237,6 +2257,9 @@ static char *cluster_config_get(redisClusterContext *cc, } c = ctx_get_by_node(cc, node); + if (c == NULL) { + goto error; + } reply = redisCommand(c, "config get %s", config_name); if (reply == NULL) { @@ -2319,7 +2342,6 @@ static int __redisClusterAppendCommand(redisClusterContext *cc, c = ctx_get_by_node(cc, node); if (c == NULL) { - __redisClusterSetError(cc, REDIS_ERR_OTHER, "ctx get by node is null"); return REDIS_ERR; } else if (c->err) { __redisClusterSetError(cc, c->err, c->errstr); @@ -2355,7 +2377,6 @@ static int __redisClusterGetReply(redisClusterContext *cc, int slot_num, c = ctx_get_by_node(cc, node); if (c == NULL) { - __redisClusterSetError(cc, REDIS_ERR_OOM, "Out of memory"); return REDIS_ERR; } else if (c->err) { if (cc->need_update_route == 0) { @@ -2398,11 +2419,17 @@ static cluster_node *node_get_by_ask_error_reply(redisClusterContext *cc, } part = sdssplitlen(reply->str, reply->len, " ", 1, &part_len); + if (part == NULL) { + goto oom; + } - if (part != NULL && part_len == 3) { + if (part_len == 3) { ip_port = sdssplitlen(part[2], sdslen(part[2]), ":", 1, &ip_port_len); + if (ip_port == NULL) { + goto oom; + } - if (ip_port != NULL && ip_port_len == 2) { + if (ip_port_len == 2) { de = dictFind(cc->nodes, part[2]); if (de == NULL) { node = hi_malloc(sizeof(cluster_node)); @@ -2477,7 +2504,6 @@ static void *redis_cluster_command_execute(redisClusterContext *cc, c = ctx_get_by_node(cc, node); if (c == NULL) { - __redisClusterSetError(cc, REDIS_ERR_OTHER, "ctx get by node is null"); return NULL; } else if (c->err) { node = node_get_which_connected(cc); @@ -2496,8 +2522,6 @@ static void *redis_cluster_command_execute(redisClusterContext *cc, c = ctx_get_by_node(cc, node); if (c == NULL) { - __redisClusterSetError(cc, REDIS_ERR_OTHER, - "ctx get by node error"); return NULL; } else if (c->err) { __redisClusterSetError(cc, c->err, c->errstr); @@ -2556,8 +2580,6 @@ static void *redis_cluster_command_execute(redisClusterContext *cc, c = ctx_get_by_node(cc, node); if (c == NULL) { - __redisClusterSetError(cc, REDIS_ERR_OTHER, - "ctx get by node error"); return NULL; } else if (c->err) { __redisClusterSetError(cc, c->err, c->errstr); diff --git a/tests/ct_out_of_memory_handling.c b/tests/ct_out_of_memory_handling.c index b8b999c..0a996f3 100644 --- a/tests/ct_out_of_memory_handling.c +++ b/tests/ct_out_of_memory_handling.c @@ -7,35 +7,49 @@ #define CLUSTER_NODE "127.0.0.1:7000" -// A OOM failing malloc() +int successfulAllocations = 0; + +// A configurable OOM failing malloc() static void *hi_malloc_fail(size_t size) { - UNUSED(size); + if (successfulAllocations > 0) { + --successfulAllocations; + return malloc(size); + } return NULL; } -// A OOM failing calloc() +// A configurable OOM failing calloc() static void *hi_calloc_fail(size_t nmemb, size_t size) { - UNUSED(nmemb); - UNUSED(size); + if (successfulAllocations > 0) { + --successfulAllocations; + return calloc(nmemb, size); + } return NULL; } -// A OOM failing realloc() +// A configurable OOM failing realloc() static void *hi_realloc_fail(void *ptr, size_t size) { - UNUSED(ptr); - UNUSED(size); + if (successfulAllocations > 0) { + --successfulAllocations; + return realloc(ptr, size); + } return NULL; } -// Reset the error string (between tests) -void reset_cluster_errors(redisClusterContext *cc) { +void prepare_allocation_test(redisClusterContext *cc, + int _successfulAllocations) { + successfulAllocations = _successfulAllocations; cc->err = 0; memset(cc->errstr, '\0', strlen(cc->errstr)); } -// Test OOM handling in first allocation in: -// redisClusterContextInit(); -void test_context_init() { +// Test of allocation handling +// The testcase will trigger allocation failures during API calls. +// It will start by triggering an allocation fault, and the next iteration +// will start with an successfull allocation and then a failing one, +// next iteration 2 successful and one failing allocation, and so on.. +void test_cluster_communication() { + int result; hiredisAllocFuncs ha = { .mallocFn = hi_malloc_fail, .callocFn = hi_calloc_fail, @@ -43,174 +57,100 @@ void test_context_init() { .strdupFn = strdup, .freeFn = free, }; - // Override allocators hiredisSetAllocators(&ha); + + // Context init + redisClusterContext *cc; { - redisClusterContext *cc = redisClusterContextInit(); + successfulAllocations = 0; + cc = redisClusterContextInit(); assert(cc == NULL); - } - hiredisResetAllocators(); -} -// Test OOM handling in first allocation in: -// redisClusterSetOptionXXXX -void test_setoptions() { - hiredisAllocFuncs ha = { - .mallocFn = hi_malloc_fail, - .callocFn = hi_calloc_fail, - .reallocFn = hi_realloc_fail, - .strdupFn = strdup, - .freeFn = free, - }; - - redisClusterContext *cc = redisClusterContextInit(); - assert(cc); - ASSERT_STR_EQ(cc->errstr, ""); + successfulAllocations = 1; + cc = redisClusterContextInit(); + assert(cc); + } - // Override allocators - hiredisSetAllocators(&ha); + // Add nodes { - int result; + for (int i = 0; i < 9; ++i) { + prepare_allocation_test(cc, i); + result = redisClusterSetOptionAddNodes(cc, CLUSTER_NODE); + assert(result == REDIS_ERR); + ASSERT_STR_EQ(cc->errstr, "Out of memory"); + } + + prepare_allocation_test(cc, 9); result = redisClusterSetOptionAddNodes(cc, CLUSTER_NODE); - assert(result == REDIS_ERR); - ASSERT_STR_STARTS_WITH(cc->errstr, "servers address is error"); - - reset_cluster_errors(cc); - - result = redisClusterSetOptionAddNode(cc, CLUSTER_NODE); - assert(result == REDIS_ERR); - ASSERT_STR_EQ(cc->errstr, "Out of memory"); - - reset_cluster_errors(cc); + assert(result == REDIS_OK); + } + // Set timeout + { struct timeval timeout = {0, 500000}; + + prepare_allocation_test(cc, 0); result = redisClusterSetOptionConnectTimeout(cc, timeout); assert(result == REDIS_ERR); ASSERT_STR_EQ(cc->errstr, "Out of memory"); - reset_cluster_errors(cc); - - result = redisClusterSetOptionTimeout(cc, timeout); - assert(result == REDIS_ERR); - ASSERT_STR_EQ(cc->errstr, "Out of memory"); + prepare_allocation_test(cc, 1); + result = redisClusterSetOptionConnectTimeout(cc, timeout); + assert(result == REDIS_OK); } - hiredisResetAllocators(); - - redisClusterFree(cc); -} - -// Test OOM handling in first allocation in: -// redisClusterConnect2() -void test_cluster_connect() { - hiredisAllocFuncs ha = { - .mallocFn = hi_malloc_fail, - .callocFn = hi_calloc_fail, - .reallocFn = hi_realloc_fail, - .strdupFn = strdup, - .freeFn = free, - }; - struct timeval timeout = {0, 500000}; - redisClusterContext *cc = redisClusterContextInit(); - assert(cc); - redisClusterSetOptionAddNodes(cc, CLUSTER_NODE); - redisClusterSetOptionConnectTimeout(cc, timeout); - ASSERT_STR_EQ(cc->errstr, ""); - - // Override allocators - hiredisSetAllocators(&ha); + // Connect { - int result; + for (int i = 0; i < 133; ++i) { + prepare_allocation_test(cc, i); + result = redisClusterConnect2(cc); + assert(result == REDIS_ERR); + } + + prepare_allocation_test(cc, 133); result = redisClusterConnect2(cc); - assert(result == REDIS_ERR); - ASSERT_STR_EQ(cc->errstr, "Out of memory"); + assert(result == REDIS_OK); } - hiredisResetAllocators(); - - redisClusterFree(cc); -} - -// Test OOM handling in first allocation in: -// redisClusterCommand() -void test_cluster_command() { - hiredisAllocFuncs ha = { - .mallocFn = hi_malloc_fail, - .callocFn = hi_calloc_fail, - .reallocFn = hi_realloc_fail, - .strdupFn = strdup, - .freeFn = free, - }; - - struct timeval timeout = {0, 500000}; - - redisClusterContext *cc = redisClusterContextInit(); - assert(cc); - redisClusterSetOptionAddNodes(cc, CLUSTER_NODE); - redisClusterSetOptionConnectTimeout(cc, timeout); - - int result; - result = redisClusterConnect2(cc); - ASSERT_MSG(result == REDIS_OK, cc->errstr); - ASSERT_STR_EQ(cc->errstr, ""); - // Override allocators - hiredisSetAllocators(&ha); + // Command { redisReply *reply; - reply = (redisReply *)redisClusterCommand(cc, "SET key value"); - assert(reply == NULL); - - ASSERT_STR_EQ(cc->errstr, "Out of memory"); - } - hiredisResetAllocators(); - - redisClusterFree(cc); -} - -// Test OOM handling in first allocation in: -// redisClusterAppendCommand() -void test_cluster_append_command() { - hiredisAllocFuncs ha = { - .mallocFn = hi_malloc_fail, - .callocFn = hi_calloc_fail, - .reallocFn = hi_realloc_fail, - .strdupFn = strdup, - .freeFn = free, - }; - - struct timeval timeout = {0, 500000}; - redisClusterContext *cc = redisClusterContextInit(); - assert(cc); - redisClusterSetOptionAddNodes(cc, CLUSTER_NODE); - redisClusterSetOptionConnectTimeout(cc, timeout); + for (int i = 0; i < 36; ++i) { + prepare_allocation_test(cc, 0 + i); + reply = (redisReply *)redisClusterCommand(cc, "SET key value"); + assert(reply == NULL); + ASSERT_STR_EQ(cc->errstr, "Out of memory"); + } - int result; - result = redisClusterConnect2(cc); - ASSERT_MSG(result == REDIS_OK, cc->errstr); - ASSERT_STR_EQ(cc->errstr, ""); + prepare_allocation_test(cc, 36); + reply = (redisReply *)redisClusterCommand(cc, "SET key value"); + CHECK_REPLY_OK(cc, reply); + freeReplyObject(reply); + } - // Override allocators - hiredisSetAllocators(&ha); + // Append command { + for (int i = 0; i < 33; ++i) { + prepare_allocation_test(cc, 0 + i); + result = redisClusterAppendCommand(cc, "SET foo one"); + assert(result == REDIS_ERR); + ASSERT_STR_EQ(cc->errstr, "Out of memory"); + } + + prepare_allocation_test(cc, 33); result = redisClusterAppendCommand(cc, "SET foo one"); - assert(result == REDIS_ERR); - ASSERT_STR_EQ(cc->errstr, "Out of memory"); + assert(result == REDIS_OK); } - hiredisResetAllocators(); redisClusterFree(cc); + hiredisResetAllocators(); } int main() { - // Test the handling of an out-of-memory situation - // in the first allocation done in the API functions. - test_context_init(); - test_setoptions(); - test_cluster_connect(); - test_cluster_command(); - test_cluster_append_command(); + + test_cluster_communication(); return 0; }