Remove explicit malloc()-family calls from first-party code. #3143

kpschoedel · 2020-10-08T16:34:27Z

Problem

Embedded device-side code should only use heap through CHIPMem.h.
In particular it should not call malloc()-family functions.

Summary of Changes

Replace direct calls to malloc()/calloc()/realloc()/free().
Ensure that malloc()-family calls in non-device code are suitably
guarded by #ifs.
Add a C wrapper for Platform::Memory… functions, for
src/lwip/standalone/sys_arch.c and platform C.
Add a str[n]dup() equivalent, as calls are paired with free().

Fixes #2807 Remove explicit use of malloc() from first-party embedded-device code
Fixes #3092 CHIPMem.h called from C

src/platform/EFR32/efr32-chip-mbedtls-config.h

bzbarsky-apple

There should probably be more use of ScopedBuffer here, but a separate PR for that would be better.

bzbarsky-apple · 2020-10-08T18:11:56Z

src/lib/core/CHIPTLVWriter.cpp


-    tmpBuf = static_cast<char *>(malloc(dataLen + 1));
+    tmpBuf = static_cast<char *>(chip::Platform::MemoryAlloc(dataLen + 1));


Why is this write to a temp buffer better than the old !HAVE_MALLOC code which directly wrote it into our buffer? Especially if we fix that code to properly length-check how much space we have left...

It isn't, but my intent is for there to be no changes in this PR beyond what's required for a straight substitution of malloc→MemoryAlloc() etc. Later PRs can try to eliminate some of those uses entirely.

andy31415 · 2020-10-09T14:31:10Z

src/inet/tests/TestInetCommon.cpp

@@ -282,7 +283,7 @@ void InitNetwork()
        for (size_t j = 0; j < gNetworkOptions.LocalIPv6Addr.size(); j++)
        {
            uint64_t iid    = gNetworkOptions.LocalIPv6Addr[j].InterfaceId();
-            char * tap_name = (char *) malloc(sizeof(gDefaultTapDeviceName));
+            char * tap_name = (char *) chip::Platform::MemoryAlloc(sizeof(gDefaultTapDeviceName));


Maybe while we change this, add an assert that this is not null?

andy31415 · 2020-10-09T14:31:42Z

src/inet/tests/TestInetCommonOptions.cpp

@@ -256,9 +258,9 @@ bool FaultInjectionOptions::HandleOption(const char * progName, OptionSet * optS
    {
 #if CHIP_CONFIG_TEST || CHIP_SYSTEM_CONFIG_TEST || INET_CONFIG_TEST
    case kToolCommonOpt_FaultInjection: {
-        char * mutableArg = strdup(arg);
+        char * mutableArg = chip::Platform::MemoryAllocString(arg);


using ScopedBuffer and asserting non-null may be useful here

andy31415 · 2020-10-09T14:32:24Z

src/lib/core/CHIPTLVReader.cpp

    if (buf == nullptr)
        return CHIP_ERROR_NO_MEMORY;

    CHIP_ERROR err = ReadData(buf, static_cast<uint32_t>(mElemLenOrVal));
    if (err != CHIP_NO_ERROR)
    {
-        free(buf);
+        chip::Platform::MemoryFree(buf);


should we also set buf to nullptr here as well?

andy31415 · 2020-10-09T14:32:41Z

src/lib/core/CHIPTLVReader.cpp

    if (buf == nullptr)
        return CHIP_ERROR_NO_MEMORY;

    CHIP_ERROR err = ReadData(reinterpret_cast<uint8_t *>(buf), static_cast<uint32_t>(mElemLenOrVal));
    if (err != CHIP_NO_ERROR)
    {
-        free(buf);
+        chip::Platform::MemoryFree(buf);


I would set buf null here as well

andy31415 · 2020-10-09T14:33:24Z

src/lib/core/tests/TestCHIPTLV.cpp

@@ -211,14 +212,14 @@ void TestString(nlTestSuite * inSuite, TLVReader & reader, uint64_t tag, const c
    uint32_t expectedLen = strlen(expectedVal);
    NL_TEST_ASSERT(inSuite, reader.GetLength() == expectedLen);

-    char * val = static_cast<char *>(malloc(expectedLen + 1));
+    char * val = static_cast<char *>(chip::Platform::MemoryAlloc(expectedLen + 1));


I would suggest using https://github.com/project-chip/connectedhomeip/blob/master/src/lib/support/ScopedBuffer.h in here if possible, to not need to pair up alloc and free.

I'd prefer to do a 1:1 swap in this PR and all the improvements in a separate one, to keep the history clearer.

andy31415 · 2020-10-09T14:52:08Z

src/lib/support/CHIPMemString.h

+ */
+
+#ifndef CHIP_MEM_STRING_H
+#define CHIP_MEM_STRING_H


nit: consider #pragma once

mspang · 2020-10-09T14:50:27Z

src/lib/support/c/CHIPPlatformMemory.cpp

+
+extern "C" {
+
+extern int CHIPPlatformMemoryInit(void * buf, size_t bufSize)


Why can't this just be in CHIPMem.h and the rest hidden by ifdef __cplusplus ?

Do you mean the declarations, or are you suggesting defining them inline in the header?

I meant the declarations. Not sure if it's better, perhaps separate is the way to go.

The rationale for making them separate is to avoid adding non-namespaced symbols to a common header.

src/lib/support/BUILD.gn

src/platform/EFR32/efr32-chip-mbedtls-config.h

kpschoedel · 2020-10-09T16:02:52Z

Argh, my rebase made a mess.

#### Problem Embedded device-side code should only use heap through CHIPMem.h. In particular it should not call malloc()-family functions. #### Summary of Changes - Replace direct calls to malloc()/calloc()/realloc()/free(). - Ensure that malloc()-family calls in non-device code are suitably guarded by #ifs. - Add a C wrapper for Platform::Memory… functions, for src/lwip/standalone/sys_arch.c and platform C. - Add a str[n]dup() equivalent, as calls are paired with free(). fixes project-chip#2807 Remove explicit use of malloc() from first-party embedded-device code fixes project-chip#3092 CHIPMem.h called from C

#### Problem As part of heap memory management, a recent change (project-chip#3143) replaced malloc()-family calls one-for-one with their "CHIPMem.h" equivalents, but a one-to-one replacement is not always ideal. #### Summary of Changes - Use ScopedMemoryBuffer instead where possible. - Added ScopedMemoryString, since this is a common case. - Some addition safety checks.

#### Problem As part of heap memory management, a recent change (project-chip#3143) replaced malloc()-family calls one-for-one with their "CHIPMem.h" equivalents. Reviewers proposed some related small safety changes. #### Summary of Changes - added an assert() - null out two dead pointers

#### Problem As part of heap memory management, a recent change (#3143) replaced malloc()-family calls one-for-one with their "CHIPMem.h" equivalents. Reviewers proposed some related small safety changes. #### Summary of Changes - added an assert() - null out two dead pointers

* Clean up ex-malloc() use of CHIPMem.h #### Problem As part of heap memory management, a recent change (#3143) replaced malloc()-family calls one-for-one with their "CHIPMem.h" equivalents, but a one-to-one replacement is not always ideal. #### Summary of Changes - Use ScopedMemoryBuffer instead where possible. - Added ScopedMemoryString, since this is a common case. - Some addition safety checks. * Split off safety changes. * Simplified string operations. * Move strlen to callers * Review

boring-cyborg bot added efr32 esp32 examples inet lib lwip platform labels Oct 8, 2020

restyled-io bot mentioned this pull request Oct 8, 2020

Restyle Remove explicit malloc()-family calls from first-party code. #3144

Closed

kpschoedel commented Oct 8, 2020

View reviewed changes

src/platform/EFR32/efr32-chip-mbedtls-config.h Outdated Show resolved Hide resolved

pullapprove bot requested review from andy31415, BroderickCarlin, bzbarsky-apple, chrisdecenzo, hawk248, jelderton, rwalker-apple and saurabhst October 8, 2020 16:47

restyled-io bot mentioned this pull request Oct 8, 2020

Restyle Remove explicit malloc()-family calls from first-party code. #3149

Closed

bzbarsky-apple approved these changes Oct 8, 2020

View reviewed changes

rwalker-apple approved these changes Oct 8, 2020

View reviewed changes

BroderickCarlin approved these changes Oct 8, 2020

View reviewed changes

andy31415 approved these changes Oct 9, 2020

View reviewed changes

mspang reviewed Oct 9, 2020

View reviewed changes

src/platform/EFR32/efr32-chip-mbedtls-config.h Outdated Show resolved Hide resolved

boring-cyborg bot added config github workflows labels Oct 9, 2020

restyled-io bot mentioned this pull request Oct 9, 2020

Restyle Remove explicit malloc()-family calls from first-party code. #3176

Closed

restyled-commits and others added 4 commits October 9, 2020 12:10

Restyled by clang-format

def173a

Include CHIPPlatformMemory.h from efr32-chip-mbedtls-config.h

5fba180

Restyled by clang-format

893d415

Review comments

1f74ad7

kpschoedel force-pushed the x2899-mem-malloc branch from d56dc0a to 1f74ad7 Compare October 9, 2020 16:11

Restyled by gn

05913fa

kpschoedel mentioned this pull request Oct 9, 2020

Use mbedtls_platform_set_calloc_free() for non-malloc() configurations #3178

Closed

Restyled by clang-format

2ef6f2a

mspang merged commit 41feafa into project-chip:master Oct 9, 2020

kpschoedel deleted the x2899-mem-malloc branch October 13, 2020 14:45

kpschoedel mentioned this pull request Oct 13, 2020

Clean up ex-malloc() use of CHIPMem.h #3222

Merged

kpschoedel mentioned this pull request Oct 13, 2020

Safety changes after malloc to CHIPMem conversions #3230

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove explicit malloc()-family calls from first-party code. #3143

Remove explicit malloc()-family calls from first-party code. #3143

kpschoedel commented Oct 8, 2020

bzbarsky-apple left a comment

bzbarsky-apple Oct 8, 2020

kpschoedel Oct 8, 2020

andy31415 Oct 9, 2020

andy31415 Oct 9, 2020

andy31415 Oct 9, 2020

andy31415 Oct 9, 2020

andy31415 Oct 9, 2020

kpschoedel Oct 9, 2020

andy31415 Oct 9, 2020

mspang Oct 9, 2020

kpschoedel Oct 9, 2020

mspang Oct 9, 2020

kpschoedel Oct 10, 2020 •

edited

Loading

kpschoedel commented Oct 9, 2020


		tmpBuf = static_cast<char *>(malloc(dataLen + 1));
		tmpBuf = static_cast<char *>(chip::Platform::MemoryAlloc(dataLen + 1));


		extern "C" {

		extern int CHIPPlatformMemoryInit(void * buf, size_t bufSize)

Remove explicit malloc()-family calls from first-party code. #3143

Remove explicit malloc()-family calls from first-party code. #3143

Conversation

kpschoedel commented Oct 8, 2020

Problem

Summary of Changes

bzbarsky-apple 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

Choose a reason for hiding this comment

kpschoedel Oct 10, 2020 • edited Loading

Choose a reason for hiding this comment

kpschoedel commented Oct 9, 2020

kpschoedel Oct 10, 2020 •

edited

Loading