Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

kernel: k_heap failures on small heaps #33009

Closed
JordanYates opened this issue Mar 6, 2021 · 2 comments · Fixed by #35471
Closed

kernel: k_heap failures on small heaps #33009

JordanYates opened this issue Mar 6, 2021 · 2 comments · Fixed by #35471
Assignees
Labels
bug The issue is a bug, or the PR is fixing a bug priority: medium Medium impact/importance bug
Milestone

Comments

@JordanYates
Copy link
Collaborator

Describe the bug

k_heap_alloc fails when attempting to allocate memory from small heaps.
The following code will fail:

K_HEAP_DEFINE(test_heap, 40);

void main(void) {
    void * p = k_heap_alloc(&test_heap, 8, K_NO_WAIT);
    printk("%p\n", p);
}

On a Cortex M4F board, this will just return NULL, but on native_posix_64 a K_ERR_KERNEL_PANIC is triggered.
Based on the documentation (https://docs.zephyrproject.org/latest/reference/kernel/memory/heap.html), it seems clear that there is simply not enough space at the start of the buffer to hold the bucket list for allocations.

The bug is not that the k_heap cannot support tiny heaps, but that fatal errors can occur.
It also not obvious from the documentation how large the heap needs to be before it will start working.
64 bytes is sufficient to allocate 8 bytes on a Cortex M4F , but not native_posix_64.

Expected behavior

Allocating from small heap should just return NULL, not hard fault.
Ideally some form of validation would be done on the values provided to K_HEAP_DEFINE to ensure that it is large enough to return a valid 8 byte (smallest allocation unit) memory chunk at least once.

Something similar to:

#define K_HEAP_DEFINE(name, bytes)				\
	BUILD_ASSERT((bytes) >= (MINIMUM_BUCKET_SIZE + 8), "Heap too small to allocate memory from");\
	char __aligned(sizeof(void *)) kheap_##name[bytes];	\
	Z_STRUCT_SECTION_ITERABLE(k_heap, name) = {		\
		.heap = {					\
			.init_mem = kheap_##name,		\
			.init_bytes = (bytes),			\
		 },						\
	}

Environment (please complete the following information):

  • Zephyr v2.5
@JordanYates JordanYates added the bug The issue is a bug, or the PR is fixing a bug label Mar 6, 2021
@nashif nashif added the priority: medium Medium impact/importance bug label Mar 6, 2021
@rljordan rljordan assigned yerabolu and unassigned andyross Mar 8, 2021
@AmberHibberd AmberHibberd assigned yerabolu and unassigned yerabolu Mar 14, 2021
@rljordan rljordan assigned andyross and unassigned yerabolu Mar 17, 2021
@yerabolu
Copy link
Contributor

just a quick update @andyross: tried running the test with a small heap size of 40 and tests passed on both Cortex_M3 and native_posix_64.

@JordanYates
Copy link
Collaborator Author

On the latest master with the following diff, I can still reproduce the fatal errors

jordan@DOTA:/mnt/c/Users/Jordan/code/zephyr/zephyr$ git diff HEAD
diff --git a/tests/kernel/mem_heap/k_heap_api/src/test_kheap_api.c b/tests/kernel/mem_heap/k_heap_api/src/test_kheap_api.c
index dc7d0c2ead..d8a93cab9b 100644
--- a/tests/kernel/mem_heap/k_heap_api/src/test_kheap_api.c
+++ b/tests/kernel/mem_heap/k_heap_api/src/test_kheap_api.c
@@ -13,6 +13,7 @@ K_THREAD_STACK_DEFINE(tstack, STACK_SIZE);
 struct k_thread tdata;

 K_HEAP_DEFINE(k_heap_test, HEAP_SIZE);
+K_HEAP_DEFINE(k_heap_test_small, 40);

 #define ALLOC_SIZE_1 1024
 #define ALLOC_SIZE_2 1536
@@ -61,6 +62,9 @@ void test_k_heap_alloc(void)
                p[i] = '0';
        }
        k_heap_free(&k_heap_test, p);
+
+       p = (char *)k_heap_alloc(&k_heap_test_small, 8, timeout);
+       zassert_not_null(p, "k_heap_alloc operation failed");
 }

@rljordan rljordan assigned laurenmurphyx64 and unassigned andyross Apr 21, 2021
@galak galak added this to the v2.6.0 milestone May 11, 2021
@rljordan rljordan assigned andyross and unassigned laurenmurphyx64 May 19, 2021
andyross pushed a commit to andyross/zephyr that referenced this issue May 20, 2021
The K_HEAP_DEFINE macro would allow users to specify heaps that are
too small, leading to potential corruption events (though at least
there were __ASSERTs that would catch this at runtime if enabled).

It would be nice to put the logic to compute this value into the heap
code, but that isn't available in kernel.h (and we don't want to pull
it in as this header is already WAY to thick).  So instead we just
hand-compute and document the choice.  We can address bitrot problems
with a test.

(Tweaks to heap size asserts and correct size bounds from Nicolas Pitre)

Fixes zephyrproject-rtos#33009

Signed-off-by: Andy Ross <[email protected]>
nashif pushed a commit that referenced this issue May 20, 2021
The K_HEAP_DEFINE macro would allow users to specify heaps that are
too small, leading to potential corruption events (though at least
there were __ASSERTs that would catch this at runtime if enabled).

It would be nice to put the logic to compute this value into the heap
code, but that isn't available in kernel.h (and we don't want to pull
it in as this header is already WAY to thick).  So instead we just
hand-compute and document the choice.  We can address bitrot problems
with a test.

(Tweaks to heap size asserts and correct size bounds from Nicolas Pitre)

Fixes #33009

Signed-off-by: Andy Ross <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug The issue is a bug, or the PR is fixing a bug priority: medium Medium impact/importance bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants