From 12747591cbb90ef3735adcf51333738d8712492b Mon Sep 17 00:00:00 2001 From: Kevin Schoedel <67607049+kpschoedel@users.noreply.github.com> Date: Fri, 21 Jan 2022 15:03:15 -0500 Subject: [PATCH] Restore configurability of abnormal termination (#13784) * Restore configurability of abnormal termination #### Problem `CodeUtils.h` builds on `nlassert.h` and contains definitions documented as configuring it, but due to an early bulk rename they don't actually apply. #### Change overview - Rename `CHIP_ASSERT_ABORT` and `CHIP_ASSERT_LOG` back to the original `NL_` forms so that they affect `nlassert.h`. - Rename `ChipDie()` to `CHIP_CONFIG_ABORT()` to avoid confusion with `chipDie()`. This was defined in most `CHIPPlatformConfig.h` files but never actually referenced; now, use it as the lowest-level abnormal termination operation. - Replace direct calls to `abort()` with suitable CodeUtils calls. #### Testing CI, plus local verification (chip-tool+m5stack) with an alternate `CHIP_CONFIG_ABORT()`. * restyle * split chipDie to reduce size while preserving current logging --- examples/minimal-mdns/client.cpp | 7 +-- .../pics/PICSBooleanExpressionParser.cpp | 4 +- .../tests/suites/pics/PICSBooleanReader.cpp | 14 +---- .../CHIPDeviceControllerSystemState.h | 10 +--- src/lib/core/ReferenceCounted.h | 16 ++--- src/lib/support/CHIPMem-Malloc.cpp | 29 +++------- src/lib/support/CodeUtils.h | 58 ++++++++++++------- src/platform/Ameba/CHIPPlatformConfig.h | 2 +- src/platform/Darwin/CHIPPlatformConfig.h | 2 +- src/platform/EFR32/CHIPPlatformConfig.h | 2 +- src/platform/ESP32/CHIPPlatformConfig.h | 2 +- src/platform/Linux/CHIPPlatformConfig.h | 2 +- src/platform/P6/CHIPPlatformConfig.h | 2 +- src/platform/Tizen/CHIPPlatformConfig.h | 2 +- src/platform/android/CHIPPlatformConfig.h | 2 +- src/platform/cc13x2_26x2/CHIPPlatformConfig.h | 2 +- src/platform/mbed/CHIPPlatformConfig.h | 2 +- src/platform/nrfconnect/CHIPPlatformConfig.h | 2 +- .../nxp/k32w/k32w0/CHIPPlatformConfig.h | 2 +- src/platform/qpg/CHIPPlatformConfig.h | 2 +- src/platform/telink/CHIPPlatformConfig.h | 2 +- src/platform/tests/TestDnssd.cpp | 6 +- src/system/TimeSource.h | 6 +- 23 files changed, 73 insertions(+), 105 deletions(-) diff --git a/examples/minimal-mdns/client.cpp b/examples/minimal-mdns/client.cpp index 1a98bc539764df..2bf5191a364912 100644 --- a/examples/minimal-mdns/client.cpp +++ b/examples/minimal-mdns/client.cpp @@ -257,12 +257,7 @@ class QuerySplitter void BroadcastPacket(mdns::Minimal::ServerBase * server) { System::PacketBufferHandle buffer = System::PacketBufferHandle::New(kMdnsMaxPacketSize); - if (buffer.IsNull()) - { - printf("Buffer allocation failure."); - abort(); - return; - } + VerifyOrDie(!buffer.IsNull()); QuerySplitter query; query.Split(gOptions.query); diff --git a/src/app/tests/suites/pics/PICSBooleanExpressionParser.cpp b/src/app/tests/suites/pics/PICSBooleanExpressionParser.cpp index 15263b754ebfb6..7de152f876313d 100644 --- a/src/app/tests/suites/pics/PICSBooleanExpressionParser.cpp +++ b/src/app/tests/suites/pics/PICSBooleanExpressionParser.cpp @@ -124,7 +124,7 @@ bool PICSBooleanExpressionParser::EvaluateExpression(std::vector & else { ChipLogError(chipTool, "Unknown token: '%s'", token.c_str()); - abort(); + chipDie(); } } @@ -139,7 +139,7 @@ bool PICSBooleanExpressionParser::EvaluateSubExpression(std::vector if (tokens[index] != ")") { ChipLogError(chipTool, "Missing ')'"); - abort(); + chipDie(); } index++; diff --git a/src/app/tests/suites/pics/PICSBooleanReader.cpp b/src/app/tests/suites/pics/PICSBooleanReader.cpp index 909f68aa3b4bdf..d1e8252878eb25 100644 --- a/src/app/tests/suites/pics/PICSBooleanReader.cpp +++ b/src/app/tests/suites/pics/PICSBooleanReader.cpp @@ -25,11 +25,7 @@ std::map PICSBooleanReader::Read(std::string filepath) { std::ifstream f(filepath); - if (!f.is_open()) - { - ChipLogError(chipTool, "Error reading: %s", filepath.c_str()); - abort(); - } + VerifyOrDieWithMsg(f.is_open(), chipTool, "Error reading: %s", filepath.c_str()); std::map PICS; std::string line; @@ -46,11 +42,7 @@ std::map PICSBooleanReader::Read(std::string filepath) std::stringstream ss(line); std::getline(ss, key, '='); - if (key.empty()) - { - ChipLogError(chipTool, "Missing PICS key at line %u", lineNumber + 1); - abort(); - } + VerifyOrDieWithMsg(!key.empty(), chipTool, "Missing PICS key at line %u", lineNumber + 1); std::getline(ss, value); if (value == "0") @@ -64,7 +56,7 @@ std::map PICSBooleanReader::Read(std::string filepath) else { ChipLogError(chipTool, "%s: PICS value should be either '0' or '1', got '%s'", key.c_str(), value.c_str()); - abort(); + chipDie(); } lineNumber++; diff --git a/src/controller/CHIPDeviceControllerSystemState.h b/src/controller/CHIPDeviceControllerSystemState.h index 337f212bc65298..6ad01c9376b5f1 100644 --- a/src/controller/CHIPDeviceControllerSystemState.h +++ b/src/controller/CHIPDeviceControllerSystemState.h @@ -96,20 +96,14 @@ class DeviceControllerSystemState DeviceControllerSystemState * Retain() { - if (mRefCount == std::numeric_limits::max()) - { - abort(); - } + VerifyOrDie(mRefCount < std::numeric_limits::max()); ++mRefCount; return this; }; void Release() { - if (mRefCount == 0) - { - abort(); - } + VerifyOrDie(mRefCount > 0); mRefCount--; if (mRefCount == 1) diff --git a/src/lib/core/ReferenceCounted.h b/src/lib/core/ReferenceCounted.h index 3086c902180a19..2d59af288c842c 100644 --- a/src/lib/core/ReferenceCounted.h +++ b/src/lib/core/ReferenceCounted.h @@ -28,6 +28,7 @@ #include #include +#include namespace chip { @@ -51,14 +52,8 @@ class ReferenceCounted /** Adds one to the usage count of this class */ Subclass * Retain() { - if (kInitRefCount && mRefCount == 0) - { - abort(); - } - if (mRefCount == std::numeric_limits::max()) - { - abort(); - } + VerifyOrDie(!kInitRefCount || mRefCount > 0); + VerifyOrDie(mRefCount < std::numeric_limits::max()); ++mRefCount; return static_cast(this); @@ -67,10 +62,7 @@ class ReferenceCounted /** Release usage of this class */ void Release() { - if (mRefCount == 0) - { - abort(); - } + VerifyOrDie(mRefCount != 0); if (--mRefCount == 0) { diff --git a/src/lib/support/CHIPMem-Malloc.cpp b/src/lib/support/CHIPMem-Malloc.cpp index 070378a46511e6..ec247a5a2322f7 100644 --- a/src/lib/support/CHIPMem-Malloc.cpp +++ b/src/lib/support/CHIPMem-Malloc.cpp @@ -25,6 +25,7 @@ #include #include +#include #include @@ -56,32 +57,20 @@ static std::atomic_int memoryInitialized{ 0 }; static void VerifyInitialized(const char * func) { - if (!memoryInitialized) - { - fprintf(stderr, "ABORT: chip::Platform::%s() called before chip::Platform::MemoryInit()\n", func); - abort(); - } + VerifyOrDieWithMsg(memoryInitialized, Support, "ABORT: chip::Platform::%s() called before chip::Platform::MemoryInit()\n", + func); } #define VERIFY_POINTER(p) \ - do \ - if (((p) != nullptr) && (MemoryDebugCheckPointer((p)) == false)) \ - { \ - fprintf(stderr, "ABORT: chip::Platform::%s() found corruption on %p\n", __func__, (p)); \ - abort(); \ - } \ - while (0) + VerifyOrDieWithMsg((p) == nullptr || MemoryDebugCheckPointer((p)), Support, \ + "ABORT: chip::Platform::%s() found corruption on %p\n", __func__, (p)) #endif CHIP_ERROR MemoryAllocatorInit(void * buf, size_t bufSize) { #ifndef NDEBUG - if (memoryInitialized++ > 0) - { - fprintf(stderr, "ABORT: chip::Platform::MemoryInit() called twice.\n"); - abort(); - } + VerifyOrDieWithMsg(memoryInitialized++ == 0, Support, "ABORT: chip::Platform::MemoryInit() called twice.\n"); #endif return CHIP_NO_ERROR; } @@ -89,11 +78,7 @@ CHIP_ERROR MemoryAllocatorInit(void * buf, size_t bufSize) void MemoryAllocatorShutdown() { #ifndef NDEBUG - if (--memoryInitialized < 0) - { - fprintf(stderr, "ABORT: chip::Platform::MemoryShutdown() called twice.\n"); - abort(); - } + VerifyOrDieWithMsg(--memoryInitialized == 0, Support, "ABORT: chip::Platform::MemoryShutdown() called twice.\n"); #endif #if CHIP_CONFIG_MEMORY_DEBUG_DMALLOC dmalloc_shutdown(); diff --git a/src/lib/support/CodeUtils.h b/src/lib/support/CodeUtils.h index 7906ab7d1a6c9a..bbbf95c1c00ca8 100644 --- a/src/lib/support/CodeUtils.h +++ b/src/lib/support/CodeUtils.h @@ -26,13 +26,24 @@ #pragma once -#ifdef __cplusplus - #include #include #include #include +/** + * Base-level abnormal termination. + * + * Terminate the program immediately, without invoking destructors, atexit callbacks, etc. + * Used to implement the default `chipDie()`. + * + * @note + * This should never be invoked directly by code outside this file. + */ +#if !defined(CHIP_CONFIG_ABORT) +#define CHIP_CONFIG_ABORT() abort() +#endif + /** * @name chip-specific nlassert.h Overrides * @@ -41,34 +52,34 @@ */ /** - * @def CHIP_ASSERT_ABORT() + * @def NL_ASSERT_ABORT() * * @brief - * This implements a chip-specific override for #CHIP_ASSERT_ABORT * + * This implements a chip-specific override for #NL_ASSERT_ABORT * * from nlassert.h. * */ -#if !defined(CHIP_ASSERT_ABORT) -#define CHIP_ASSERT_ABORT() chipDie() +#if !defined(NL_ASSERT_ABORT) +#define NL_ASSERT_ABORT() chipAbort() #endif /** - * @def CHIP_ASSERT_LOG(aPrefix, aName, aCondition, aLabel, aFile, aLine, aMessage) + * @def NL_ASSERT_LOG(aPrefix, aName, aCondition, aLabel, aFile, aLine, aMessage) * * @brief - * This implements a chip-specific override for \c CHIP_ASSERT_LOG + * This implements a chip-specific override for \c NL_ASSERT_LOG * from nlassert.h. * * @param[in] aPrefix A pointer to a NULL-terminated C string printed * at the beginning of the logged assertion * message. Typically this is and should be - * \c CHIP_ASSERT_PREFIX_STRING. + * \c NL_ASSERT_PREFIX_STRING. * @param[in] aName A pointer to a NULL-terminated C string printed * following @a aPrefix that indicates what * module, program, application or subsystem * the assertion occurred in Typically this * is and should be - * \c CHIP_ASSERT_COMPONENT_STRING. + * \c NL_ASSERT_COMPONENT_STRING. * @param[in] aCondition A pointer to a NULL-terminated C string indicating * the expression that evaluated to false in * the assertion. Typically this is a @@ -93,12 +104,12 @@ * */ // clang-format off -#if !defined(CHIP_ASSERT_LOG) -#define CHIP_ASSERT_LOG(aPrefix, aName, aCondition, aLabel, aFile, aLine, aMessage) \ +#if !defined(NL_ASSERT_LOG) +#define NL_ASSERT_LOG(aPrefix, aName, aCondition, aLabel, aFile, aLine, aMessage) \ do \ { \ ChipLogError(NotSpecified, \ - CHIP_ASSERT_LOG_FORMAT_DEFAULT, \ + NL_ASSERT_LOG_FORMAT_DEFAULT, \ aPrefix, \ (((aName) == 0) || (*(aName) == '\0')) ? "" : aName, \ (((aName) == 0) || (*(aName) == '\0')) ? "" : ": ", \ @@ -455,18 +466,25 @@ constexpr inline const _T & max(const _T & a, const _T & b) * @endcode * */ +#ifndef chipAbort +extern "C" void chipAbort(void) __attribute((noreturn)); + +inline void chipAbort(void) +{ + while (true) + { + // NL_ASSERT_ABORT is redefined to be chipAbort, so not useful here. + CHIP_CONFIG_ABORT(); + } +} +#endif // chipAbort #ifndef chipDie extern "C" void chipDie(void) __attribute((noreturn)); inline void chipDie(void) { ChipLogError(NotSpecified, "chipDie chipDie chipDie"); - - while (true) - { - // CHIP_ASSERT_ABORT is redefined to be chipDie, so not useful here. - abort(); - } + chipAbort(); } #endif // chipDie @@ -631,8 +649,6 @@ inline void chipDie(void) #define FALLTHROUGH (void) 0 #endif -#endif // __cplusplus - /** * @def ArraySize(aArray) * diff --git a/src/platform/Ameba/CHIPPlatformConfig.h b/src/platform/Ameba/CHIPPlatformConfig.h index 68099f9cb17442..c4702c67fac9d9 100644 --- a/src/platform/Ameba/CHIPPlatformConfig.h +++ b/src/platform/Ameba/CHIPPlatformConfig.h @@ -40,7 +40,7 @@ #define CHIP_CONFIG_TIME_ENABLE_CLIENT 1 #define CHIP_CONFIG_TIME_ENABLE_SERVER 0 -#define ChipDie() abort() +#define CHIP_CONFIG_ABORT() abort() // ==================== Security Adaptations ==================== diff --git a/src/platform/Darwin/CHIPPlatformConfig.h b/src/platform/Darwin/CHIPPlatformConfig.h index f1778c437dc984..5ced752436a19d 100644 --- a/src/platform/Darwin/CHIPPlatformConfig.h +++ b/src/platform/Darwin/CHIPPlatformConfig.h @@ -25,7 +25,7 @@ // ==================== General Platform Adaptations ==================== -#define ChipDie() abort() +#define CHIP_CONFIG_ABORT() abort() // TODO:(#756) Add FabricState support #define CHIP_CONFIG_ENABLE_FABRIC_STATE 0 diff --git a/src/platform/EFR32/CHIPPlatformConfig.h b/src/platform/EFR32/CHIPPlatformConfig.h index bf6de056644859..11fb925012f204 100644 --- a/src/platform/EFR32/CHIPPlatformConfig.h +++ b/src/platform/EFR32/CHIPPlatformConfig.h @@ -28,7 +28,7 @@ // ==================== General Platform Adaptations ==================== -#define ChipDie() abort() +#define CHIP_CONFIG_ABORT() abort() #define CHIP_CONFIG_PERSISTED_STORAGE_KEY_TYPE uint8_t #define CHIP_CONFIG_PERSISTED_STORAGE_ENC_MSG_CNTR_ID 1 diff --git a/src/platform/ESP32/CHIPPlatformConfig.h b/src/platform/ESP32/CHIPPlatformConfig.h index aadc79461d67ef..5c4232e0281dec 100644 --- a/src/platform/ESP32/CHIPPlatformConfig.h +++ b/src/platform/ESP32/CHIPPlatformConfig.h @@ -45,7 +45,7 @@ #define CHIP_CONFIG_TIME_ENABLE_CLIENT 1 #define CHIP_CONFIG_TIME_ENABLE_SERVER 0 -#define ChipDie() abort() +#define CHIP_CONFIG_ABORT() abort() // ==================== Security Adaptations ==================== diff --git a/src/platform/Linux/CHIPPlatformConfig.h b/src/platform/Linux/CHIPPlatformConfig.h index fa8cf092f3c390..3dc45ac9bcdf09 100644 --- a/src/platform/Linux/CHIPPlatformConfig.h +++ b/src/platform/Linux/CHIPPlatformConfig.h @@ -25,7 +25,7 @@ // ==================== General Platform Adaptations ==================== -#define ChipDie() abort() +#define CHIP_CONFIG_ABORT() abort() // TODO:(#756) Add FabricState support #define CHIP_CONFIG_ENABLE_FABRIC_STATE 0 diff --git a/src/platform/P6/CHIPPlatformConfig.h b/src/platform/P6/CHIPPlatformConfig.h index 7622576cf71141..59f48dc144d61e 100644 --- a/src/platform/P6/CHIPPlatformConfig.h +++ b/src/platform/P6/CHIPPlatformConfig.h @@ -48,7 +48,7 @@ #define CHIP_CONFIG_TIME_ENABLE_CLIENT 1 #define CHIP_CONFIG_TIME_ENABLE_SERVER 0 -#define ChipDie() abort() +#define CHIP_CONFIG_ABORT() abort() // ==================== Security Adaptations ==================== diff --git a/src/platform/Tizen/CHIPPlatformConfig.h b/src/platform/Tizen/CHIPPlatformConfig.h index e1b95a71fb0a82..796ef5369784d5 100644 --- a/src/platform/Tizen/CHIPPlatformConfig.h +++ b/src/platform/Tizen/CHIPPlatformConfig.h @@ -26,7 +26,7 @@ // ==================== General Platform Adaptations ==================== -#define ChipDie() abort() +#define CHIP_CONFIG_ABORT() abort() #define CHIP_CONFIG_ENABLE_FABRIC_STATE 0 diff --git a/src/platform/android/CHIPPlatformConfig.h b/src/platform/android/CHIPPlatformConfig.h index 97ae7648bdbfe1..3be5f4a7aa308d 100644 --- a/src/platform/android/CHIPPlatformConfig.h +++ b/src/platform/android/CHIPPlatformConfig.h @@ -25,7 +25,7 @@ // ==================== General Platform Adaptations ==================== -#define ChipDie() abort() +#define CHIP_CONFIG_ABORT() abort() // TODO:(#756) Add FabricState support #define CHIP_CONFIG_ENABLE_FABRIC_STATE 0 diff --git a/src/platform/cc13x2_26x2/CHIPPlatformConfig.h b/src/platform/cc13x2_26x2/CHIPPlatformConfig.h index 4d6fec8a2309dd..674bd48d6401d0 100644 --- a/src/platform/cc13x2_26x2/CHIPPlatformConfig.h +++ b/src/platform/cc13x2_26x2/CHIPPlatformConfig.h @@ -30,7 +30,7 @@ // ==================== General Platform Adaptations ==================== -#define ChipDie() assert() +#define CHIP_CONFIG_ABORT() assert() #define CHIP_CONFIG_PERSISTED_STORAGE_KEY_TYPE uint16_t #define CHIP_CONFIG_PERSISTED_STORAGE_ENC_MSG_CNTR_ID 1 diff --git a/src/platform/mbed/CHIPPlatformConfig.h b/src/platform/mbed/CHIPPlatformConfig.h index 5e2fdb071d885a..ad40d27b6955bc 100644 --- a/src/platform/mbed/CHIPPlatformConfig.h +++ b/src/platform/mbed/CHIPPlatformConfig.h @@ -28,7 +28,7 @@ // ==================== General Platform Adaptations ==================== -#define ChipDie() abort() +#define CHIP_CONFIG_ABORT() abort() #define CHIP_CONFIG_PERSISTED_STORAGE_KEY_TYPE const char * #define CHIP_CONFIG_PERSISTED_STORAGE_ENC_MSG_CNTR_ID 1 diff --git a/src/platform/nrfconnect/CHIPPlatformConfig.h b/src/platform/nrfconnect/CHIPPlatformConfig.h index 2c6e5054a5b091..fb0446cb4557e8 100644 --- a/src/platform/nrfconnect/CHIPPlatformConfig.h +++ b/src/platform/nrfconnect/CHIPPlatformConfig.h @@ -25,7 +25,7 @@ // ==================== General Platform Adaptations ==================== -#define ChipDie() abort() +#define CHIP_CONFIG_ABORT() abort() #define CHIP_CONFIG_PERSISTED_STORAGE_KEY_TYPE const char * #define CHIP_CONFIG_PERSISTED_STORAGE_MAX_KEY_LENGTH 2 diff --git a/src/platform/nxp/k32w/k32w0/CHIPPlatformConfig.h b/src/platform/nxp/k32w/k32w0/CHIPPlatformConfig.h index dca1ba32ec34ec..ce299ce535152a 100644 --- a/src/platform/nxp/k32w/k32w0/CHIPPlatformConfig.h +++ b/src/platform/nxp/k32w/k32w0/CHIPPlatformConfig.h @@ -29,7 +29,7 @@ // ==================== General Platform Adaptations ==================== -#define ChipDie() abort() +#define CHIP_CONFIG_ABORT() abort() #define CHIP_CONFIG_PERSISTED_STORAGE_KEY_TYPE uint16_t #define CHIP_CONFIG_PERSISTED_STORAGE_ENC_MSG_CNTR_ID 1 diff --git a/src/platform/qpg/CHIPPlatformConfig.h b/src/platform/qpg/CHIPPlatformConfig.h index f36d4adecc06a8..32c53f2b1bcc66 100644 --- a/src/platform/qpg/CHIPPlatformConfig.h +++ b/src/platform/qpg/CHIPPlatformConfig.h @@ -25,7 +25,7 @@ // ==================== General Platform Adaptations ==================== -#define ChipDie() abort() +#define CHIP_CONFIG_ABORT() abort() #define CHIP_CONFIG_ENABLE_TUNNELING 0 #define CHIP_CONFIG_MAX_TUNNELS 0 diff --git a/src/platform/telink/CHIPPlatformConfig.h b/src/platform/telink/CHIPPlatformConfig.h index 954f3d42f15af1..4b095a0babd78e 100644 --- a/src/platform/telink/CHIPPlatformConfig.h +++ b/src/platform/telink/CHIPPlatformConfig.h @@ -25,7 +25,7 @@ // ==================== General Platform Adaptations ==================== -#define ChipDie() abort() +#define CHIP_CONFIG_ABORT() abort() #define CHIP_CONFIG_PERSISTED_STORAGE_KEY_TYPE const char * #define CHIP_CONFIG_PERSISTED_STORAGE_MAX_KEY_LENGTH 2 diff --git a/src/platform/tests/TestDnssd.cpp b/src/platform/tests/TestDnssd.cpp index 6baa99c1cd7559..c34c0ced39747e 100644 --- a/src/platform/tests/TestDnssd.cpp +++ b/src/platform/tests/TestDnssd.cpp @@ -77,11 +77,7 @@ static void InitCallback(void * context, CHIP_ERROR error) static void ErrorCallback(void * context, CHIP_ERROR error) { - if (error != CHIP_NO_ERROR) - { - fprintf(stderr, "Mdns error: %" CHIP_ERROR_FORMAT "\n", error.Format()); - abort(); - } + VerifyOrDieWithMsg(error == CHIP_NO_ERROR, DeviceLayer, "Mdns error: %" CHIP_ERROR_FORMAT "\n", error.Format()); } void TestDnssdPubSub(nlTestSuite * inSuite, void * inContext) diff --git a/src/system/TimeSource.h b/src/system/TimeSource.h index 9fe5c383f76524..64fa436337694f 100644 --- a/src/system/TimeSource.h +++ b/src/system/TimeSource.h @@ -22,6 +22,7 @@ #pragma once +#include #include #include @@ -77,10 +78,7 @@ class TimeSource void SetMonotonicTimestamp(System::Clock::Timestamp value) { - if (value < mCurrentTime) - { - abort(); - } + VerifyOrDie(value >= mCurrentTime); mCurrentTime = value; }