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 <[email protected]> * 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 <[email protected]> --------- Signed-off-by: Adrian Gielniewski <[email protected]>
project-chip · Feb 4, 2025 · 0d27f42 · 0d27f42
1 parent 3700a5a
commit 0d27f42
Show file tree

Hide file tree

Showing 7 changed files with 44 additions and 3 deletions.
diff --git 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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -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
@@ -100,6 +100,9 @@ class TestSystemPacketBuffer : public ::testing::Test
     {
         chip::DeviceLayer::PlatformMgr().Shutdown();
         chip::Platform::MemoryShutdown();
+
+        // Deregister the layer error formatter
+        DeregisterCHIPLayerErrorFormatter();
     }
 
     void SetUp()