Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Re-enable clang-tidy check for enum value range validation #32289

Closed
wants to merge 7 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion .clang-tidy
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ Checks: >
-clang-analyzer-cplusplus.Move,
-clang-analyzer-deadcode.DeadStores,
-clang-analyzer-nullability.NullablePassedToNonnull,
-clang-analyzer-optin.core.EnumCastOutOfRange,
-clang-analyzer-optin.cplusplus.UninitializedObject,
-clang-analyzer-optin.cplusplus.VirtualCall,
-clang-analyzer-optin.osx.cocoa.localizability.EmptyLocalizationContextChecker,
Expand Down
2 changes: 1 addition & 1 deletion src/access/AccessControl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -554,7 +554,7 @@ bool AccessControl::IsValid(const Entry & entry)

AuthMode authMode = AuthMode::kNone;
FabricIndex fabricIndex = kUndefinedFabricIndex;
Privilege privilege = static_cast<Privilege>(0);
Privilege privilege = Privilege::kView;
size_t subjectCount = 0;
size_t targetCount = 0;

Expand Down
7 changes: 7 additions & 0 deletions src/app/clusters/door-lock-server/door-lock-server.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2590,8 +2590,15 @@ bool DoorLockServer::weekDayIndexValid(chip::EndpointId endpointId, uint8_t week

DlStatus DoorLockServer::clearWeekDaySchedule(chip::EndpointId endpointId, uint16_t userIndex, uint8_t weekDayIndex)
{
// TODO: DaysMaskMap 0 is not an actual ENUM value. However code is too coupled to easily
// convert to BitFlags<DaysMaskMap> as part of CLANG-upgrade.
// See https://github.com/project-chip/connectedhomeip/issues/32249
//
// NOLINTBEGIN(*.EnumCastOutOfRange)
auto status = emberAfPluginDoorLockSetSchedule(endpointId, weekDayIndex, userIndex, DlScheduleStatus::kAvailable,
DaysMaskMap(0), 0, 0, 0, 0);
// NOLINTEND(*.EnumCastOutOfRange)

if (DlStatus::kSuccess != status && DlStatus::kNotFound != status)
{
ChipLogError(Zcl,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -287,10 +287,14 @@ void ModeSet(chip::EndpointId endpoint, chip::BitMask<Mode> & newMode)
}

if (oldMode != newMode)
{
Attributes::Mode::Set(endpoint, newMode);
}

if (oldStatus != newStatus)
{
ConfigStatusSet(endpoint, newStatus);
}
}

chip::BitMask<Mode> ModeGet(chip::EndpointId endpoint)
Expand Down
5 changes: 5 additions & 0 deletions src/app/icd/server/ICDManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -391,6 +391,8 @@ void ICDManager::SetKeepActiveModeRequirements(KeepActiveFlags flag, bool state)
{
assertChipStackLockedByCurrentThread();

// TODO: fix lint: https://github.com/project-chip/connectedhomeip/issues/32249
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That dos not look like the right link.

// NOLINTNEXTLINE(*.EnumCastOutOfRange)
mKeepActiveFlags.Set(flag, state);
if (mOperationalState == OperationalState::IdleMode && mKeepActiveFlags.HasAny())
{
Expand Down Expand Up @@ -511,6 +513,9 @@ void ICDManager::OnActiveRequestWithdrawal(KeepActiveFlags request)
{
// Only 1 request per type (kCommissioningWindowOpen, kFailSafeArmed)
// remove requirement directly

// TODO: fix lint: https://github.com/project-chip/connectedhomeip/issues/32249
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong link?

// NOLINTNEXTLINE(*.EnumCastOutOfRange)
this->SetKeepActiveModeRequirements(request, false /* state */);
}
}
Expand Down
2 changes: 2 additions & 0 deletions src/app/tests/TestDataModelSerialization.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -241,6 +241,8 @@ void TestDataModelSerialization::TestDataModelSerialization_EncAndDecSimpleStruc

t.a = 20;
t.b = true;
// Intentionally out of range to get kUnknownEnumValue
// NOLINTNEXTLINE(*.EnumCastOutOfRange)
t.c = static_cast<Clusters::UnitTesting::SimpleEnum>(10);
t.d = buf;

Expand Down
5 changes: 4 additions & 1 deletion src/app/tests/TestNumericAttributeTraits.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,10 @@ void Test_SimpleEnum(nlTestSuite * apSuite, void * apContext)
WorkingType wValue;
StorageType sNullValue;
WorkingType wNullValue;
const StorageType storageTestData = SimpleEnum::kOne;
const StorageType storageTestData = SimpleEnum::kOne;

// Intentionally set a "NULL" value
// NOLINTNEXTLINE(*.EnumCastOutOfRange)
const WorkingType workingTestUnsignedNullValue = static_cast<SimpleEnum>(0xFF);

// 1) Verify the size of the types
Expand Down
2 changes: 2 additions & 0 deletions src/crypto/tests/TestChipCryptoPAL.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -2692,6 +2692,8 @@ static void TestVIDPID_StringExtraction(nlTestSuite * inSuite, void * inContext)
{ DNAttrType::kMatterPID, ByteSpan(reinterpret_cast<const uint8_t *>(sTestMatterAttribute14), strlen(sTestMatterAttribute14)), false, false, chip::VendorId::NotSpecified, 0, CHIP_ERROR_WRONG_CERT_DN },
// Common Name (CN) VID/PID encoding examples:
{ DNAttrType::kCommonName, ByteSpan(reinterpret_cast<const uint8_t *>(sTestCNAttribute01), strlen(sTestCNAttribute01)), true, false, chip::VendorId::TestVendor1, 0, CHIP_NO_ERROR },
// TODO: fix lint: https://github.com/project-chip/connectedhomeip/issues/32249
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wrong link?

// NOLINTNEXTLINE(*.EnumCastOutOfRange)
{ DNAttrType::kCommonName, ByteSpan(reinterpret_cast<const uint8_t *>(sTestCNAttribute02), strlen(sTestCNAttribute02)), true, false, static_cast<chip::VendorId>(0x002A), 0, CHIP_NO_ERROR },
{ DNAttrType::kCommonName, ByteSpan(reinterpret_cast<const uint8_t *>(sTestCNAttribute03), strlen(sTestCNAttribute03)), false, true, chip::VendorId::NotSpecified, 0xC20A, CHIP_NO_ERROR },
{ DNAttrType::kCommonName, ByteSpan(reinterpret_cast<const uint8_t *>(sTestCNAttribute04), strlen(sTestCNAttribute04)), false, true, chip::VendorId::NotSpecified, 0x03A5, CHIP_NO_ERROR },
Expand Down
6 changes: 6 additions & 0 deletions src/lib/core/TLVReader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,9 @@ TLVType TLVReader::GetType() const
return kTLVType_FloatingPointNumber;
if (elemType == TLVElementType::NotSpecified || elemType >= TLVElementType::Null)
return static_cast<TLVType>(elemType);

// TODO: fix lint: https://github.com/project-chip/connectedhomeip/issues/32249
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All these issue links are wrong....

// NOLINTNEXTLINE(*.EnumCastOutOfRange)
return static_cast<TLVType>(static_cast<uint8_t>(elemType) & ~kTLVTypeSizeMask);
}

Expand Down Expand Up @@ -1030,6 +1033,9 @@ TLVElementType TLVReader::ElementType() const
{
if (mControlByte == static_cast<uint16_t>(kTLVControlByte_NotSpecified))
return TLVElementType::NotSpecified;

// TODO: fix lint: https://github.com/project-chip/connectedhomeip/issues/32249
// NOLINTNEXTLINE(*.EnumCastOutOfRange)
return static_cast<TLVElementType>(mControlByte & kTLVTypeMask);
}

Expand Down
18 changes: 15 additions & 3 deletions src/lib/support/BitFlags.h
Original file line number Diff line number Diff line change
Expand Up @@ -200,10 +200,22 @@ class BitFlags
/**
* Get the flags as the type FlagsEnum.
*
* @note This allows easily storing flags as a base FlagsEnum in a POD type,
* and enables equality comparisons.
* This MAY return enum values that are not inside the range of valid enumerations.
*/
operator FlagsEnum() const { return static_cast<FlagsEnum>(mValue); }
operator FlagsEnum() const
{
// TODO: fix lint: https://github.com/project-chip/connectedhomeip/issues/32249
// NOLINTNEXTLINE(*.EnumCastOutOfRange)
return static_cast<FlagsEnum>(mValue);
}

bool operator==(const BitFlags<FlagsEnum> & other) const { return mValue == other.mValue; }
bool operator!=(const BitFlags<FlagsEnum> & other) const { return mValue != other.mValue; }

// TODO: This is a convenience for exact equality, however this hides away that
// a bitflag is used instead of an exact enum.
// Should consider removing existence of this.
bool operator==(FlagsEnum other) const { return (*this == BitFlags<FlagsEnum>(other)); }

/**
* Set and/or clear all flags with a value of the underlying storage type.
Expand Down
11 changes: 11 additions & 0 deletions src/platform/Linux/ConnectivityManagerImpl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -510,6 +510,7 @@ void ConnectivityManagerImpl::_OnWpaInterfaceProxyReady(GObject * sourceObject,
mWpaSupplicant.state = GDBusWpaSupplicant::WPA_INTERFACE_CONNECTED;
ChipLogProgress(DeviceLayer, "wpa_supplicant: connected to wpa_supplicant interface proxy");

// NOLINTBEGIN(*.EnumCastOutOfRange)
g_signal_connect(
mWpaSupplicant.iface, "properties-changed",
G_CALLBACK(+[](WpaFiW1Wpa_supplicant1Interface * proxy, GVariant * properties, ConnectivityManagerImpl * self) {
Expand All @@ -521,6 +522,7 @@ void ConnectivityManagerImpl::_OnWpaInterfaceProxyReady(GObject * sourceObject,
return self->_OnWpaInterfaceScanDone(proxy, success);
}),
this);
// NOLINTEND(*.EnumCastOutOfRange)
}
else
{
Expand Down Expand Up @@ -750,6 +752,7 @@ void ConnectivityManagerImpl::_OnWpaProxyReady(GObject * sourceObject, GAsyncRes
mWpaSupplicant.state = GDBusWpaSupplicant::WPA_CONNECTED;
ChipLogProgress(DeviceLayer, "wpa_supplicant: connected to wpa_supplicant proxy");

// NOLINTBEGIN(*.EnumCastOutOfRange)
g_signal_connect(
mWpaSupplicant.proxy, "interface-added",
G_CALLBACK(+[](WpaFiW1Wpa_supplicant1 * proxy, const char * path, GVariant * properties,
Expand All @@ -760,6 +763,7 @@ void ConnectivityManagerImpl::_OnWpaProxyReady(GObject * sourceObject, GAsyncRes
G_CALLBACK(+[](WpaFiW1Wpa_supplicant1 * proxy, const char * path, GVariant * properties,
ConnectivityManagerImpl * self) { return self->_OnWpaInterfaceRemoved(proxy, path, properties); }),
this);
// NOLINTEND(*.EnumCastOutOfRange)

wpa_fi_w1_wpa_supplicant1_call_get_interface(
mWpaSupplicant.proxy, sWiFiIfName, nullptr,
Expand Down Expand Up @@ -1676,9 +1680,13 @@ bool ConnectivityManagerImpl::_GetBssInfo(const gchar * bssPath, NetworkCommissi
return false;
}

// NOLINTNEXTLINE(bugprone-casting-through-void)
WpaFiW1Wpa_supplicant1BSSProxy * bssProxy = WPA_FI_W1_WPA_SUPPLICANT1_BSS_PROXY(bss.get());

// NOLINTNEXTLINE(bugprone-casting-through-void)
GAutoPtr<GVariant> ssid(g_dbus_proxy_get_cached_property(G_DBUS_PROXY(bssProxy), "SSID"));

// NOLINTNEXTLINE(bugprone-casting-through-void)
GAutoPtr<GVariant> bssid(g_dbus_proxy_get_cached_property(G_DBUS_PROXY(bssProxy), "BSSID"));

// Network scan is performed in the background, so the BSS
Expand Down Expand Up @@ -1789,7 +1797,10 @@ bool ConnectivityManagerImpl::_GetBssInfo(const gchar * bssPath, NetworkCommissi
return res;
};
auto GetNetworkSecurityType = [IsNetworkWPAPSK, IsNetworkWPA2PSK](WpaFiW1Wpa_supplicant1BSSProxy * proxy) -> uint8_t {
// NOLINTNEXTLINE(bugprone-casting-through-void)
GAutoPtr<GVariant> wpa(g_dbus_proxy_get_cached_property(G_DBUS_PROXY(proxy), "WPA"));

// NOLINTNEXTLINE(bugprone-casting-through-void)
GAutoPtr<GVariant> rsn(g_dbus_proxy_get_cached_property(G_DBUS_PROXY(proxy), "RSN"));

uint8_t res = IsNetworkWPAPSK(wpa.get()) | IsNetworkWPA2PSK(rsn.get());
Expand Down
5 changes: 5 additions & 0 deletions src/platform/Linux/ThreadStackManagerImpl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ CHIP_ERROR ThreadStackManagerImpl::GLibMatterContextInitThreadStack(ThreadStackM
self->mProxy != nullptr, CHIP_ERROR_INTERNAL,
ChipLogError(DeviceLayer, "openthread: failed to create openthread dbus proxy %s", err ? err->message : "unknown error"));

// NOLINTNEXTLINE(*.EnumCastOutOfRange)
g_signal_connect(self->mProxy.get(), "g-properties-changed", G_CALLBACK(OnDbusPropertiesChanged), self);

return CHIP_NO_ERROR;
Expand Down Expand Up @@ -270,6 +271,8 @@ CHIP_ERROR ThreadStackManagerImpl::_GetThreadProvision(Thread::OperationalDatase

{
GAutoPtr<GError> err;

// NOLINTNEXTLINE(bugprone-casting-through-void)
GAutoPtr<GVariant> response(g_dbus_proxy_call_sync(G_DBUS_PROXY(mProxy.get()), "org.freedesktop.DBus.Properties.Get",
g_variant_new("(ss)", "io.openthread.BorderRouter", "ActiveDatasetTlvs"),
G_DBUS_CALL_FLAGS_NONE, -1, nullptr, &err.GetReceiver()));
Expand Down Expand Up @@ -326,6 +329,8 @@ bool ThreadStackManagerImpl::_IsThreadEnabled()
VerifyOrReturnError(mProxy, false);

GAutoPtr<GError> err;

// NOLINTNEXTLINE(bugprone-casting-through-void)
GAutoPtr<GVariant> response(g_dbus_proxy_call_sync(G_DBUS_PROXY(mProxy.get()), "org.freedesktop.DBus.Properties.Get",
g_variant_new("(ss)", "io.openthread.BorderRouter", "DeviceRole"),
G_DBUS_CALL_FLAGS_NONE, -1, nullptr, &err.GetReceiver()));
Expand Down
2 changes: 2 additions & 0 deletions src/platform/Linux/bluez/AdapterIterator.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -77,6 +77,7 @@ bool AdapterIterator::Advance()

while (mCurrentListItem != nullptr)
{
// NOLINTNEXTLINE(bugprone-casting-through-void)
BluezAdapter1 * adapter = bluez_object_get_adapter1(BLUEZ_OBJECT(mCurrentListItem->data));
if (adapter == nullptr)
{
Expand All @@ -98,6 +99,7 @@ uint32_t AdapterIterator::GetIndex() const
{
// PATH is of the for BLUEZ_PATH / hci<nr>, i.e. like '/org/bluez/hci0'
// Index represents the number after hci
// NOLINTNEXTLINE(bugprone-casting-through-void)
const char * path = g_dbus_proxy_get_object_path(G_DBUS_PROXY(mCurrentAdapter.get()));
unsigned index = 0;

Expand Down
10 changes: 10 additions & 0 deletions src/platform/Linux/bluez/BluezAdvertisement.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -75,12 +75,15 @@ BluezLEAdvertisement1 * BluezAdvertisement::CreateLEAdvertisement()
// empty secondary channel for now

bluez_object_skeleton_set_leadvertisement1(object, adv);
// NOLINTBEGIN(*.EnumCastOutOfRange)
g_signal_connect(adv, "handle-release",
G_CALLBACK(+[](BluezLEAdvertisement1 * aAdv, GDBusMethodInvocation * aInv, BluezAdvertisement * self) {
return self->BluezLEAdvertisement1Release(aAdv, aInv);
}),
this);
// NOLINTEND(*.EnumCastOutOfRange)

// NOLINTNEXTLINE(bugprone-casting-through-void)
g_dbus_object_manager_server_export(mRoot.get(), G_DBUS_OBJECT_SKELETON(object));
g_object_unref(object);

Expand Down Expand Up @@ -118,6 +121,7 @@ CHIP_ERROR BluezAdvertisement::Init(const BluezEndpoint & aEndpoint, const char
mRoot.reset(reinterpret_cast<GDBusObjectManagerServer *>(g_object_ref(aEndpoint.GetGattApplicationObjectManager())));
mAdapter.reset(reinterpret_cast<BluezAdapter1 *>(g_object_ref(aEndpoint.GetAdapter())));

// NOLINTNEXTLINE(bugprone-casting-through-void)
g_object_get(G_OBJECT(mRoot.get()), "object-path", &rootPath.GetReceiver(), nullptr);
g_snprintf(mAdvPath, sizeof(mAdvPath), "%s/advertising", rootPath.get());
g_strlcpy(mAdvUUID, aAdvUUID, sizeof(mAdvUUID));
Expand Down Expand Up @@ -224,6 +228,7 @@ void BluezAdvertisement::Shutdown()

void BluezAdvertisement::StartDone(GObject * aObject, GAsyncResult * aResult)
{
// NOLINTNEXTLINE(bugprone-casting-through-void)
BluezLEAdvertisingManager1 * advMgr = BLUEZ_LEADVERTISING_MANAGER1(aObject);
GAutoPtr<GError> error;
gboolean success = FALSE;
Expand All @@ -249,9 +254,11 @@ CHIP_ERROR BluezAdvertisement::StartImpl()
VerifyOrExit(!mIsAdvertising, ChipLogError(DeviceLayer, "FAIL: Advertising has already been enabled in %s", __func__));
VerifyOrExit(mAdapter.get() != nullptr, ChipLogError(DeviceLayer, "FAIL: NULL mAdapter in %s", __func__));

// NOLINTNEXTLINE(bugprone-casting-through-void)
adapterObject = g_dbus_interface_get_object(G_DBUS_INTERFACE(mAdapter.get()));
VerifyOrExit(adapterObject != nullptr, ChipLogError(DeviceLayer, "FAIL: NULL adapterObject in %s", __func__));

// NOLINTNEXTLINE(bugprone-casting-through-void)
advMgr.reset(bluez_object_get_leadvertising_manager1(BLUEZ_OBJECT(adapterObject)));
VerifyOrExit(advMgr.get() != nullptr, ChipLogError(DeviceLayer, "FAIL: NULL advMgr in %s", __func__));

Expand Down Expand Up @@ -282,6 +289,7 @@ CHIP_ERROR BluezAdvertisement::Start()

void BluezAdvertisement::StopDone(GObject * aObject, GAsyncResult * aResult)
{
// NOLINTNEXTLINE(bugprone-casting-through-void)
BluezLEAdvertisingManager1 * advMgr = BLUEZ_LEADVERTISING_MANAGER1(aObject);
GAutoPtr<GError> error;
gboolean success = FALSE;
Expand All @@ -305,9 +313,11 @@ CHIP_ERROR BluezAdvertisement::StopImpl()
VerifyOrExit(mIsAdvertising, ChipLogError(DeviceLayer, "FAIL: Advertising has already been disabled in %s", __func__));
VerifyOrExit(mAdapter.get() != nullptr, ChipLogError(DeviceLayer, "FAIL: NULL mAdapter in %s", __func__));

// NOLINTNEXTLINE(bugprone-casting-through-void)
adapterObject = g_dbus_interface_get_object(G_DBUS_INTERFACE(mAdapter.get()));
VerifyOrExit(adapterObject != nullptr, ChipLogError(DeviceLayer, "FAIL: NULL adapterObject in %s", __func__));

// NOLINTNEXTLINE(bugprone-casting-through-void)
advMgr.reset(bluez_object_get_leadvertising_manager1(BLUEZ_OBJECT(adapterObject)));
VerifyOrExit(advMgr.get() != nullptr, ChipLogError(DeviceLayer, "FAIL: NULL advMgr in %s", __func__));

Expand Down
Loading
Loading