Fix deregistering error formatter (project-chip#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 <adrian.gielniewski@nordicsemi.no>

* 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 <adrian.gielniewski@nordicsemi.no>

---------

Signed-off-by: Adrian Gielniewski <adrian.gielniewski@nordicsemi.no>

Author: adigie

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.

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

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;
+        }
     }
 }
 

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();
 }

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)

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();
 }

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()