Skip to content

Commit 406bdfe

Browse files
emargolisemargolis
authored
Fix Logging When Trying to Log Nullptr To Strings (project-chip#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.
1 parent 361c74d commit 406bdfe

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

48 files changed

+267
-201
lines changed

‎src/app/tests/suites/credentials/TestHarnessDACProvider.cpp

+2-2
Original file line numberDiff line numberDiff line change
@@ -147,15 +147,15 @@ void TestHarnessDACProvider::Init(const char * filepath)
147147
std::ifstream json(filepath, std::ifstream::binary);
148148
if (!json)
149149
{
150-
ChipLogError(AppServer, "Error opening json file: %s", filepath);
150+
ChipLogError(AppServer, "Error opening json file: %s", StringOrNullMarker(filepath));
151151
return;
152152
}
153153

154154
Json::Reader reader;
155155
Json::Value root;
156156
if (!reader.parse(json, root))
157157
{
158-
ChipLogError(AppServer, "Error parsing json file: %s", filepath);
158+
ChipLogError(AppServer, "Error parsing json file: %s", StringOrNullMarker(filepath));
159159
return;
160160
}
161161

‎src/app/tests/suites/include/ConstraintsChecker.h

+2-1
Original file line numberDiff line numberDiff line change
@@ -46,7 +46,8 @@ class ConstraintsChecker
4646

4747
bool CheckConstraintFormat(const char * itemName, const char * current, const char * expected)
4848
{
49-
ChipLogError(chipTool, "Warning: %s format checking is not implemented yet. Expected format: '%s'", itemName, expected);
49+
ChipLogError(chipTool, "Warning: %s format checking is not implemented yet. Expected format: '%s'",
50+
StringOrNullMarker(itemName), StringOrNullMarker(expected));
5051
return true;
5152
}
5253

‎src/app/tests/suites/include/PICSChecker.h

+1-1
Original file line numberDiff line numberDiff line change
@@ -40,7 +40,7 @@ class PICSChecker
4040
bool shouldSkip = !PICSBooleanExpressionParser::Eval(expression, pics);
4141
if (shouldSkip)
4242
{
43-
ChipLogProgress(chipTool, " **** Skipping: %s == false\n", expression);
43+
ChipLogProgress(chipTool, " **** Skipping: %s == false\n", StringOrNullMarker(expression));
4444
}
4545
return shouldSkip;
4646
}

‎src/app/tests/suites/include/TestRunner.h

+2-2
Original file line numberDiff line numberDiff line change
@@ -30,11 +30,11 @@ class TestRunner
3030
TestRunner(const char * name, uint16_t testCount) : mTestName(name), mTestCount(testCount), mTestIndex(0) {}
3131
virtual ~TestRunner(){};
3232

33-
void LogStart() { ChipLogProgress(chipTool, " ***** Test Start : %s\n", mTestName); }
33+
void LogStart() { ChipLogProgress(chipTool, " ***** Test Start : %s\n", StringOrNullMarker(mTestName)); }
3434

3535
void LogStep(uint32_t stepNumber, const char * stepName)
3636
{
37-
ChipLogProgress(chipTool, " ***** Test Step %u : %s\n", stepNumber, stepName);
37+
ChipLogProgress(chipTool, " ***** Test Step %u : %s\n", stepNumber, StringOrNullMarker(stepName));
3838
}
3939

4040
void LogEnd(std::string message, CHIP_ERROR err)

‎src/controller/java/AndroidDeviceControllerWrapper.cpp

+3-3
Original file line numberDiff line numberDiff line change
@@ -649,7 +649,7 @@ void AndroidDeviceControllerWrapper::OnScanNetworksFailure(CHIP_ERROR error)
649649

