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

* 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

src/controller/CHIPDeviceController.cpp

@@ -39,6 +39,7 @@
 #include <core/CHIPEncoding.h>
 #include <core/CHIPSafeCasts.h>
 #include <support/Base64.h>
+#include <support/CHIPMem.h>
 #include <support/CodeUtils.h>
 #include <support/ErrorStr.h>
 #include <support/TimeUtils.h>

src/inet/tests/TestInetCommonOptions.cpp

@@ -30,6 +30,7 @@
 
 #include "TestInetCommonOptions.h"
 
+#include <assert.h>
 #include <inttypes.h>
 #include <stdint.h>
 #include <stdio.h>
@@ -258,9 +259,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 = chip::Platform::MemoryAllocString(arg);
-        bool parseRes     = ParseFaultInjectionStr(mutableArg, faultMgrFnTable, faultMgrFnTableLen);
-        chip::Platform::MemoryFree(mutableArg);
+        chip::Platform::ScopedMemoryString mutableArg(arg, strlen(arg));
+        assert(mutableArg);
+        bool parseRes = ParseFaultInjectionStr(mutableArg.Get(), faultMgrFnTable, faultMgrFnTableLen);
         if (!parseRes)
         {
             PrintArgError("%s: Invalid string specified for fault injection option: %s\n", progName, arg);

src/lib/core/tests/TestCHIPTLV.cpp

@@ -38,6 +38,7 @@
 #include <support/CHIPMem.h>
 #include <support/CodeUtils.h>
 #include <support/RandUtils.h>
+#include <support/ScopedBuffer.h>
 #include <support/TestUtils.h>
 
 #include <string.h>
@@ -212,14 +213,13 @@ 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 *>(chip::Platform::MemoryAlloc(expectedLen + 1));
+    chip::Platform::ScopedMemoryBuffer<char> valBuffer;
+    char * val = static_cast<char *>(valBuffer.Alloc(expectedLen + 1).Get());
 
     CHIP_ERROR err = reader.GetString(val, expectedLen + 1);
     NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
 
     NL_TEST_ASSERT(inSuite, memcmp(val, expectedVal, expectedLen + 1) == 0);
-
-    chip::Platform::MemoryFree(val);
 }
 
 void TestDupString(nlTestSuite * inSuite, TLVReader & reader, uint64_t tag, const char * expectedVal)
@@ -230,14 +230,13 @@ void TestDupString(nlTestSuite * inSuite, TLVReader & reader, uint64_t tag, cons
     uint32_t expectedLen = strlen(expectedVal);
     NL_TEST_ASSERT(inSuite, reader.GetLength() == expectedLen);
 
-    char * val = static_cast<char *>(chip::Platform::MemoryAlloc(expectedLen + 1));
+    chip::Platform::ScopedMemoryBuffer<char> valBuffer;
+    char * val = valBuffer.Alloc(expectedLen + 1).Get();
 
     CHIP_ERROR err = reader.DupString(val);
     NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
 
     NL_TEST_ASSERT(inSuite, memcmp(val, expectedVal, expectedLen + 1) == 0);
-
-    chip::Platform::MemoryFree(val);
 }
 
 void TestDupBytes(nlTestSuite * inSuite, TLVReader & reader, uint64_t tag, const uint8_t * expectedVal, uint32_t expectedLen)
@@ -247,13 +246,12 @@ void TestDupBytes(nlTestSuite * inSuite, TLVReader & reader, uint64_t tag, const
 
     NL_TEST_ASSERT(inSuite, reader.GetLength() == expectedLen);
 
-    uint8_t * val  = static_cast<uint8_t *>(chip::Platform::MemoryAlloc(expectedLen));
+    chip::Platform::ScopedMemoryBuffer<uint8_t> valBuffer;
+    uint8_t * val  = valBuffer.Alloc(expectedLen).Get();
     CHIP_ERROR err = reader.DupBytes(val, expectedLen);
     NL_TEST_ASSERT(inSuite, err == CHIP_NO_ERROR);
 
     NL_TEST_ASSERT(inSuite, memcmp(val, expectedVal, expectedLen) == 0);
-
-    chip::Platform::MemoryFree(val);
 }
 
 void TestBufferContents(nlTestSuite * inSuite, PacketBuffer * buf, const uint8_t * expectedVal, uint32_t expectedLen)

src/lib/support/BUILD.gn

@@ -69,7 +69,6 @@ static_library("support") {
     "Base64.cpp",
     "CHIPArgParser.cpp",
     "CHIPCounter.cpp",
-    "CHIPMemString.cpp",
     "CHIPPlatformMemory.cpp",
     "ErrorStr.cpp",
     "FibonacciUtils.cpp",

src/lib/support/CHIPArgParser.cpp

@@ -491,29 +491,26 @@ bool ParseArgs(const char * progName, int argc, char * argv[], OptionSet * optSe
 bool ParseArgsFromString(const char * progName, const char * argStr, OptionSet * optSets[],
                          NonOptionArgHandlerFunct nonOptArgHandler, bool ignoreUnknown)
 {
-    char * argStrCopy = nullptr;
-    char ** argv      = nullptr;
+    char ** argv = nullptr;
     int argc;
     bool res;
 
-    argStrCopy = chip::Platform::MemoryAllocString(argStr);
-    if (argStrCopy == nullptr)
+    chip::Platform::ScopedMemoryString argStrCopy(argStr, strlen(argStr));
+    if (!argStrCopy)
     {
         PrintArgError("%s: Memory allocation failure\n", progName);
         return false;
     }
 
-    argc = SplitArgs(argStrCopy, argv, const_cast<char *>(progName));
+    argc = SplitArgs(argStrCopy.Get(), argv, const_cast<char *>(progName));
     if (argc < 0)
     {
         PrintArgError("%s: Memory allocation failure\n", progName);
-        chip::Platform::MemoryFree(argStrCopy);
         return false;
     }
 
     res = ParseArgs(progName, argc, argv, optSets, nonOptArgHandler, ignoreUnknown);
 
-    chip::Platform::MemoryFree(argStrCopy);
     chip::Platform::MemoryFree(argv);
 
     return res;

src/lib/support/CHIPMemString.h

@@ -21,31 +21,79 @@
  *      This file defines string operations that allocate heap memory.
  */
 
-#ifndef CHIP_MEM_STRING_H
-#define CHIP_MEM_STRING_H
+#pragma once
 
 #include <stdlib.h>
+#include <string.h>
+
+#include <support/ScopedBuffer.h>
 
 namespace chip {
 namespace Platform {
 
+/**
+ * Copies a C-style string.
+ *
+ * This differs from `strncpy()` in some important ways:
+ *  - `dest` can be nullptr, in which case no copy is attempted, and the function returns nullptr.
+ *  - A non-nullptr result is always null-terminated.
+ *
+ * @param[in]  dest             Destination string buffer (which must be at least `length`+1 bytes)
+ *                              or nullptr.
+ *
+ * @param[in]  source           String to be copied.
+ *
+ * @param[in]  length           Length to be copied.
+ *
+ * @retval  Same as `dest`.
+ */
+inline char * CopyString(char * dest, const char * source, size_t length)
+{
+    if (dest)
+    {
+        strncpy(dest, source, length);
+        dest[length] = 0;
+    }
+    return dest;
+}
+
 /**
  * This function copies a C-style string to memory newly allocated by Platform::MemoryAlloc().
  *
  * @param[in]  string           String to be copied.
  *
- * @param[in]  length           Length of the string to be copied. If zero, `strlen(string)`
- *                              will be used. Like `strncpy()`, if the `string` is shorter
- *                              then the remaining space up to `length` will be filled with
- *                              null bytes.
+ * @param[in]  length           Length to be copied. Like `strncpy()`, if the `string` is shorter
+ *                              than `length`, then the remaining space will be filled with null
+ *                              bytes. Like `strndup()` but unlike `strncpy()`, the result is always
+ *                              null-terminated.
  *
  * @retval  Pointer to a null-terminated string in case of success.
  * @retval  `nullptr` if memory allocation fails.
  *
  */
-extern char * MemoryAllocString(const char * string, size_t length = 0);
+inline char * MemoryAllocString(const char * string, size_t length)
+{
+    return CopyString(static_cast<char *>(MemoryAlloc(length + 1)), string, length);
+}
+
+/**
+ * Represents a C string in a ScopedMemoryBuffer.
+ */
+
+class ScopedMemoryString : public ScopedMemoryBuffer<char>
+{
+public:
+    /**
+     * Create a ScopedMemoryString.
+     *
+     * @param[in]  string           String to be copied.
+     *
+     * @param[in]  length           Length to be copied. Like `strncpy()`, if the `string` is shorter than
+     *                              `length`, then the remaining space will be filled with null bytes. Like
+     *                              `strndup()` but unlike `strncpy()`, the result is always null-terminated.
+     */
+    ScopedMemoryString(const char * string, size_t length) { CopyString(Alloc(length + 1).Get(), string, length); }
+};
 
 } // namespace Platform
 } // namespace chip
-
-#endif // CHIP_MEM_STRING_H

src/lib/support/tests/TestCHIPArgParser.cpp

@@ -691,11 +691,11 @@ static bool HandleOption(const char * progName, OptionSet * optSet, int id, cons
 
     VerifyOrQuit(sCallbackRecordCount < kMaxCallbackRecords, "Out of callback records");
     sCallbackRecords[sCallbackRecordCount].Type     = CallbackRecord::kHandleOption;
-    sCallbackRecords[sCallbackRecordCount].ProgName = chip::Platform::MemoryAllocString(progName);
+    sCallbackRecords[sCallbackRecordCount].ProgName = chip::Platform::MemoryAllocString(progName, strlen(progName));
     sCallbackRecords[sCallbackRecordCount].OptSet   = optSet;
     sCallbackRecords[sCallbackRecordCount].Id       = id;
-    sCallbackRecords[sCallbackRecordCount].Name     = chip::Platform::MemoryAllocString(name);
-    sCallbackRecords[sCallbackRecordCount].Arg      = (arg != nullptr) ? chip::Platform::MemoryAllocString(arg) : nullptr;
+    sCallbackRecords[sCallbackRecordCount].Name     = chip::Platform::MemoryAllocString(name, strlen(name));
+    sCallbackRecords[sCallbackRecordCount].Arg = (arg != nullptr) ? chip::Platform::MemoryAllocString(arg, strlen(arg)) : nullptr;
     sCallbackRecordCount++;
     return true;
 }
@@ -716,15 +716,15 @@ static bool HandleNonOptionArgs(const char * progName, int argc, char * argv[])
 
     VerifyOrQuit(sCallbackRecordCount < kMaxCallbackRecords, "Out of callback records");
     sCallbackRecords[sCallbackRecordCount].Type     = CallbackRecord::kHandleNonOptionArgs;
-    sCallbackRecords[sCallbackRecordCount].ProgName = chip::Platform::MemoryAllocString(progName);
+    sCallbackRecords[sCallbackRecordCount].ProgName = chip::Platform::MemoryAllocString(progName, strlen(progName));
     sCallbackRecords[sCallbackRecordCount].Argc     = argc;
     sCallbackRecordCount++;
 
     for (int i = 0; i < argc; i++)
     {
         VerifyOrQuit(sCallbackRecordCount < kMaxCallbackRecords, "Out of callback records");
         sCallbackRecords[sCallbackRecordCount].Type = CallbackRecord::kNonOptionArg;
-        sCallbackRecords[sCallbackRecordCount].Arg  = chip::Platform::MemoryAllocString(argv[i]);
+        sCallbackRecords[sCallbackRecordCount].Arg  = chip::Platform::MemoryAllocString(argv[i], strlen(argv[i]));
         sCallbackRecordCount++;
     }
 

src/platform/ESP32/ESP32Config.cpp

@@ -365,21 +365,17 @@ CHIP_ERROR ESP32Config::WriteConfigValueStr(Key key, const char * str)
 CHIP_ERROR ESP32Config::WriteConfigValueStr(Key key, const char * str, size_t strLen)
 {
     CHIP_ERROR err;
-    char * strCopy = NULL;
+    chip::Platform::ScopedMemoryBuffer<char> strCopy;
 
     if (str != NULL)
     {
-        strCopy = chip::Platform::MemoryAllocString(str, strLen);
-        VerifyOrExit(strCopy != NULL, err = CHIP_ERROR_NO_MEMORY);
+        strCopy.Calloc(strLen + 1);
+        VerifyOrExit(strCopy, err = CHIP_ERROR_NO_MEMORY);
+        strncpy(strCopy.Get(), str, strLen);
     }
-
-    err = ESP32Config::WriteConfigValueStr(key, strCopy);
+    err = ESP32Config::WriteConfigValueStr(key, strCopy.Get());
 
 exit:
-    if (strCopy != NULL)
-    {
-        chip::Platform::MemoryFree(strCopy);
-    }
     return err;
 }