[K32W] Fix memory allocation #5242

doru91 · 2021-03-09T11:53:43Z

We had to fix the alloc/free pointers because OpenThread API changed.

Signed-off-by: Doru Gucea doru-cristian.gucea@nxp.com

CLAassistant · 2021-03-09T11:53:47Z

All committers have signed the CLA.

tcarmelveilleux · 2021-03-09T15:05:47Z

examples/lock-app/k32w/main/main.cpp

@@ -65,6 +65,8 @@ extern "C" void main_task(void const * argument)
        (*pFunc)();
    }

+    mbedtls_platform_set_calloc_free(otPlatCAlloc, otPlatFree);


Why go through otPlatCalloc/otPlatFree? The mbedtls library should use whatever this platform's allocator is, and not rely on implementations from OpenThread, which, as it stands, are no longer supported to be called outside of OpenThread IIUC.

Suggested change

mbedtls_platform_set_calloc_free(otPlatCAlloc, otPlatFree);

mbedtls_platform_set_calloc_free(pvPortCallocRtos, vPortFree);

I thought malloc/calloc/free (and new/delete) are automatically mapped by FreeRTOS to the port calls.

What makes mbedtls require manual setting of the platform callbacks? Also for sure wondering why we move to ot (guessing openthread prefix) as an intermediate.

Shouldn't we use CHIPPlatformMemoryCalloc and CHIPPlatformMemoryFree as defined in src/lib/support/CHIPPlatformMemory.cpp

You can also map otPlatCAlloc and otPlatFree to use those too so it is consistent across the project

Changed the call to mbedtls_platform_set_calloc_free(pvPortCallocRtos, vPortFree) as suggested by @tcarmelveilleux.

If I use CHIPPlatformMemoryCalloc then a call to malloc is done internally and I'm not sure if this is redirected to FreeRTOS pvPortMalloc.

Below is the pvPortMalloc function from heap3.c (FreeRTOS). In addition to calling malloc is also do some task related processing. So I believe that it's not safe to call directly CHIPPlatformMemoryCalloc instead of pvPortMalloc.

void *pvPortMalloc( size_t xWantedSize ) { void *pvReturn; vTaskSuspendAll(); { pvReturn = malloc( xWantedSize ); traceMALLOC( pvReturn, xWantedSize ); } ( void ) xTaskResumeAll(); #if( configUSE_MALLOC_FAILED_HOOK == 1 ) { if( pvReturn == NULL ) { extern void vApplicationMallocFailedHook( void ); vApplicationMallocFailedHook(); } } #endif return pvReturn; }

There are a few points here:

CHIPPlatformMemoryCalloc calls chip::platform::MemoryCalloc() which calls calloc().

pvPortMalloc on FreeRTOS, needs to be tied to replace the C/C++ library malloc, so that you don't have 2 heaps: FreeRTOS and C library "leftover linker space" in the linker script as an untracked heap. The other reason it needs to be replaced is that pvPortMalloc does the necessary processing to make malloc concurrency-safe on FreeRTOS.

It is odd to me that these allocators (e.g. chip::platform::MemoryCalloc()) are using global functions scoping and end-up calling C library, which is what you are trying to defeat by directly calling pvPortCallocRtos/vPortFree in other places. It means there is an expectation that all platforms override core malloc/calloc/free and that these remain callable. That's OK, I just didn't know about that assumption not being handled yet on your platform.

I agree with @andy31415 that a core expectation it seems is that C library malloc/calloc/free are redirected to a platform version rather than using whatever heap allocated is built-in to your C standard library.

tcarmelveilleux · 2021-03-09T15:06:33Z

examples/platform/k32w/app/support/FreeRtosMbedtlsUtils.c

+void * otPlatCAlloc(size_t aNum, size_t aSize)
+{
+    return pvPortCallocRtos(aNum, aSize);
+}
+
+void otPlatFree(void * aPtr)
+{
+    return vPortFree(aPtr);
+}


Is this really OpenThread-specific (like the ot prefix implies)? If so, why is it in *MbedtlsUtils.c ?

Agree, the naming is not correct. I moved otPlatCAlloc and otPlatFree to src/platform/K32W/ThreadStackManagerImpl.cpp.

andy31415 · 2021-03-10T15:40:06Z

src/platform/K32W/ThreadStackManagerImpl.cpp

+    return p;
+}
+
+extern "C" void * otPlatCAlloc(size_t aNum, size_t aSize)


