Skip to content

Commit be957c6

Browse files
Alami-Amineclaudepre-commit-ci[bot]
authored
[clang-tidy] Enable core/cplusplus/security static-analyzer checks (project-chip#72487)
* [clang-tidy] Enable core/cplusplus/security static-analyzer checks The .clang-tidy Checks block carries -clang-analyzer-... disable lines but no clang-analyzer-* positive, and the pigweed clang-tidy used in CI does not enable the analyzer family by default. As a result the path-sensitive static analyzer (null-deref, out-of-bounds, use-after-free/double-free, uninitialized reads, etc.) does not run at all in the clang-tidy CI step. Add clang-analyzer-core.*, clang-analyzer-cplusplus.*, clang-analyzer-security* and clang-analyzer-unix.Malloc as positives, keep the checkers that only produce false positives or intentional-pattern hits disabled (each with an inline rationale), and fix the findings that blocked enabling the rest: - initialize out-parameters before passing them to encode helpers in the General/Software/Ethernet diagnostics clusters and in the interaction-model trace decoder (which read an uninitialized id on a malformed-TLV path) - initialize attribute out-vars read by several cluster unit tests - guard memcmp() against null zero-length buffers in crypto unit tests (passing null to memcmp is undefined behavior even when the length is 0) - use ASSERT_EQ for a create-then-dereference precondition in a UDC test Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * [clang-tidy] Move disable rationale to YAML comments above Checks `Checks:` is a YAML folded block scalar, so `#` inside it is literal text parsed as a check glob, not a comment. Move the rationale for the disabled cplusplus.Move / cplusplus.NewDeleteLeaks checks to real YAML comments above the block to avoid the footgun and keep the check list clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
1 parent 231d4dd commit be957c6

12 files changed

Lines changed: 46 additions & 29 deletions

File tree

‎.clang-tidy‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,19 @@
11
---
2+
# NOTE: `Checks:` below is a YAML folded block scalar, so `#` is NOT a comment there
3+
# (it becomes literal text parsed as a check glob). Keep rationale here, above the block.
4+
#
5+
# clang-analyzer checks intentionally left disabled (false-positive-only on this codebase;
6+
# enabling them would require ongoing NOLINTs for zero real findings):
7+
# - clang-analyzer-cplusplus.Move: only fires on intentional moved-from-state test
8+
# assertions (already NOLINTed for bugprone-use-after-move) plus a TLVReader.h false positive.
9+
# - clang-analyzer-cplusplus.NewDeleteLeaks: std::function / self-deleting-callback lifetimes
10+
# that the analyzer cannot model (e.g. controller InvokeInteraction/ReadInteraction templates).
211
Checks: >
312
bugprone-*,
13+
clang-analyzer-core.*,
14+
clang-analyzer-cplusplus.*,
15+
clang-analyzer-security*,
16+
clang-analyzer-unix.Malloc,
417
cppcoreguidelines-virtual-class-destructor,
518
modernize-redundant-void-arg,
619
modernize-use-bool-literals,
@@ -33,9 +46,8 @@ Checks: >
3346
-bugprone-switch-missing-default-case,
3447
-bugprone-undelegated-constructor,
3548
-bugprone-unused-return-value,
36-
-clang-analyzer-core.CallAndMessage,
37-
-clang-analyzer-core.NonNullParamChecker,
3849
-clang-analyzer-cplusplus.Move,
50+
-clang-analyzer-cplusplus.NewDeleteLeaks,
3951
-clang-analyzer-deadcode.DeadStores,
4052
-clang-analyzer-nullability.NullablePassedToNonnull,
4153
-clang-analyzer-optin.core.EnumCastOutOfRange,

‎examples/common/tracing/decoder/interaction_model/DecoderCustomLog.cpp‎

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -291,8 +291,8 @@ CHIP_ERROR MaybeDecodeAttributeData(TLV::TLVReader & reader)
291291
{
292292
CHIP_ERROR err = CHIP_NO_ERROR;
293293

294-
ClusterId clusterId;
295-
AttributeId attributeId;
294+
ClusterId clusterId = kInvalidClusterId;
295+
AttributeId attributeId = kInvalidAttributeId;
296296

297297
TLV::TLVType containerType;
298298
while (CHIP_NO_ERROR == (err = reader.Next()))
@@ -338,8 +338,8 @@ CHIP_ERROR MaybeDecodeCommandData(TLV::TLVReader & reader)
338338
{
339339
CHIP_ERROR err = CHIP_NO_ERROR;
340340

341-
ClusterId clusterId;
342-
CommandId commandId;
341+
ClusterId clusterId = kInvalidClusterId;
342+
CommandId commandId = kInvalidCommandId;
343343

344344
TLV::TLVType containerType;
345345
while (CHIP_NO_ERROR == (err = reader.Next()))

‎src/app/clusters/ethernet-network-diagnostics-server/EthernetDiagnosticsCluster.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -60,7 +60,7 @@ EthernetDiagnosticsServerCluster::EthernetDiagnosticsServerCluster(DeviceLayer::
6060
DataModel::ActionReturnStatus EthernetDiagnosticsServerCluster::ReadAttribute(const DataModel::ReadAttributeRequest & request,
6161
AttributeValueEncoder & encoder)
6262
{
63-
uint64_t value;
63+
uint64_t value = 0;
6464
CHIP_ERROR err = CHIP_ERROR_UNSUPPORTED_CHIP_FEATURE;
6565

6666
switch (request.path.mAttributeId)

‎src/app/clusters/general-diagnostics-server/GeneralDiagnosticsCluster.cpp‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -250,7 +250,7 @@ DataModel::ActionReturnStatus GeneralDiagnosticsCluster::ReadAttribute(const Dat
250250
return EncodeListOfValues(valueList, err, encoder);
251251
}
252252
case GeneralDiagnostics::Attributes::RebootCount::Id: {
253-
uint16_t value;
253+
uint16_t value = 0;
254254
CHIP_ERROR err = GetRebootCount(value);
255255
return EncodeValue(value, err, encoder);
256256
}
@@ -259,12 +259,12 @@ DataModel::ActionReturnStatus GeneralDiagnosticsCluster::ReadAttribute(const Dat
259259
return encoder.Encode(static_cast<uint64_t>(system_time_seconds.count()));
260260
}
261261
case GeneralDiagnostics::Attributes::TotalOperationalHours::Id: {
262-
uint32_t value;
262+
uint32_t value = 0;
263263
CHIP_ERROR err = GetTotalOperationalHours(value);
264264
return EncodeValue(value, err, encoder);
265265
}
266266
case GeneralDiagnostics::Attributes::BootReason::Id: {
267-
GeneralDiagnostics::BootReasonEnum value;
267+
GeneralDiagnostics::BootReasonEnum value{};
268268
CHIP_ERROR err = GetBootReason(value);
269269
return EncodeValue(value, err, encoder);
270270
}

‎src/app/clusters/groupcast/tests/TestGroupcastCluster.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -797,7 +797,7 @@ TEST_F(TestGroupcastCluster, TestMaxMcastAddrCount)
797797
tester.SetSubjectDescriptor(kAdminSubjectDescriptor);
798798

799799
// Read MaxMcastAddrCount
800-
app::Clusters::Groupcast::Attributes::MaxMcastAddrCount::TypeInfo::DecodableType maxMcastAddrCount;
800+
app::Clusters::Groupcast::Attributes::MaxMcastAddrCount::TypeInfo::DecodableType maxMcastAddrCount{};
801801
ASSERT_EQ(tester.ReadAttribute(app::Clusters::Groupcast::Attributes::MaxMcastAddrCount::Id, maxMcastAddrCount), CHIP_NO_ERROR);
802802
ASSERT_GT(maxMcastAddrCount, 0u);
803803

‎src/app/clusters/software-diagnostics-server/SoftwareDiagnosticsCluster.cpp‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -57,17 +57,17 @@ DataModel::ActionReturnStatus SoftwareDiagnosticsServerCluster::ReadAttribute(co
5757
switch (request.path.mAttributeId)
5858
{
5959
case Attributes::CurrentHeapFree::Id: {
60-
uint64_t value;
60+
uint64_t value = 0;
6161
CHIP_ERROR err = mDiagnosticDataProvider.GetCurrentHeapFree(value);
6262
return EncodeValue(value, err, encoder);
6363
}
6464
case Attributes::CurrentHeapUsed::Id: {
65-
uint64_t value;
65+
uint64_t value = 0;
6666
CHIP_ERROR err = mDiagnosticDataProvider.GetCurrentHeapUsed(value);
6767
return EncodeValue(value, err, encoder);
6868
}
6969
case Attributes::CurrentHeapHighWatermark::Id: {
70-
uint64_t value;
70+
uint64_t value = 0;
7171
CHIP_ERROR err = mDiagnosticDataProvider.GetCurrentHeapHighWatermark(value);
7272
return EncodeValue(value, err, encoder);
7373
}

‎src/app/clusters/software-diagnostics-server/tests/TestSoftwareDiagnosticsCluster.cpp‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -202,27 +202,27 @@ TEST_F(TestSoftwareDiagnosticsCluster, AttributesAndCommandTest)
202202

203203
// Test all attributes
204204
// cluster revision
205-
Attributes::ClusterRevision::TypeInfo::DecodableType clusterRevision;
205+
Attributes::ClusterRevision::TypeInfo::DecodableType clusterRevision{};
206206
ASSERT_TRUE(tester.ReadAttribute(Attributes::ClusterRevision::Id, clusterRevision).IsSuccess());
207207
EXPECT_EQ(clusterRevision, SoftwareDiagnostics::kRevision);
208208

209209
// feature map
210-
Attributes::FeatureMap::TypeInfo::DecodableType featureMap;
210+
Attributes::FeatureMap::TypeInfo::DecodableType featureMap{};
211211
ASSERT_TRUE(tester.ReadAttribute(Attributes::FeatureMap::Id, featureMap).IsSuccess());
212212
EXPECT_EQ(featureMap, BitFlags<SoftwareDiagnostics::Feature>{ SoftwareDiagnostics::Feature::kWatermarks }.Raw());
213213

214214
// heapfree
215-
Attributes::CurrentHeapFree::TypeInfo::DecodableType heapFree;
215+
Attributes::CurrentHeapFree::TypeInfo::DecodableType heapFree{};
216216
ASSERT_TRUE(tester.ReadAttribute(Attributes::CurrentHeapFree::Id, heapFree).IsSuccess());
217217
EXPECT_EQ(heapFree, 123u);
218218

219219
// heapused
220-
Attributes::CurrentHeapUsed::TypeInfo::DecodableType heapUsed;
220+
Attributes::CurrentHeapUsed::TypeInfo::DecodableType heapUsed{};
221221
ASSERT_TRUE(tester.ReadAttribute(Attributes::CurrentHeapUsed::Id, heapUsed).IsSuccess());
222222
EXPECT_EQ(heapUsed, 234u);
223223

224224
// highwatermark
225-
Attributes::CurrentHeapHighWatermark::TypeInfo::DecodableType highWatermark;
225+
Attributes::CurrentHeapHighWatermark::TypeInfo::DecodableType highWatermark{};
226226
ASSERT_TRUE(tester.ReadAttribute(Attributes::CurrentHeapHighWatermark::Id, highWatermark).IsSuccess());
227227
EXPECT_EQ(highWatermark, 456u);
228228

‎src/app/clusters/thread-border-router-management-server/tests/TestThreadBorderRouterManagementCluster.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -222,7 +222,7 @@ TEST_F(TestThreadBorderRouterManagementCluster, TestFeatureMap_PanChangeSupporte
222222
chip::Testing::ClusterTester tester(cluster);
223223
EXPECT_EQ(cluster.Startup(tester.GetServerClusterContext()), CHIP_NO_ERROR);
224224

225-
uint32_t featureMap;
225+
uint32_t featureMap = 0;
226226
EXPECT_TRUE(tester.ReadAttribute(Globals::Attributes::FeatureMap::Id, featureMap).IsSuccess());
227227
EXPECT_EQ(featureMap, static_cast<uint32_t>(Feature::kPANChange));
228228
}
@@ -238,7 +238,7 @@ TEST_F(TestThreadBorderRouterManagementCluster, TestFeatureMap_PanChangeNotSuppo
238238
chip::Testing::ClusterTester tester(localCluster);
239239
EXPECT_EQ(localCluster.Startup(tester.GetServerClusterContext()), CHIP_NO_ERROR);
240240

241-
uint32_t featureMap;
241+
uint32_t featureMap = 0;
242242
EXPECT_TRUE(tester.ReadAttribute(Globals::Attributes::FeatureMap::Id, featureMap).IsSuccess());
243243
EXPECT_EQ(featureMap, 0u);
244244

‎src/credentials/tests/TestGroupDataProvider.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -891,8 +891,8 @@ TEST_F(TestGroupDataProvider, TestKeySetCacheAndSyncRemap)
891891
// 2. Read the saved TLV from storage
892892
auto keyName = DefaultStorageKeyAllocator::FabricKeyset(kFabric1, kKeysetId1);
893893

894-
uint8_t tlvBuffer[128];
895-
uint16_t tlvLength = sizeof(tlvBuffer);
894+
uint8_t tlvBuffer[128] = {};
895+
uint16_t tlvLength = sizeof(tlvBuffer);
896896
EXPECT_EQ(sDelegate.SyncGetKeyValue(keyName.KeyName(), tlvBuffer, tlvLength), CHIP_NO_ERROR);
897897

898898
// 3. Sanity check and patch the policy to CacheAndSync (1) in-place.

‎src/crypto/tests/TestChipCryptoPAL.cpp‎

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -386,7 +386,8 @@ TEST_F(TestChipCryptoPAL, TestAES_CCM_128EncryptTestVectors)
386386

387387
if (vector->result == CHIP_NO_ERROR)
388388
{
389-
bool areCTsEqual = memcmp(out_ct_ptr, vector->ct, vector->ct_len) == 0;
389+
// memcmp() requires non-null pointers even when length is 0; out_ct_ptr is null for zero-length vectors.
390+
bool areCTsEqual = (vector->ct_len == 0) || (memcmp(out_ct_ptr, vector->ct, vector->ct_len) == 0);
390391
bool areTagsEqual = memcmp(out_tag.Get(), vector->tag, vector->tag_len) == 0;
391392
EXPECT_TRUE(areCTsEqual);
392393
EXPECT_TRUE(areTagsEqual);
@@ -431,7 +432,8 @@ TEST_F(TestChipCryptoPAL, TestAES_CCM_128DecryptTestVectors)
431432
EXPECT_EQ(err, vector->result);
432433
if (vector->result == CHIP_NO_ERROR)
433434
{
434-
bool arePTsEqual = memcmp(vector->pt, out_pt_ptr, vector->pt_len) == 0;
435+
// memcmp() requires non-null pointers even when length is 0; out_pt_ptr is null for zero-length vectors.
436+
bool arePTsEqual = (vector->pt_len == 0) || (memcmp(vector->pt, out_pt_ptr, vector->pt_len) == 0);
435437
EXPECT_TRUE(arePTsEqual);
436438
if (!arePTsEqual)
437439
{
@@ -480,7 +482,8 @@ TEST_F(TestChipCryptoPAL, TestAES_CCM_128InPlaceEncryption)
480482
EXPECT_EQ(err, vector->result);
481483
if (vector->result == CHIP_NO_ERROR)
482484
{
483-
bool areCTsEqual = memcmp(inplace_buffer_ptr, vector->ct, vector->ct_len) == 0;
485+
// memcmp() requires non-null pointers even when length is 0; inplace_buffer_ptr is null for zero-length vectors.
486+
bool areCTsEqual = (vector->ct_len == 0) || (memcmp(inplace_buffer_ptr, vector->ct, vector->ct_len) == 0);
484487
bool areTagsEqual = memcmp(out_tag.Get(), vector->tag, vector->tag_len) == 0;
485488
EXPECT_TRUE(areCTsEqual);
486489
EXPECT_TRUE(areTagsEqual);
@@ -532,7 +535,8 @@ TEST_F(TestChipCryptoPAL, TestAES_CCM_128InPlaceDecryption)
532535
EXPECT_EQ(err, vector->result);
533536
if (vector->result == CHIP_NO_ERROR)
534537
{
535-
bool arePTsEqual = memcmp(vector->pt, inplace_buffer_ptr, vector->pt_len) == 0;
538+
// memcmp() requires non-null pointers even when length is 0; inplace_buffer_ptr is null for zero-length vectors.
539+
bool arePTsEqual = (vector->pt_len == 0) || (memcmp(vector->pt, inplace_buffer_ptr, vector->pt_len) == 0);
536540
EXPECT_TRUE(arePTsEqual);
537541
if (!arePTsEqual)
538542
{

0 commit comments

Comments
 (0)