Safe allocations #15

bjosv · 2020-12-03T23:01:43Z

This PR handles vipshop/hiredis-vip#136 and the vulnerability report regarding handling failing allocations, see https://nvd.nist.gov/vuln/detail/CVE-2020-7105

Allocation results are now checked before the memory is used.
This PR also replaces existing allocation/free macros to use the same api as hiredis

Some details:
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)

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.

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

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

There is no usage of strdup(), and therefor no need for hi_strdup()

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

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.

zuiderkwast

Awesome!

zuiderkwast · 2020-12-03T23:29:23Z

dict.c

-    free(ht->table);
+    hi_free(ht->table);
+    ht->table = NULL;
+
    /* Re-initialize the table */
    _dictReset(ht);


I don't think ht->table = NULL is necessary since _dictReset() does that, but if you think it's clearer style to do it just after hi_free(), we can keep it.

Yes, good, can remove, always nice with code with a reason. This file actually also exists in hiredis, and without the NULL ine aswell.. I change this
Maybe we should use dict.c from hiredis instead..but that could be a future change

zuiderkwast · 2020-12-04T01:21:42Z

Btw, can we add the CVE under Security? I've seen it there in other Github projects.

zuiderkwast · 2020-12-04T09:08:57Z

Please add "allocation failure handling" in the README, perhaps together with leak corrections. I seems like an important point.

The iterators performs a malloc under the hood that might fail due to out of memory. This avoids using a null iterator.

Making sure to set the error string when there is an out of memory and cleanup the temporary allocated data.

bjosv · 2020-12-04T15:40:46Z

tests/ct_out_of_memory_handling.c

+    return NULL;
+}
+
+// A OOM failing realloc()calloc


will delete "calloc"

bjosv · 2020-12-04T16:29:10Z

hircluster.c

