From 0d27f42b6a97b9af349a105bddfa9ba84b81e295 Mon Sep 17 00:00:00 2001 From: Adrian Gielniewski Date: Tue, 4 Feb 2025 04:47:47 +0100 Subject: [PATCH] Fix deregistering error formatter (#37254) * Fix deregistering error formatter * Don't dereference address pointed by lfp in for loop expression as it may be a nullptr. * Add simple test that registers and deregisters single error formatter. * Deregister all formatters at the end of CheckRegisterDeregisterErrorFormatter. Signed-off-by: Adrian Gielniewski * Fix error formatter tests * Add function to deregister CHIP layer error formatter. * Deregister CHIP error formatter at the end of tests to not interfere with other tests. Signed-off-by: Adrian Gielniewski --------- Signed-off-by: Adrian Gielniewski --- src/lib/core/CHIPError.cpp | 12 ++++++++++-- src/lib/core/CHIPError.h | 1 + src/lib/core/ErrorStr.cpp | 6 +++++- src/lib/core/tests/TestCHIPErrorStr.cpp | 12 ++++++++++++ src/lib/support/tests/TestErrorStr.cpp | 10 ++++++++++ src/system/tests/TestSystemErrorStr.cpp | 3 +++ src/system/tests/TestSystemPacketBuffer.cpp | 3 +++ 7 files changed, 44 insertions(+), 3 deletions(-) diff --git a/src/lib/core/CHIPError.cpp b/src/lib/core/CHIPError.cpp index 31959718b94380..2068a9fbc68c42 100644 --- a/src/lib/core/CHIPError.cpp +++ b/src/lib/core/CHIPError.cpp @@ -22,16 +22,24 @@ namespace chip { +static ErrorFormatter sCHIPErrorFormatter = { FormatCHIPError, nullptr }; + /** * Register a text error formatter for CHIP core errors. */ void RegisterCHIPLayerErrorFormatter() { - static ErrorFormatter sCHIPErrorFormatter = { FormatCHIPError, nullptr }; - RegisterErrorFormatter(&sCHIPErrorFormatter); } +/** + * Deregister a text error formatter for CHIP core errors. + */ +void DeregisterCHIPLayerErrorFormatter() +{ + DeregisterErrorFormatter(&sCHIPErrorFormatter); +} + /** * Given a CHIP error, returns a human-readable NULL-terminated C string * describing the error. diff --git a/src/lib/core/CHIPError.h b/src/lib/core/CHIPError.h index 0a002d89c088f5..03217378eecb87 100644 --- a/src/lib/core/CHIPError.h +++ b/src/lib/core/CHIPError.h @@ -1821,6 +1821,7 @@ using CHIP_ERROR = ::chip::ChipError; namespace chip { extern void RegisterCHIPLayerErrorFormatter(); +extern void DeregisterCHIPLayerErrorFormatter(); extern bool FormatCHIPError(char * buf, uint16_t bufSize, CHIP_ERROR err); } // namespace chip diff --git a/src/lib/core/ErrorStr.cpp b/src/lib/core/ErrorStr.cpp index b3cf968c674dd5..13335b12fe214c 100644 --- a/src/lib/core/ErrorStr.cpp +++ b/src/lib/core/ErrorStr.cpp @@ -142,13 +142,17 @@ DLL_EXPORT void RegisterErrorFormatter(ErrorFormatter * errFormatter) DLL_EXPORT void DeregisterErrorFormatter(ErrorFormatter * errFormatter) { // Remove the formatter if present - for (ErrorFormatter ** lfp = &sErrorFormatterList; *lfp != nullptr; lfp = &(*lfp)->Next) + for (ErrorFormatter ** lfp = &sErrorFormatterList; *lfp != nullptr;) { // Remove the formatter from the global list, if found. if (*lfp == errFormatter) { *lfp = errFormatter->Next; } + else + { + lfp = &(*lfp)->Next; + } } } diff --git a/src/lib/core/tests/TestCHIPErrorStr.cpp b/src/lib/core/tests/TestCHIPErrorStr.cpp index 1ce158cc8594ae..a65d45875becae 100644 --- a/src/lib/core/tests/TestCHIPErrorStr.cpp +++ b/src/lib/core/tests/TestCHIPErrorStr.cpp @@ -207,6 +207,9 @@ TEST(TestCHIPErrorStr, CheckCoreErrorStr) // ErrorStr with static char array. CheckCoreErrorStrHelper(ErrorStr(err, /*withSourceLocation=*/true), err); } + + // Deregister the layer error formatter + DeregisterCHIPLayerErrorFormatter(); } TEST(TestCHIPErrorStr, CheckCoreErrorStrStorage) @@ -222,6 +225,9 @@ TEST(TestCHIPErrorStr, CheckCoreErrorStrStorage) ErrorStrStorage storage; CheckCoreErrorStrHelper(ErrorStr(err, /*withSourceLocation=*/true, storage), err); } + + // Deregister the layer error formatter + DeregisterCHIPLayerErrorFormatter(); } void CheckCoreErrorStrWithoutSourceLocationHelper(const char * errStr, CHIP_ERROR err) @@ -258,6 +264,9 @@ TEST(TestCHIPErrorStr, CheckCoreErrorStrWithoutSourceLocation) // ErrorStr with static char array. CheckCoreErrorStrWithoutSourceLocationHelper(ErrorStr(err, /*withSourceLocation=*/false), err); } + + // Deregister the layer error formatter + DeregisterCHIPLayerErrorFormatter(); } TEST(TestCHIPErrorStr, CheckCoreErrorStrStorageWithoutSourceLocation) @@ -273,4 +282,7 @@ TEST(TestCHIPErrorStr, CheckCoreErrorStrStorageWithoutSourceLocation) ErrorStrStorage storage; CheckCoreErrorStrWithoutSourceLocationHelper(ErrorStr(err, /*withSourceLocation=*/false, storage), err); } + + // Deregister the layer error formatter + DeregisterCHIPLayerErrorFormatter(); } diff --git a/src/lib/support/tests/TestErrorStr.cpp b/src/lib/support/tests/TestErrorStr.cpp index ac8a9f5e9e3469..99e667efe96e17 100644 --- a/src/lib/support/tests/TestErrorStr.cpp +++ b/src/lib/support/tests/TestErrorStr.cpp @@ -67,6 +67,14 @@ static bool trueFormat(char * buf, uint16_t bufSize, CHIP_ERROR err) return true; // means I handled it } +TEST(TestErrorStr, CheckRegisterDeregisterSingleErrorFormatter) +{ + static ErrorFormatter falseFormatter = { falseFormat, nullptr }; + + RegisterErrorFormatter(&falseFormatter); + DeregisterErrorFormatter(&falseFormatter); +} + TEST(TestErrorStr, CheckRegisterDeregisterErrorFormatter) { static ErrorFormatter falseFormatter = { falseFormat, nullptr }; @@ -114,6 +122,8 @@ TEST(TestErrorStr, CheckRegisterDeregisterErrorFormatter) // verify this doesn't crash DeregisterErrorFormatter(&trueFormatter); + DeregisterErrorFormatter(&falseFormatter); + DeregisterErrorFormatter(&falseFormatter2); } TEST(TestErrorStr, CheckNoError) diff --git a/src/system/tests/TestSystemErrorStr.cpp b/src/system/tests/TestSystemErrorStr.cpp index 32d4870c9b40da..ed78a7b1caf533 100644 --- a/src/system/tests/TestSystemErrorStr.cpp +++ b/src/system/tests/TestSystemErrorStr.cpp @@ -69,4 +69,7 @@ TEST(TestSystemErrorStr, CheckSystemErrorStr) EXPECT_NE(strchr(errStr, ':'), nullptr); #endif // !CHIP_CONFIG_SHORT_ERROR_STR } + + // Deregister the layer error formatter + DeregisterCHIPLayerErrorFormatter(); } diff --git a/src/system/tests/TestSystemPacketBuffer.cpp b/src/system/tests/TestSystemPacketBuffer.cpp index bdfa771888bd92..b1f939d03f11c8 100644 --- a/src/system/tests/TestSystemPacketBuffer.cpp +++ b/src/system/tests/TestSystemPacketBuffer.cpp @@ -100,6 +100,9 @@ class TestSystemPacketBuffer : public ::testing::Test { chip::DeviceLayer::PlatformMgr().Shutdown(); chip::Platform::MemoryShutdown(); + + // Deregister the layer error formatter + DeregisterCHIPLayerErrorFormatter(); } void SetUp()