650650
CHIP_ERROR AndroidDeviceControllerWrapper::SyncGetKeyValue(const char * key, void * value, uint16_t & size)
651651
{
652-
ChipLogProgress(chipTool, "KVS: Getting key %s", key);
652+
ChipLogProgress(chipTool, "KVS: Getting key %s", StringOrNullMarker(key));
653653

654654
size_t read_size = 0;
655655

@@ -662,12 +662,12 @@ CHIP_ERROR AndroidDeviceControllerWrapper::SyncGetKeyValue(const char * key, voi
662662

663663
CHIP_ERROR AndroidDeviceControllerWrapper::SyncSetKeyValue(const char * key, const void * value, uint16_t size)
664664
{
665-
ChipLogProgress(chipTool, "KVS: Setting key %s", key);
665+
ChipLogProgress(chipTool, "KVS: Setting key %s", StringOrNullMarker(key));
666666
return chip::DeviceLayer::PersistedStorage::KeyValueStoreMgr().Put(key, value, size);
667667
}
668668

669669
CHIP_ERROR AndroidDeviceControllerWrapper::SyncDeleteKeyValue(const char * key)
670670
{
671-
ChipLogProgress(chipTool, "KVS: Deleting key %s", key);
671+
ChipLogProgress(chipTool, "KVS: Deleting key %s", StringOrNullMarker(key));
672672
return chip::DeviceLayer::PersistedStorage::KeyValueStoreMgr().Delete(key);
673673
}

‎src/controller/python/ChipDeviceController-StorageDelegate.cpp

+5-6
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,6 @@
1-
21
/*
32
*
4-
* Copyright (c) 2021 Project CHIP Authors
3+
* Copyright (c) 2021-2022 Project CHIP Authors
54
* All rights reserved.
65
*
76
* Licensed under the Apache License, Version 2.0 (the "License");
@@ -58,7 +57,7 @@ CHIP_ERROR PythonPersistentStorageDelegate::SyncGetKeyValue(const char * key, vo
5857
CHIP_ERROR PythonPersistentStorageDelegate::SyncSetKeyValue(const char * key, const void * value, uint16_t size)
5958
{
6059
mStorage[key] = std::string(static_cast<const char *>(value), size);
61-
ChipLogDetail(Controller, "SyncSetKeyValue on %s", key);
60+
ChipLogDetail(Controller, "SyncSetKeyValue on %s", StringOrNullMarker(key));
6261

6362
return CHIP_NO_ERROR;
6463
}
@@ -79,7 +78,7 @@ namespace Python {
7978

8079
CHIP_ERROR StorageAdapter::SyncGetKeyValue(const char * key, void * value, uint16_t & size)
8180
{
82-
ChipLogDetail(Controller, "StorageAdapter::GetKeyValue: Key = %s, Value = %p (%u)", key, value, size);
81+
ChipLogDetail(Controller, "StorageAdapter::GetKeyValue: Key = %s, Value = %p (%u)", StringOrNullMarker(key), value, size);
8382
if ((value == nullptr) && (size != 0))
8483
{
8584
return CHIP_ERROR_INVALID_ARGUMENT;
@@ -109,7 +108,7 @@ CHIP_ERROR StorageAdapter::SyncGetKeyValue(const char * key, void * value, uint1
109108
CHIP_ERROR StorageAdapter::SyncSetKeyValue(const char * key, const void * value, uint16_t size)
110109
{
111110
ReturnErrorCodeIf(((value == nullptr) && (size != 0)), CHIP_ERROR_INVALID_ARGUMENT);
112-
ChipLogDetail(Controller, "StorageAdapter::SetKeyValue: Key = %s, Value = %p (%u)", key, value, size);
111+
ChipLogDetail(Controller, "StorageAdapter::SetKeyValue: Key = %s, Value = %p (%u)", StringOrNullMarker(key), value, size);
113112
mSetKeyCb(mContext, key, value, size);
114113
return CHIP_NO_ERROR;
115114
}
@@ -124,7 +123,7 @@ CHIP_ERROR StorageAdapter::SyncDeleteKeyValue(const char * key)
124123
return err;
125124
}
126125

127-
ChipLogDetail(Controller, "StorageAdapter::DeleteKeyValue: Key = %s", key);
126+
ChipLogDetail(Controller, "StorageAdapter::DeleteKeyValue: Key = %s", StringOrNullMarker(key));
128127
mDeleteKeyCb(mContext, key);
129128
return CHIP_NO_ERROR;
130129
}

‎src/credentials/LastKnownGoodTime.cpp

+1-1
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,7 @@ void LastKnownGoodTime::LogTime(const char * msg, System::Clock::Seconds32 chipE
4444
uint8_t second;
4545
ChipEpochToCalendarTime(chipEpochTime.count(), year, month, day, hour, minute, second);
4646
snprintf(buf, sizeof(buf), "%04u-%02u-%02uT%02u:%02u:%02u", year, month, day, hour, minute, second);
47-
ChipLogProgress(TimeService, "%s%s", msg, buf);
47+
ChipLogProgress(TimeService, "%s%s", StringOrNullMarker(msg), buf);
4848
}
4949

5050
CHIP_ERROR LastKnownGoodTime::LoadLastKnownGoodChipEpochTime(System::Clock::Seconds32 & lastKnownGoodChipEpochTime) const

‎src/crypto/CHIPCryptoPALOpenSSL.cpp

+2-1
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,8 @@ static void _logSSLError()
115115
const char * err_str_reason = ERR_reason_error_string(static_cast<libssl_err_type>(ssl_err_code));
116116
if (err_str_lib)
117117
{
118-
ChipLogError(Crypto, " ssl err %s %s %s\n", err_str_lib, err_str_routine, err_str_reason);
118+
ChipLogError(Crypto, " ssl err %s %s %s\n", StringOrNullMarker(err_str_lib), StringOrNullMarker(err_str_routine),
119+
StringOrNullMarker(err_str_reason));
119120
}
120121
#endif // CHIP_ERROR_LOGGING
121122
ssl_err_code = ERR_get_error();

‎src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp

+7-5
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
/*
22
*
3-
* Copyright (c) 2020 Project CHIP Authors
3+
* Copyright (c) 2020-2022 Project CHIP Authors
44
*
55
* Licensed under the Apache License, Version 2.0 (the "License");
66
* you may not use this file except in compliance with the License.
@@ -531,7 +531,8 @@ CHIP_ERROR AdvertiserMinMdns::Advertise(const OperationalAdvertisingParameters &
531531

532532
AdvertiseRecords(BroadcastAdvertiseType::kStarted);
533533

534-
ChipLogProgress(Discovery, "mDNS service published: %s.%s", instanceName.names[1], instanceName.names[2]);
534+
ChipLogProgress(Discovery, "mDNS service published: %s.%s", StringOrNullMarker(instanceName.names[1]),
535+
StringOrNullMarker(instanceName.names[2]));
535536

536537
return CHIP_NO_ERROR;
537538
}
@@ -725,17 +726,18 @@ CHIP_ERROR AdvertiserMinMdns::Advertise(const CommissionAdvertisingParameters &
725726
if (params.GetCommissionAdvertiseMode() == CommssionAdvertiseMode::kCommissionableNode)
726727
{
727728
ChipLogProgress(Discovery, "CHIP minimal mDNS configured as 'Commissionable node device'; instance name: %s.",
728-
instanceName.names[0]);
729+
StringOrNullMarker(instanceName.names[0]));
729730
}
730731
else
731732
{
732733
ChipLogProgress(Discovery, "CHIP minimal mDNS configured as 'Commissioner device'; instance name: %s.",
733-
instanceName.names[0]);
734+
StringOrNullMarker(instanceName.names[0]));
734735
}
735736

736737
AdvertiseRecords(BroadcastAdvertiseType::kStarted);
737738

738-
ChipLogProgress(Discovery, "mDNS service published: %s.%s", instanceName.names[1], instanceName.names[2]);
739+
ChipLogProgress(Discovery, "mDNS service published: %s.%s", StringOrNullMarker(instanceName.names[1]),
740+
StringOrNullMarker(instanceName.names[2]));
739741

740742
return CHIP_NO_ERROR;
741743
}

‎src/lib/dnssd/Discovery_ImplPlatform.cpp

+5-3
Original file line numberDiff line numberDiff line change
@@ -466,7 +466,8 @@ void DiscoveryImplPlatform::HandleDnssdPublish(void * context, const char * type
466466
{
467467
if (CHIP_NO_ERROR == error)
468468
{
469-
ChipLogProgress(Discovery, "mDNS service published: %s; instance name: %s", type, instanceName);
469+
ChipLogProgress(Discovery, "mDNS service published: %s; instance name: %s", StringOrNullMarker(type),
470+
StringOrNullMarker(instanceName));
470471
}
471472
else
472473
{
@@ -520,12 +521,13 @@ CHIP_ERROR DiscoveryImplPlatform::PublishService(const char * serviceType, TextE
520521

521522
for (size_t i = 0; i < textEntrySize; i++)
522523
{
523-
printf(" entry [%u] : %s %s\n", static_cast<unsigned int>(i), textEntries[i].mKey, (char *) (textEntries[i].mData));
524+
printf(" entry [%u] : %s %s\n", static_cast<unsigned int>(i), StringOrNullMarker(textEntries[i].mKey),
525+
StringOrNullMarker((char *) (textEntries[i].mData)));
524526
}
525527

526528
for (size_t i = 0; i < subTypeSize; i++)
527529
{
528-
printf(" type [%u] : %s\n", static_cast<unsigned int>(i), subTypes[i]);
530+
printf(" type [%u] : %s\n", static_cast<unsigned int>(i), StringOrNullMarker(subTypes[i]));
529531
}
530532
#endif
531533

‎src/lib/support/JniReferences.cpp

+3-3
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
/*
22
*
3-
* Copyright (c) 2021 Project CHIP Authors
3+
* Copyright (c) 2021-2022 Project CHIP Authors
44
*
55
* Licensed under the Apache License, Version 2.0 (the "License");
66
* 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
145145
{
146146
if (cbErr == CHIP_JNI_ERROR_EXCEPTION_THROWN)
147147
{
148-
ChipLogError(Support, "Java exception thrown in %s", functName);
148+
ChipLogError(Support, "Java exception thrown in %s", StringOrNullMarker(functName));
149149
env->ExceptionDescribe();
150150
}
151151
else
@@ -166,7 +166,7 @@ void JniReferences::ReportError(JNIEnv * env, CHIP_ERROR cbErr, const char * fun
166166
errStr = ErrorStr(cbErr);
167167
break;
168168
}
169-
ChipLogError(Support, "Error in %s : %s", functName, errStr);
169+
ChipLogError(Support, "Error in %s : %s", StringOrNullMarker(functName), errStr);
170170
}
171171
}
172172

‎src/lib/support/TestPersistentStorageDelegate.h

+13-8
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
/*
22
*
3-
* Copyright (c) 2021 Project CHIP Authors
3+
* Copyright (c) 2021-2022 Project CHIP Authors
44
* All rights reserved.
55
*
66
* Licensed under the Apache License, Version 2.0 (the "License");
@@ -57,7 +57,7 @@ class TestPersistentStorageDelegate : public PersistentStorageDelegate
5757
{
5858
if (mLoggingLevel >= LoggingLevel::kLogMutationAndReads)
5959
{
60-
ChipLogDetail(Test, "TestPersistentStorageDelegate::SyncGetKeyValue: Get key '%s'", key);
60+
ChipLogDetail(Test, "TestPersistentStorageDelegate::SyncGetKeyValue: Get key '%s'", StringOrNullMarker(key));
6161
}
6262

6363
CHIP_ERROR err = SyncGetKeyValueInternal(key, buffer, size);
@@ -66,11 +66,13 @@ class TestPersistentStorageDelegate : public PersistentStorageDelegate
6666
{
6767
if (err == CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND)
6868
{
69-
ChipLogDetail(Test, "--> TestPersistentStorageDelegate::SyncGetKeyValue: Key '%s' not found", key);
69+
ChipLogDetail(Test, "--> TestPersistentStorageDelegate::SyncGetKeyValue: Key '%s' not found",
70+
StringOrNullMarker(key));
7071
}
7172
else if (err == CHIP_ERROR_PERSISTED_STORAGE_FAILED)
7273
{
73-
ChipLogDetail(Test, "--> TestPersistentStorageDelegate::SyncGetKeyValue: Key '%s' is a poison key", key);
74+
ChipLogDetail(Test, "--> TestPersistentStorageDelegate::SyncGetKeyValue: Key '%s' is a poison key",
75+
StringOrNullMarker(key));
7476
}
7577
}
7678

@@ -91,7 +93,8 @@ class TestPersistentStorageDelegate : public PersistentStorageDelegate
9193
{
9294
if (err == CHIP_ERROR_PERSISTED_STORAGE_FAILED)
9395
{
94-
ChipLogDetail(Test, "--> TestPersistentStorageDelegate::SyncSetKeyValue: Key '%s' is a poison key", key);
96+
ChipLogDetail(Test, "--> TestPersistentStorageDelegate::SyncSetKeyValue: Key '%s' is a poison key",
97+
StringOrNullMarker(key));
9598
}
9699
}
97100

@@ -102,19 +105,21 @@ class TestPersistentStorageDelegate : public PersistentStorageDelegate
102105
{
103106
if (mLoggingLevel >= LoggingLevel::kLogMutation)
104107
{
105-
ChipLogDetail(Test, "TestPersistentStorageDelegate::SyncDeleteKeyValue, Delete key '%s'", key);
108+
ChipLogDetail(Test, "TestPersistentStorageDelegate::SyncDeleteKeyValue, Delete key '%s'", StringOrNullMarker(key));
106109
}
107110
CHIP_ERROR err = SyncDeleteKeyValueInternal(key);
108111

109112
if (mLoggingLevel >= LoggingLevel::kLogMutation)
110113
{
111114
if (err == CHIP_ERROR_PERSISTED_STORAGE_VALUE_NOT_FOUND)
112115
{
113-
ChipLogDetail(Test, "--> TestPersistentStorageDelegate::SyncDeleteKeyValue: Key '%s' not found", key);
116+
ChipLogDetail(Test, "--> TestPersistentStorageDelegate::SyncDeleteKeyValue: Key '%s' not found",
117+
StringOrNullMarker(key));
114118
}
115119
else if (err == CHIP_ERROR_PERSISTED_STORAGE_FAILED)
116120
{
117-
ChipLogDetail(Test, "--> TestPersistentStorageDelegate::SyncDeleteKeyValue: Key '%s' is a poison key", key);
121+
ChipLogDetail(Test, "--> TestPersistentStorageDelegate::SyncDeleteKeyValue: Key '%s' is a poison key",
122+
StringOrNullMarker(key));
118123
}
119124
}
120125

0 commit comments

Comments
 (0)