Fix Logging When Trying to Log Nullptr To Strings (#23604)

This PR attempts to identify all cases where %s specifiers in the logging APIs (ChipLogError(), ChipLogProgress(), ChipLogDetail()) don't have a guaranteed non-null string parameter. In all identified cases the issue is fixed using StringOrNullMarker() helper method to guarantee it doesn't happen.
project-chip · Nov 17, 2022 · 406bdfe · 406bdfe
1 parent 361c74d
commit 406bdfe
Show file tree

Hide file tree

Showing 48 changed files with 267 additions and 201 deletions.
diff --git a/src/app/tests/suites/credentials/TestHarnessDACProvider.cpp b/src/app/tests/suites/credentials/TestHarnessDACProvider.cpp
@@ -147,15 +147,15 @@ void TestHarnessDACProvider::Init(const char * filepath)
     std::ifstream json(filepath, std::ifstream::binary);
     if (!json)
     {
-        ChipLogError(AppServer, "Error opening json file: %s", filepath);
+        ChipLogError(AppServer, "Error opening json file: %s", StringOrNullMarker(filepath));
         return;
     }
 
     Json::Reader reader;
     Json::Value root;
     if (!reader.parse(json, root))
     {
-        ChipLogError(AppServer, "Error parsing json file: %s", filepath);
+        ChipLogError(AppServer, "Error parsing json file: %s", StringOrNullMarker(filepath));
         return;
     }
 

diff --git a/src/app/tests/suites/include/ConstraintsChecker.h b/src/app/tests/suites/include/ConstraintsChecker.h
@@ -46,7 +46,8 @@ class ConstraintsChecker
 
     bool CheckConstraintFormat(const char * itemName, const char * current, const char * expected)
     {
-        ChipLogError(chipTool, "Warning: %s format checking is not implemented yet. Expected format: '%s'", itemName, expected);
+        ChipLogError(chipTool, "Warning: %s format checking is not implemented yet. Expected format: '%s'",
+                     StringOrNullMarker(itemName), StringOrNullMarker(expected));
         return true;
     }
 

diff --git a/src/app/tests/suites/include/PICSChecker.h b/src/app/tests/suites/include/PICSChecker.h
@@ -40,7 +40,7 @@ class PICSChecker
         bool shouldSkip = !PICSBooleanExpressionParser::Eval(expression, pics);
         if (shouldSkip)
         {
-            ChipLogProgress(chipTool, " **** Skipping: %s == false\n", expression);
+            ChipLogProgress(chipTool, " **** Skipping: %s == false\n", StringOrNullMarker(expression));
         }
         return shouldSkip;
     }

diff --git a/src/app/tests/suites/include/TestRunner.h b/src/app/tests/suites/include/TestRunner.h
@@ -30,11 +30,11 @@ class TestRunner
     TestRunner(const char * name, uint16_t testCount) : mTestName(name), mTestCount(testCount), mTestIndex(0) {}
     virtual ~TestRunner(){};
 
-    void LogStart() { ChipLogProgress(chipTool, " ***** Test Start : %s\n", mTestName); }
+    void LogStart() { ChipLogProgress(chipTool, " ***** Test Start : %s\n", StringOrNullMarker(mTestName)); }
 
     void LogStep(uint32_t stepNumber, const char * stepName)
     {
-        ChipLogProgress(chipTool, " ***** Test Step %u : %s\n", stepNumber, stepName);
+        ChipLogProgress(chipTool, " ***** Test Step %u : %s\n", stepNumber, StringOrNullMarker(stepName));
     }
 
     void LogEnd(std::string message, CHIP_ERROR err)