@@ -4146,7 +4171,6 @@ int redisClusterAsyncFormattedCommand(redisClusterAsyncContext *acc,

    cad = cluster_async_data_get();
    if (cad == NULL) {
-        __redisClusterAsyncSetError(acc, REDIS_ERR_OOM, "Out of memory");
        goto error;


Will change this back to OOM, cluster_async_data_get() return NULL when its malloc() fails
goto error; will change to goto oom;

Good. That's not a very good name of that function. It just allocates and empties a struct... But let's not refactor more than necessary in the same PR.

bjosv · 2020-12-04T16:35:31Z

hircluster.c

-                                cc, REDIS_ERR_OOM,
-                                "Slot ref node failed: out of memory.");
-                            goto error;
+                            goto oom;


"cluster_slot_ref_node() is not doing any allocations, and oom is checked on arguments before.
Will change back to goto error;"

Wrong, it does allocations, like when using listAddNodeTail.
Seem very unlikely to have wrong role aswell, going for oom as we started with.

bjosv · 2020-12-04T16:35:49Z

hircluster.c

-                        __redisClusterSetError(
-                            cc, REDIS_ERR_OOM,
-                            "Slot ref node failed: out of memory.");
-                        goto error;


cluster_slot_ref_node() is not doing any allocations, and oom is checked on arguments before.
Will change back to goto error;

I was wrong, it does allocations, like when using listAddNodeTail.
Seem very unlikely to have wrong role aswell, going for oom as we started with.

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.

zuiderkwast

It looks very good. Sometimes the commit messages could contain more details or so. It's hard to verify what's happening without a lot of grepping (which I assume you have done) and probably we need to extend the tests as you noted.

zuiderkwast · 2020-12-07T10:02:58Z

hircluster.c

@@ -4146,7 +4171,6 @@ int redisClusterAsyncFormattedCommand(redisClusterAsyncContext *acc,

    cad = cluster_async_data_get();
    if (cad == NULL) {
-        __redisClusterAsyncSetError(acc, REDIS_ERR_OOM, "Out of memory");
        goto error;


Good. That's not a very good name of that function. It just allocates and empties a struct... But let's not refactor more than necessary in the same PR.

zuiderkwast · 2020-12-07T10:09:00Z

hircluster.c

                        ret = cluster_slot_ref_node(slot, master);
                        if (ret != REDIS_OK) {
-                            goto oom;
+                            goto error;
                        }


Are you sure? It returns REDIS_ERR if listCreate() returns NULL (OOM?) or if slot or master is NULL (not OOM, but shouldn't happen?).

It was a bit hard to follow this, the thing that I hesitate on was if master node always for certain was a master, so I went on error. The difference is the errstr

cluster_slot_ref_node()
..
if (node->role != REDIS_ROLE_MASTER) {
return REDIS_ERR;
}

zuiderkwast · 2020-12-07T10:36:35Z

tests/ct_out_of_memory_handling.c

+// A OOM failing malloc()
+static void *hi_malloc_fail(size_t size) {
+    UNUSED(size);
+    return NULL;
+}


This could be extended with 2 global variables: a counter and a setting fail_on_nth_allocation, so we can test failing on the Nth allocation. The counter can be common for all the allocator functions and the test can loop over N. Is there a way to get code coverage so that we can know if every OOM if statement is covered?

Another option is of course to randomize the failures, but it might be more difficult to write predictable test cases that way.

Yes, nice. I have done this, and fixed a few things while running. Will push the commit

zuiderkwast · 2020-12-07T10:40:52Z

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) {


Perhaps mention in the commit message that we use hiarray function from hiredis? (if that's what we do)

I seems that hiarray.c was copied from hiredis, and this was an unused function that I removed.
hiarray_create() contains what it needs

zuiderkwast · 2020-12-07T10:56:10Z

hircluster.c

                    }

                    ret = cluster_slot_ref_node(slot, master);
                    if (ret != REDIS_OK) {
-                        goto error;
+                        goto error; // or possibly OOM


In a review comment on a previous commit in this PR, you wrote "cluster_slot_ref_node() is not doing any allocations, and oom is checked on arguments before." here. Better write this in a comment here instead of "or possibly OOM".

Yes, cluster_slot_ref_node is doing allocations, but also checking if master has correct role. So its not obvious if role always has REDIS_ROLE_MASTER. Ill improve the comment.

zuiderkwast · 2020-12-07T11:00:11Z

hircluster.c

                if (ret != DICT_OK) {
+                    // Key already exists, but possibly an OOM error


Is there no way to tell them apart? Can't we make dictAdd() return specific error codes such as DICT_OOM?

its possible, would have to make sure the internals in dict handles it correctly for every case. Can do it in another pr

zuiderkwast

Great!

Also found some issues during the testcases regarding OOM handling, like a leak during OOM. Also fixed the order of setting timeout when reconnecting.

bjosv · 2020-12-07T15:27:30Z

Thats it.
I think we can wait with the DICT_OOM since it affects internal handling in dict, then since dict is a copy from hiredis we will diverge. Currently its very similar. The current tests make me quite certain that his is behaving as expected.

zuiderkwast

Very nice.

Should we use dict and array from hiredis in another PR?

bjosv · 2020-12-07T16:08:00Z

That would be nice

bjosv added 5 commits December 3, 2020 21:57

Remove unused string duplication macro and define

e6dcd60

There is no usage of strdup(), and therefor no need for hi_strdup()

bjosv requested a review from zuiderkwast December 3, 2020 23:01

Remove hi_zalloc()

36dec32

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.

bjosv force-pushed the safe_allocations branch from 1eac88e to 36dec32 Compare December 3, 2020 23:03

zuiderkwast approved these changes Dec 4, 2020

View reviewed changes

bjosv added 2 commits December 4, 2020 15:35

Handle OOM in creations of list and dict iterators

c2857ad

The iterators performs a malloc under the hood that might fail due to out of memory. This avoids using a null iterator.

Handle oom commonly in each function

e42ab36

Making sure to set the error string when there is an out of memory and cleanup the temporary allocated data.

bjosv marked this pull request as ready for review December 4, 2020 15:32

bjosv commented Dec 4, 2020

View reviewed changes

tests/ct_out_of_memory_handling.c Outdated

return NULL;

}

// A OOM failing realloc()calloc

Copy link

Collaborator Author

bjosv Dec 4, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will delete "calloc"

bjosv commented Dec 4, 2020

View reviewed changes

Added initial out-of-memory handling tests

3d4db90

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.

bjosv force-pushed the safe_allocations branch from f0475f7 to 3d4db90 Compare December 6, 2020 11:26

bjosv added 3 commits December 6, 2020 12:57

Handle out of memory in command parsing

4369266

Cleanups of oom handling where hiarray is involved

c324347

Cleanups of OOM in list and dict cases

ed5eb61

bjosv force-pushed the safe_allocations branch from 9bca5c2 to ed5eb61 Compare December 6, 2020 22:40

Handle allocation failures of sds strings

81a86af

bjosv force-pushed the safe_allocations branch from 45cedbf to 81a86af Compare December 7, 2020 10:56

zuiderkwast reviewed Dec 7, 2020

View reviewed changes

zuiderkwast approved these changes Dec 7, 2020

View reviewed changes

zuiderkwast reviewed Dec 7, 2020

View reviewed changes

Improve allocation handling testcase

afc84ed

Also found some issues during the testcases regarding OOM handling, like a leak during OOM. Also fixed the order of setting timeout when reconnecting.

bjosv force-pushed the safe_allocations branch from 23fef4a to afc84ed Compare December 7, 2020 14:52

zuiderkwast reviewed Dec 7, 2020

View reviewed changes

bjosv merged commit 0f2922a into master Dec 7, 2020

bjosv deleted the safe_allocations branch December 7, 2020 17:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Safe allocations #15

Safe allocations #15

bjosv commented Dec 3, 2020 •

edited

Loading

zuiderkwast left a comment

zuiderkwast Dec 3, 2020

bjosv Dec 4, 2020

bjosv Dec 4, 2020

zuiderkwast commented Dec 4, 2020

zuiderkwast commented Dec 4, 2020

bjosv Dec 4, 2020

bjosv Dec 4, 2020

zuiderkwast Dec 7, 2020

bjosv Dec 4, 2020 •

edited

Loading

bjosv Dec 4, 2020

bjosv Dec 7, 2020

zuiderkwast left a comment

zuiderkwast Dec 7, 2020

zuiderkwast Dec 7, 2020

bjosv Dec 7, 2020

zuiderkwast Dec 7, 2020

bjosv Dec 7, 2020

zuiderkwast Dec 7, 2020

bjosv Dec 7, 2020

zuiderkwast Dec 7, 2020

bjosv Dec 7, 2020

zuiderkwast Dec 7, 2020

bjosv Dec 7, 2020

zuiderkwast left a comment

bjosv commented Dec 7, 2020

zuiderkwast left a comment

bjosv commented Dec 7, 2020

		if (ret != DICT_OK) {
		// Key already exists, but possibly an OOM error

Safe allocations #15

Safe allocations #15

Conversation

bjosv commented Dec 3, 2020 • edited Loading

zuiderkwast left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zuiderkwast commented Dec 4, 2020

zuiderkwast commented Dec 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bjosv Dec 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zuiderkwast left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zuiderkwast left a comment

Choose a reason for hiding this comment

bjosv commented Dec 7, 2020

zuiderkwast left a comment

Choose a reason for hiding this comment

bjosv commented Dec 7, 2020

bjosv commented Dec 3, 2020 •

edited

Loading

bjosv Dec 4, 2020 •

edited

Loading