are the otPlatCAlloc/otPlatFree methods still needed?

They are still needed because I want OT to use the memory allocator from FreeRTOS.

mspang · 2021-03-10T17:18:29Z

src/platform/K32W/ThreadStackManagerImpl.cpp

+
+extern "C" void * pvPortCallocRtos(size_t num, size_t size)
+{
+    size_t totalAllocSize = (size_t)(num * size);


This arithmetic is not always correct. calloc should fail if the multiplication overflows.

Please use C++ style casts.

Any suggestion on how to do it using C++ style casts?

I don't think you need a cast.

To check for overflow you can use division.

totalAllocSize = num * size; if (size && totalAllocSize / size != num) return nullptr;

Thanks, pushed this fix!

doru91 · 2021-03-11T10:09:39Z

I don't know why the QEMU/ESP32 build is failing as this PR doesn't touch any ESP32 related files.

bzbarsky-apple · 2021-03-11T15:15:39Z

I don't know why the QEMU/ESP32 build is failing as this PR doesn't touch any ESP32 related files.

Is the PR rebased to tip of master? If not, that might help; there was some sort of QEMU failure sometime last week, iirc.

We had to fix the alloc/free pointers because OpenThread API changed. Signed-off-by: Doru Gucea <doru-cristian.gucea@nxp.com>

Signed-off-by: Doru Gucea <doru-cristian.gucea@nxp.com>

doru91 · 2021-03-12T07:23:59Z

I don't know why the QEMU/ESP32 build is failing as this PR doesn't touch any ESP32 related files.

Is the PR rebased to tip of master? If not, that might help; there was some sort of QEMU failure sometime last week, iirc.

OK, I just rebased, let's see if the issue is solved. L.E.: It was solved.

LuDuda

LGTM 👍

* [K32W] Fix memory allocation We had to fix the alloc/free pointers because OpenThread API changed. Signed-off-by: Doru Gucea <doru-cristian.gucea@nxp.com> * Restyled by clang-format * [K32W] Fix possible buffer overflow Signed-off-by: Doru Gucea <doru-cristian.gucea@nxp.com> Co-authored-by: Restyled.io <commits@restyled.io>

boring-cyborg bot added examples k32w platform labels Mar 9, 2021

restyled-io bot mentioned this pull request Mar 9, 2021

Restyle [K32W] Fix memory allocation #5243

Closed

pullapprove bot requested review from andy31415, bzbarsky-apple, chrisdecenzo, hawk248, jelderton, msandstedt, mspang, saurabhst, woody-apple and yufengwangca March 9, 2021 14:34

doru91 force-pushed the master branch from 55d1c2b to faf6445 Compare March 9, 2021 15:04

tcarmelveilleux reviewed Mar 9, 2021

View reviewed changes

mspang approved these changes Mar 9, 2021

View reviewed changes

pullapprove bot requested review from jepenven-silabs and jmartinez-silabs March 9, 2021 22:52

doru91 force-pushed the master branch from faf6445 to 7acd5ec Compare March 10, 2021 07:48

restyled-io bot mentioned this pull request Mar 10, 2021

Restyle [K32W] Fix memory allocation #5288

Closed

doru91 requested a review from tcarmelveilleux March 10, 2021 12:25

andy31415 reviewed Mar 10, 2021

View reviewed changes

mspang reviewed Mar 10, 2021

View reviewed changes

pullapprove bot requested review from Damian-Nordic and LuDuda March 10, 2021 20:29

doru91 force-pushed the master branch from d23535d to 2c7d68c Compare March 11, 2021 07:49

doru91 requested a review from andy31415 March 11, 2021 07:49

mspang approved these changes Mar 11, 2021

View reviewed changes

woody-apple approved these changes Mar 11, 2021

View reviewed changes

jmartinez-silabs approved these changes Mar 11, 2021

View reviewed changes

doru91 and others added 3 commits March 11, 2021 23:20

[K32W] Fix memory allocation

5ce5f52

We had to fix the alloc/free pointers because OpenThread API changed. Signed-off-by: Doru Gucea <doru-cristian.gucea@nxp.com>

Restyled by clang-format

2c58418

[K32W] Fix possible buffer overflow

874c5c8

Signed-off-by: Doru Gucea <doru-cristian.gucea@nxp.com>

doru91 force-pushed the master branch from 2c7d68c to 874c5c8 Compare March 12, 2021 07:22

LuDuda approved these changes Mar 12, 2021

View reviewed changes

andy31415 approved these changes Mar 12, 2021

View reviewed changes

andy31415 merged commit 18b484f into project-chip:master Mar 12, 2021

kpschoedel mentioned this pull request Mar 12, 2021

Configuration support for wrapping malloc-family #5345

Closed

franck-apple added the TE1 cherry pick label Mar 19, 2021

andy31415 added the TE1 cherrypick DONE label Mar 19, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[K32W] Fix memory allocation #5242

[K32W] Fix memory allocation #5242

doru91 commented Mar 9, 2021

CLAassistant commented Mar 9, 2021 •

edited

Loading

tcarmelveilleux Mar 9, 2021 •

edited

Loading

andy31415 Mar 9, 2021

jmartinez-silabs Mar 10, 2021 •

edited

Loading

doru91 Mar 10, 2021 •

edited

Loading

doru91 Mar 11, 2021 •

edited

Loading

tcarmelveilleux Mar 12, 2021 •

edited

Loading

tcarmelveilleux Mar 9, 2021 •

edited

Loading

doru91 Mar 10, 2021

andy31415 Mar 10, 2021

doru91 Mar 11, 2021

mspang Mar 10, 2021

doru91 Mar 11, 2021

mspang Mar 11, 2021

doru91 Mar 11, 2021

doru91 commented Mar 11, 2021

bzbarsky-apple commented Mar 11, 2021

doru91 commented Mar 12, 2021 •

edited

Loading

LuDuda left a comment

	mbedtls_platform_set_calloc_free(otPlatCAlloc, otPlatFree);
	mbedtls_platform_set_calloc_free(pvPortCallocRtos, vPortFree);

[K32W] Fix memory allocation #5242

[K32W] Fix memory allocation #5242

Conversation

doru91 commented Mar 9, 2021

CLAassistant commented Mar 9, 2021 • edited Loading

tcarmelveilleux Mar 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmartinez-silabs Mar 10, 2021 • edited Loading

Choose a reason for hiding this comment

doru91 Mar 10, 2021 • edited Loading

Choose a reason for hiding this comment

doru91 Mar 11, 2021 • edited Loading

Choose a reason for hiding this comment

tcarmelveilleux Mar 12, 2021 • edited Loading

Choose a reason for hiding this comment

tcarmelveilleux Mar 9, 2021 • edited Loading

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

doru91 commented Mar 11, 2021

bzbarsky-apple commented Mar 11, 2021

doru91 commented Mar 12, 2021 • edited Loading

LuDuda left a comment

Choose a reason for hiding this comment

CLAassistant commented Mar 9, 2021 •

edited

Loading

tcarmelveilleux Mar 9, 2021 •

edited

Loading

jmartinez-silabs Mar 10, 2021 •

edited

Loading

doru91 Mar 10, 2021 •

edited

Loading

doru91 Mar 11, 2021 •

edited

Loading

tcarmelveilleux Mar 12, 2021 •

edited

Loading

tcarmelveilleux Mar 9, 2021 •

edited

Loading

doru91 commented Mar 12, 2021 •

edited

Loading