diff --git a/src/controller/java/AndroidDeviceControllerWrapper.cpp b/src/controller/java/AndroidDeviceControllerWrapper.cpp
@@ -649,7 +649,7 @@ void AndroidDeviceControllerWrapper::OnScanNetworksFailure(CHIP_ERROR error)
 
 CHIP_ERROR AndroidDeviceControllerWrapper::SyncGetKeyValue(const char * key, void * value, uint16_t & size)
 {
-    ChipLogProgress(chipTool, "KVS: Getting key %s", key);
+    ChipLogProgress(chipTool, "KVS: Getting key %s", StringOrNullMarker(key));
 
     size_t read_size = 0;
 
@@ -662,12 +662,12 @@ CHIP_ERROR AndroidDeviceControllerWrapper::SyncGetKeyValue(const char * key, voi
 
 CHIP_ERROR AndroidDeviceControllerWrapper::SyncSetKeyValue(const char * key, const void * value, uint16_t size)
 {
-    ChipLogProgress(chipTool, "KVS: Setting key %s", key);
+    ChipLogProgress(chipTool, "KVS: Setting key %s", StringOrNullMarker(key));
     return chip::DeviceLayer::PersistedStorage::KeyValueStoreMgr().Put(key, value, size);
 }
 
 CHIP_ERROR AndroidDeviceControllerWrapper::SyncDeleteKeyValue(const char * key)
 {
-    ChipLogProgress(chipTool, "KVS: Deleting key %s", key);
+    ChipLogProgress(chipTool, "KVS: Deleting key %s", StringOrNullMarker(key));
     return chip::DeviceLayer::PersistedStorage::KeyValueStoreMgr().Delete(key);
 }
diff --git a/src/controller/python/ChipDeviceController-StorageDelegate.cpp b/src/controller/python/ChipDeviceController-StorageDelegate.cpp
@@ -1,7 +1,6 @@
-
 /*
  *
- *    Copyright (c) 2021 Project CHIP Authors
+ *    Copyright (c) 2021-2022 Project CHIP Authors
  *    All rights reserved.
  *
  *    Licensed under the Apache License, Version 2.0 (the "License");
@@ -58,7 +57,7 @@ CHIP_ERROR PythonPersistentStorageDelegate::SyncGetKeyValue(const char * key, vo
 CHIP_ERROR PythonPersistentStorageDelegate::SyncSetKeyValue(const char * key, const void * value, uint16_t size)
 {
     mStorage[key] = std::string(static_cast<const char *>(value), size);
-    ChipLogDetail(Controller, "SyncSetKeyValue on %s", key);
+    ChipLogDetail(Controller, "SyncSetKeyValue on %s", StringOrNullMarker(key));
 
     return CHIP_NO_ERROR;
 }
@@ -79,7 +78,7 @@ namespace Python {
 
 CHIP_ERROR StorageAdapter::SyncGetKeyValue(const char * key, void * value, uint16_t & size)
 {
-    ChipLogDetail(Controller, "StorageAdapter::GetKeyValue: Key = %s, Value = %p (%u)", key, value, size);
+    ChipLogDetail(Controller, "StorageAdapter::GetKeyValue: Key = %s, Value = %p (%u)", StringOrNullMarker(key), value, size);
     if ((value == nullptr) && (size != 0))
     {
         return CHIP_ERROR_INVALID_ARGUMENT;
@@ -109,7 +108,7 @@ CHIP_ERROR StorageAdapter::SyncGetKeyValue(const char * key, void * value, uint1
 CHIP_ERROR StorageAdapter::SyncSetKeyValue(const char * key, const void * value, uint16_t size)
 {
     ReturnErrorCodeIf(((value == nullptr) && (size != 0)), CHIP_ERROR_INVALID_ARGUMENT);
-    ChipLogDetail(Controller, "StorageAdapter::SetKeyValue: Key = %s, Value = %p (%u)", key, value, size);
+    ChipLogDetail(Controller, "StorageAdapter::SetKeyValue: Key = %s, Value = %p (%u)", StringOrNullMarker(key), value, size);
     mSetKeyCb(mContext, key, value, size);
     return CHIP_NO_ERROR;
 }
@@ -124,7 +123,7 @@ CHIP_ERROR StorageAdapter::SyncDeleteKeyValue(const char * key)
         return err;
     }
 
-    ChipLogDetail(Controller, "StorageAdapter::DeleteKeyValue: Key = %s", key);
+    ChipLogDetail(Controller, "StorageAdapter::DeleteKeyValue: Key = %s", StringOrNullMarker(key));
     mDeleteKeyCb(mContext, key);
     return CHIP_NO_ERROR;
 }

diff --git a/src/credentials/LastKnownGoodTime.cpp b/src/credentials/LastKnownGoodTime.cpp
@@ -44,7 +44,7 @@ void LastKnownGoodTime::LogTime(const char * msg, System::Clock::Seconds32 chipE
     uint8_t second;
     ChipEpochToCalendarTime(chipEpochTime.count(), year, month, day, hour, minute, second);
     snprintf(buf, sizeof(buf), "%04u-%02u-%02uT%02u:%02u:%02u", year, month, day, hour, minute, second);
-    ChipLogProgress(TimeService, "%s%s", msg, buf);
+    ChipLogProgress(TimeService, "%s%s", StringOrNullMarker(msg), buf);
 }
 
 CHIP_ERROR LastKnownGoodTime::LoadLastKnownGoodChipEpochTime(System::Clock::Seconds32 & lastKnownGoodChipEpochTime) const

diff --git a/src/crypto/CHIPCryptoPALOpenSSL.cpp b/src/crypto/CHIPCryptoPALOpenSSL.cpp
@@ -115,7 +115,8 @@ static void _logSSLError()
         const char * err_str_reason  = ERR_reason_error_string(static_cast<libssl_err_type>(ssl_err_code));
         if (err_str_lib)
         {
-            ChipLogError(Crypto, " ssl err  %s %s %s\n", err_str_lib, err_str_routine, err_str_reason);
+            ChipLogError(Crypto, " ssl err  %s %s %s\n", StringOrNullMarker(err_str_lib), StringOrNullMarker(err_str_routine),
+                         StringOrNullMarker(err_str_reason));
         }
 #endif // CHIP_ERROR_LOGGING
         ssl_err_code = ERR_get_error();

diff --git a/src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp b/src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp
@@ -1,6 +1,6 @@
 /*
  *
- *    Copyright (c) 2020 Project CHIP Authors
+ *    Copyright (c) 2020-2022 Project CHIP Authors
  *
  *    Licensed under the Apache License, Version 2.0 (the "License");
  *    you may not use this file except in compliance with the License.
@@ -531,7 +531,8 @@ CHIP_ERROR AdvertiserMinMdns::Advertise(const OperationalAdvertisingParameters &
 
     AdvertiseRecords(BroadcastAdvertiseType::kStarted);
 
-    ChipLogProgress(Discovery, "mDNS service published: %s.%s", instanceName.names[1], instanceName.names[2]);
+    ChipLogProgress(Discovery, "mDNS service published: %s.%s", StringOrNullMarker(instanceName.names[1]),
+                    StringOrNullMarker(instanceName.names[2]));
 
     return CHIP_NO_ERROR;
 }
@@ -725,17 +726,18 @@ CHIP_ERROR AdvertiserMinMdns::Advertise(const CommissionAdvertisingParameters &
     if (params.GetCommissionAdvertiseMode() == CommssionAdvertiseMode::kCommissionableNode)
     {
         ChipLogProgress(Discovery, "CHIP minimal mDNS configured as 'Commissionable node device'; instance name: %s.",
-                        instanceName.names[0]);
+                        StringOrNullMarker(instanceName.names[0]));
     }
     else
     {
         ChipLogProgress(Discovery, "CHIP minimal mDNS configured as 'Commissioner device'; instance name: %s.",
-                        instanceName.names[0]);
+                        StringOrNullMarker(instanceName.names[0]));
     }
 
     AdvertiseRecords(BroadcastAdvertiseType::kStarted);
 
-    ChipLogProgress(Discovery, "mDNS service published: %s.%s", instanceName.names[1], instanceName.names[2]);
+    ChipLogProgress(Discovery, "mDNS service published: %s.%s", StringOrNullMarker(instanceName.names[1]),
+                    StringOrNullMarker(instanceName.names[2]));
 
     return CHIP_NO_ERROR;
 }

diff --git a/src/lib/dnssd/Discovery_ImplPlatform.cpp b/src/lib/dnssd/Discovery_ImplPlatform.cpp
@@ -466,7 +466,8 @@ void DiscoveryImplPlatform::HandleDnssdPublish(void * context, const char * type
 {
     if (CHIP_NO_ERROR == error)
     {
-        ChipLogProgress(Discovery, "mDNS service published: %s; instance name: %s", type, instanceName);
+        ChipLogProgress(Discovery, "mDNS service published: %s; instance name: %s", StringOrNullMarker(type),
+                        StringOrNullMarker(instanceName));
     }
     else
     {
@@ -520,12 +521,13 @@ CHIP_ERROR DiscoveryImplPlatform::PublishService(const char * serviceType, TextE
 
     for (size_t i = 0; i < textEntrySize; i++)
     {
-        printf(" entry [%u] : %s %s\n", static_cast<unsigned int>(i), textEntries[i].mKey, (char *) (textEntries[i].mData));
+        printf(" entry [%u] : %s %s\n", static_cast<unsigned int>(i), StringOrNullMarker(textEntries[i].mKey),
+               StringOrNullMarker((char *) (textEntries[i].mData)));
     }
 
     for (size_t i = 0; i < subTypeSize; i++)
     {
-        printf(" type [%u] : %s\n", static_cast<unsigned int>(i), subTypes[i]);
+        printf(" type [%u] : %s\n", static_cast<unsigned int>(i), StringOrNullMarker(subTypes[i]));
     }
 #endif
 

diff --git a/src/lib/support/JniReferences.cpp b/src/lib/support/JniReferences.cpp
@@ -1,6 +1,6 @@
 /*
  *
- *    Copyright (c) 2021 Project CHIP Authors
+ *    Copyright (c) 2021-2022 Project CHIP Authors
  *
  *    Licensed under the Apache License, Version 2.0 (the "License");
  *    you may not use this file except in compliance with the License.
@@ -145,7 +145,7 @@ void JniReferences::ReportError(JNIEnv * env, CHIP_ERROR cbErr, const char * fun
 {
     if (cbErr == CHIP_JNI_ERROR_EXCEPTION_THROWN)
     {
-        ChipLogError(Support, "Java exception thrown in %s", functName);
+        ChipLogError(Support, "Java exception thrown in %s", StringOrNullMarker(functName));
         env->ExceptionDescribe();
     }
     else
@@ -166,7 +166,7 @@ void JniReferences::ReportError(JNIEnv * env, CHIP_ERROR cbErr, const char * fun
             errStr = ErrorStr(cbErr);
             break;
         }
-        ChipLogError(Support, "Error in %s : %s", functName, errStr);
+        ChipLogError(Support, "Error in %s : %s", StringOrNullMarker(functName), errStr);
     }
 }
 

diff --git a/src/lib/support/TestPersistentStorageDelegate.h b/src/lib/support/TestPersistentStorageDelegate.h
@@ -1,6 +1,6 @@
 /*
  *
- *    Copyright (c) 2021 Project CHIP Authors
+ *    Copyright (c) 2021-2022 Project CHIP Authors
  *    All rights reserved.
  *
  *    Licensed under the Apache License, Version 2.0 (the "License");
@@ -57,7 +57,7 @@ class TestPersistentStorageDelegate : public PersistentStorageDelegate
     {
         if (mLoggingLevel >= LoggingLevel::kLogMutationAndReads)
         {
-            ChipLogDetail(Test, "TestPersistentStorageDelegate::SyncGetKeyValue: Get key '%s'", key);
+            ChipLogDetail(Test, "TestPersistentStorageDelegate::SyncGetKeyValue: Get key '%s'", StringOrNullMarker(key));
         }
 
         CHIP_ERROR err = SyncGetKeyValueInternal(key, buffer, size);
@@ -66,11 +66,13 @@ class TestPersistentStorageDelegate : public PersistentStorageDelegate
         {
             if (err == CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND)
             {
-                ChipLogDetail(Test, "--> TestPersistentStorageDelegate::SyncGetKeyValue: Key '%s' not found", key);
+                ChipLogDetail(Test, "--> TestPersistentStorageDelegate::SyncGetKeyValue: Key '%s' not found",
+                              StringOrNullMarker(key));
             }
             else if (err == CHIP_ERROR_PERSISTED_STORAGE_FAILED)
             {
-                ChipLogDetail(Test, "--> TestPersistentStorageDelegate::SyncGetKeyValue: Key '%s' is a poison key", key);
+                ChipLogDetail(Test, "--> TestPersistentStorageDelegate::SyncGetKeyValue: Key '%s' is a poison key",
+                              StringOrNullMarker(key));
             }
         }
 
@@ -91,7 +93,8 @@ class TestPersistentStorageDelegate : public PersistentStorageDelegate
         {
             if (err == CHIP_ERROR_PERSISTED_STORAGE_FAILED)
             {
-                ChipLogDetail(Test, "--> TestPersistentStorageDelegate::SyncSetKeyValue: Key '%s' is a poison key", key);
+                ChipLogDetail(Test, "--> TestPersistentStorageDelegate::SyncSetKeyValue: Key '%s' is a poison key",
+                              StringOrNullMarker(key));
             }
         }
 
@@ -102,19 +105,21 @@ class TestPersistentStorageDelegate : public PersistentStorageDelegate
     {
         if (mLoggingLevel >= LoggingLevel::kLogMutation)
         {
-            ChipLogDetail(Test, "TestPersistentStorageDelegate::SyncDeleteKeyValue, Delete key '%s'", key);
+            ChipLogDetail(Test, "TestPersistentStorageDelegate::SyncDeleteKeyValue, Delete key '%s'", StringOrNullMarker(key));
         }
         CHIP_ERROR err = SyncDeleteKeyValueInternal(key);
 
         if (mLoggingLevel >= LoggingLevel::kLogMutation)
         {
             if (err == CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND)
             {
-                ChipLogDetail(Test, "--> TestPersistentStorageDelegate::SyncDeleteKeyValue: Key '%s' not found", key);
+                ChipLogDetail(Test, "--> TestPersistentStorageDelegate::SyncDeleteKeyValue: Key '%s' not found",
+                              StringOrNullMarker(key));
             }
             else if (err == CHIP_ERROR_PERSISTED_STORAGE_FAILED)
             {
-                ChipLogDetail(Test, "--> TestPersistentStorageDelegate::SyncDeleteKeyValue: Key '%s' is a poison key", key);
+                ChipLogDetail(Test, "--> TestPersistentStorageDelegate::SyncDeleteKeyValue: Key '%s' is a poison key",
+                              StringOrNullMarker(key));
             }
         }