Add overflow tests #587

PatKamin · 2024-07-02T08:04:22Z

Description

Checklist

Code compiles without errors locally
All tests pass locally
CI workflows execute properly

CI workflows, not executed per PR (e.g. Nightly), execute properly
New tests added, especially if they will fail without my changes
Added/extended example(s) to cover this functionality
Extended the README/documentation
All newly added source files have a license
All newly added source files are referenced in CMake files
Logger (with debug/info/... messages) is used

lukaszstolarczuk · 2024-07-02T10:52:50Z

test/provider_os_memory_multiple_numa_nodes.cpp

+                                       &os_memory_provider_params,
+                                       &os_memory_provider);
+    ASSERT_EQ(res, UMF_RESULT_SUCCESS);
+    ASSERT_EQ(os_memory_provider, nullptr);


so provider is null, but we expect UMF_RES_SUCCESS?

lplewa · 2024-07-05T13:40:42Z

src/provider/provider_os_memory.c

@@ -360,6 +360,15 @@ validatePartitions(umf_os_memory_provider_params_t *params) {
    return UMF_RESULT_SUCCESS;
 }

+static umf_result_t validatePartSize(os_memory_provider_t *provider, umf_os_memory_provider_params_t *params) {
+    size_t page_size;
+    if (ALIGN_UP(params->part_size, os_get_min_page_size(provider, NULL, &page_size)) < params->part_size) {


This do not compile (os_get_min_page_size, return ut_result - page_size is returned by pointer)

Also i think we need function that align up/down and returns value if it succeeded or not. And use it everywhere. Not sure what is better:

dozen of functions (aling_up_safe_uint32, aling_up_safe_uint64, aling_up_safe_int64 etc...)

macro ALIGN_UP_SAFE - which might be bit hacky. Probably something like this
#define ALIGN_UP_SAFE(value, alignment) (((alignment) == 0) ? (value = 0) : (((value) + (alignment) - 1) < (value) ? -1 : ALIGN_UP(value, alignment)))

Write C++ template function align_up_safe<T>(value).

lplewa · 2024-07-05T13:48:39Z

test/common/provider.hpp

@@ -106,6 +106,10 @@ struct provider_malloc : public provider_base_t {
            align = 8;
        }

+        if (SIZE_MAX - size < align) {


I'm allmost sure (but please double check) that it should be SIZE_MAX-size + 1 < align

In ALIGN_UP macro we have (((value) + (align)-1) & ~((align)-1))
The part which overflow is (value) + (align)-1 so we want to check
(value) + (align)-1 > SIZE_MAX - this equation is prone for overflow so we have to change it to + align > SIZE_MAX - value + 1

ldorau · 2024-09-04T13:22:54Z

src/base_alloc/base_alloc.c

@@ -134,7 +134,10 @@ static void *ba_os_alloc_annotated(size_t pool_size) {
 }

 umf_ba_pool_t *umf_ba_create(size_t size) {
-    size_t chunk_size = ALIGN_UP(size, MEMORY_ALIGNMENT);
+    size_t chunk_size = ALIGN_UP_SAFE(size, MEMORY_ALIGNMENT);
+    if (!chunk_size) {


IMHO in case of numeric values you should use (just for better understanding)

if (chunk_size == 0)

(instead of if (!chunk_size)) - also in all places below.

ldorau · 2024-09-04T13:24:48Z

src/utils/utils_common.h

@@ -29,8 +29,13 @@ extern "C" {
        expression;                                                            \
    } while (0)

-#define ALIGN_UP(value, align) (((value) + (align)-1) & ~((align)-1))
-#define ALIGN_DOWN(value, align) ((value) & ~((align)-1))
+#define ALIGN_UP(value, align) ((value + align - 1) & ~(align - 1))


It should stay as was:

#define ALIGN_UP(value, align) (((value) + (align) - 1) & ~((align) - 1))

Please revert it.

ldorau · 2024-09-04T13:25:48Z

src/utils/utils_common.h

+                      : ((value + alignment - 1) < value                       \
+                             ? 0                                               \
+                             : ((value + alignment - 1) & ~(alignment - 1))))
+#define ALIGN_DOWN(value, align) (value & ~(align - 1))


It should stay as was:

#define ALIGN_DOWN(value, align) ((value) & ~((align) - 1))

Please revert it.

ldorau · 2024-09-04T13:28:11Z

src/utils/utils_common.h

-#define ALIGN_UP(value, align) (((value) + (align)-1) & ~((align)-1))
-#define ALIGN_DOWN(value, align) ((value) & ~((align)-1))
+#define ALIGN_UP(value, align) ((value + align - 1) & ~(align - 1))
+#define ALIGN_UP_SAFE(value, alignment)                                        \


All occurrences of value and alignment should be in parenthesis ((value) and (alignment)), in case they were expressions.

PatKamin requested a review from a team as a code owner July 2, 2024 08:04

bratpiorka mentioned this pull request Jul 2, 2024

Add tests for integer overflow #541

Open

lukaszstolarczuk reviewed Jul 2, 2024

View reviewed changes

PatKamin force-pushed the integer-overflow-tests branch 3 times, most recently from 4f36fe1 to 0086875 Compare July 5, 2024 09:20

bratpiorka requested a review from lplewa July 5, 2024 09:39

lplewa requested changes Jul 5, 2024

View reviewed changes

PatKamin force-pushed the integer-overflow-tests branch from 0086875 to 23b6809 Compare August 14, 2024 11:35

Add integer overflow tests

e404fc2

PatKamin force-pushed the integer-overflow-tests branch from 23b6809 to adf2055 Compare September 4, 2024 09:03

PatKamin added 2 commits September 4, 2024 14:43

Fix integer overflows

2397c91

Align values up safely

4cb4024

PatKamin force-pushed the integer-overflow-tests branch from adf2055 to 4cb4024 Compare September 4, 2024 12:46

ldorau requested changes Sep 4, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add overflow tests #587

Add overflow tests #587

PatKamin commented Jul 2, 2024

lukaszstolarczuk Jul 2, 2024

PatKamin Jul 5, 2024

lplewa Jul 5, 2024

KFilipek Jul 17, 2024 •

edited

Loading

lplewa Jul 5, 2024

ldorau Sep 4, 2024 •

edited

Loading

ldorau Sep 4, 2024 •

edited

Loading

ldorau Sep 4, 2024 •

edited

Loading

ldorau Sep 4, 2024

Add overflow tests #587

Are you sure you want to change the base?

Add overflow tests #587

Conversation

PatKamin commented Jul 2, 2024

Description

Checklist

lukaszstolarczuk Jul 2, 2024

Choose a reason for hiding this comment

PatKamin Jul 5, 2024

Choose a reason for hiding this comment

lplewa Jul 5, 2024

Choose a reason for hiding this comment

KFilipek Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

lplewa Jul 5, 2024

Choose a reason for hiding this comment

ldorau Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

ldorau Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

ldorau Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

ldorau Sep 4, 2024

Choose a reason for hiding this comment

KFilipek Jul 17, 2024 •

edited

Loading

ldorau Sep 4, 2024 •

edited

Loading

ldorau Sep 4, 2024 •

edited

Loading

ldorau Sep 4, 2024 •

edited

Loading