Skip to content

Commit

Permalink
Make all public FabricInfo getters const (#20088)
Browse files Browse the repository at this point in the history
* Make all public FabricInfo getters const

- Only FabricTable APIs are allowed to mutate FabricInfo entries
  due to the requirements of fail-safe shadow data handling
- Previous PRs made sure none of the mutators were used outside
  of FabricTable, but `FindFabricByIndex` remained a non-const
  getter due to how much code was using it.

Fixes #19929

This PR:

- Renames the mutable `FabricInfo *` getter as `GetMutableFabricByIndex()`
  and makes it private to FabricTable.
- Adds `const` qualifier to every other usage of FindFabricByIndex in the SDK.

Testing done:

- All unit tests pass
- Cert tests still pass
- Still compiles

* Fix CI on Shell

* Fix one const in ESP32 ShellCommands.h
  • Loading branch information
tcarmelveilleux authored and pull[bot] committed Feb 28, 2024
1 parent b637740 commit 1356518
Show file tree
Hide file tree
Showing 18 changed files with 70 additions and 73 deletions.
12 changes: 6 additions & 6 deletions examples/all-clusters-app/esp32/main/include/ShellCommands.h
Original file line number Diff line number Diff line change
Expand Up @@ -125,10 +125,10 @@ class CASECommands
// Register the CASESession commands
void Register();

void SetFabricInfo(FabricInfo * fabricInfo) { mFabricInfo = fabricInfo; }
void SetFabricInfo(const FabricInfo * fabricInfo) { mFabricInfo = fabricInfo; }
void SetNodeId(NodeId nodeId) { mNodeId = nodeId; }
void SetOnConnecting(bool onConnecting) { mOnConnecting = onConnecting; }
FabricInfo * GetFabricInfo(void) { return mFabricInfo; }
const FabricInfo * GetFabricInfo(void) { return mFabricInfo; }
NodeId GetNodeId(void) { return mNodeId; }
bool GetOnConnecting(void) { return mOnConnecting; }

Expand Down Expand Up @@ -167,7 +167,7 @@ class CASECommands
return CHIP_ERROR_INCORRECT_STATE;
}
const FabricIndex fabricIndex = static_cast<FabricIndex>(strtoul(argv[0], nullptr, 10));
FabricInfo * fabricInfo = Server::GetInstance().GetFabricTable().FindFabricWithIndex(fabricIndex);
const FabricInfo * fabricInfo = Server::GetInstance().GetFabricTable().FindFabricWithIndex(fabricIndex);

if (fabricInfo == nullptr)
{
Expand Down Expand Up @@ -201,9 +201,9 @@ class CASECommands
static Callback::Callback<OnDeviceConnected> sOnConnectedCallback;
static Callback::Callback<OnDeviceConnectionFailure> sOnConnectionFailureCallback;
static Shell::Engine sSubShell;
FabricInfo * mFabricInfo = nullptr;
NodeId mNodeId = 0;
bool mOnConnecting = false;
const FabricInfo * mFabricInfo = nullptr;
NodeId mNodeId = 0;
bool mOnConnecting = false;
};

} // namespace Shell
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -125,10 +125,10 @@ class CASECommands
// Register the CASESession commands
void Register();

void SetFabricInfo(FabricInfo * fabricInfo) { mFabricInfo = fabricInfo; }
void SetFabricInfo(const FabricInfo * fabricInfo) { mFabricInfo = fabricInfo; }
void SetNodeId(NodeId nodeId) { mNodeId = nodeId; }
void SetOnConnecting(bool onConnecting) { mOnConnecting = onConnecting; }
FabricInfo * GetFabricInfo(void) { return mFabricInfo; }
const FabricInfo * GetFabricInfo(void) { return mFabricInfo; }
NodeId GetNodeId(void) { return mNodeId; }
bool GetOnConnecting(void) { return mOnConnecting; }

Expand Down Expand Up @@ -167,7 +167,7 @@ class CASECommands
return CHIP_ERROR_INCORRECT_STATE;
}
const FabricIndex fabricIndex = static_cast<FabricIndex>(strtoul(argv[0], nullptr, 10));
FabricInfo * fabricInfo = Server::GetInstance().GetFabricTable().FindFabricWithIndex(fabricIndex);
const FabricInfo * fabricInfo = Server::GetInstance().GetFabricTable().FindFabricWithIndex(fabricIndex);

if (fabricInfo == nullptr)
{
Expand Down Expand Up @@ -201,9 +201,9 @@ class CASECommands
static Callback::Callback<OnDeviceConnected> sOnConnectedCallback;
static Callback::Callback<OnDeviceConnectionFailure> sOnConnectionFailureCallback;
static Shell::Engine sSubShell;
FabricInfo * mFabricInfo = nullptr;
NodeId mNodeId = 0;
bool mOnConnecting = false;
const FabricInfo * mFabricInfo = nullptr;
NodeId mNodeId = 0;
bool mOnConnecting = false;
};

} // namespace Shell
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -247,9 +247,9 @@ void OTAProviderExample::SendQueryImageResponse(app::CommandHandler * commandObj

// TODO: This uses the current node as the provider to supply the OTA image. This can be configurable such that the
// provider supplying the response is not the provider supplying the OTA image.
FabricIndex fabricIndex = commandObj->GetAccessingFabricIndex();
FabricInfo * fabricInfo = Server::GetInstance().GetFabricTable().FindFabricWithIndex(fabricIndex);
NodeId nodeId = fabricInfo->GetPeerId().GetNodeId();
FabricIndex fabricIndex = commandObj->GetAccessingFabricIndex();
const FabricInfo * fabricInfo = Server::GetInstance().GetFabricTable().FindFabricWithIndex(fabricIndex);
NodeId nodeId = fabricInfo->GetPeerId().GetNodeId();

// Generate the ImageURI if one is not already preset
if (strlen(mImageUri) == 0)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ CHIP_ERROR ModelCommand::RunCommand()
}

Server * server = &(chip::Server::GetInstance());
chip::FabricInfo * fabric = server->GetFabricTable().FindFabricWithIndex(fabricIndex);
const FabricInfo * fabric = server->GetFabricTable().FindFabricWithIndex(fabricIndex);
if (fabric == nullptr)
{
ChipLogError(AppServer, "Did not find fabric for index %d", fabricIndex);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ CHIP_ERROR TargetVideoPlayerInfo::Initialize(NodeId nodeId, FabricIndex fabricIn
}

Server * server = &(chip::Server::GetInstance());
chip::FabricInfo * fabric = server->GetFabricTable().FindFabricWithIndex(fabricIndex);
const FabricInfo * fabric = server->GetFabricTable().FindFabricWithIndex(fabricIndex);
if (fabric == nullptr)
{
ChipLogError(AppServer, "Did not find fabric for index %d", fabricIndex);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -105,10 +105,10 @@ bool emberAfAdministratorCommissioningClusterOpenCommissioningWindowCallback(

ChipLogProgress(Zcl, "Received command to open commissioning window");

FabricIndex fabricIndex = commandObj->GetAccessingFabricIndex();
FabricInfo * fabricInfo = Server::GetInstance().GetFabricTable().FindFabricWithIndex(fabricIndex);
auto & failSafeContext = Server::GetInstance().GetFailSafeContext();
auto & commissionMgr = Server::GetInstance().GetCommissioningWindowManager();
FabricIndex fabricIndex = commandObj->GetAccessingFabricIndex();
const FabricInfo * fabricInfo = Server::GetInstance().GetFabricTable().FindFabricWithIndex(fabricIndex);
auto & failSafeContext = Server::GetInstance().GetFailSafeContext();
auto & commissionMgr = Server::GetInstance().GetCommissioningWindowManager();

VerifyOrExit(fabricInfo != nullptr, status.Emplace(StatusCode::EMBER_ZCL_STATUS_CODE_PAKE_PARAMETER_ERROR));
VerifyOrExit(!failSafeContext.IsFailSafeArmed(), status.Emplace(StatusCode::EMBER_ZCL_STATUS_CODE_BUSY));
Expand Down Expand Up @@ -167,10 +167,10 @@ bool emberAfAdministratorCommissioningClusterOpenBasicCommissioningWindowCallbac
InteractionModel::Status globalStatus = InteractionModel::Status::Success;
ChipLogProgress(Zcl, "Received command to open basic commissioning window");

FabricIndex fabricIndex = commandObj->GetAccessingFabricIndex();
FabricInfo * fabricInfo = Server::GetInstance().GetFabricTable().FindFabricWithIndex(fabricIndex);
auto & failSafeContext = Server::GetInstance().GetFailSafeContext();
auto & commissionMgr = Server::GetInstance().GetCommissioningWindowManager();
FabricIndex fabricIndex = commandObj->GetAccessingFabricIndex();
const FabricInfo * fabricInfo = Server::GetInstance().GetFabricTable().FindFabricWithIndex(fabricIndex);
auto & failSafeContext = Server::GetInstance().GetFailSafeContext();
auto & commissionMgr = Server::GetInstance().GetCommissioningWindowManager();

VerifyOrExit(fabricInfo != nullptr, status.Emplace(StatusCode::EMBER_ZCL_STATUS_CODE_PAKE_PARAMETER_ERROR));

Expand Down
4 changes: 2 additions & 2 deletions src/app/clusters/bindings/BindingManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -52,7 +52,7 @@ namespace {

chip::PeerId PeerIdForNode(chip::FabricTable * fabricTable, chip::FabricIndex fabric, chip::NodeId node)
{
chip::FabricInfo * fabricInfo = fabricTable->FindFabricWithIndex(fabric);
const chip::FabricInfo * fabricInfo = fabricTable->FindFabricWithIndex(fabric);
if (fabricInfo == nullptr)
{
return chip::PeerId();
Expand Down Expand Up @@ -202,7 +202,7 @@ CHIP_ERROR BindingManager::NotifyBoundClusterChanged(EndpointId endpoint, Cluste
{
if (iter->type == EMBER_UNICAST_BINDING)
{
FabricInfo * fabricInfo = mInitParams.mFabricTable->FindFabricWithIndex(iter->fabricIndex);
const FabricInfo * fabricInfo = mInitParams.mFabricTable->FindFabricWithIndex(iter->fabricIndex);
VerifyOrReturnError(fabricInfo != nullptr, CHIP_ERROR_NOT_FOUND);
PeerId peer = fabricInfo->GetPeerIdForNode(iter->nodeId);
OperationalDeviceProxy * peerDevice = mInitParams.mCASESessionManager->FindExistingSession(peer);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -244,7 +244,7 @@ CHIP_ERROR OperationalCredentialsAttrAccess::Read(const ConcreteReadAttributePat
return CHIP_NO_ERROR;
}

FabricInfo * RetrieveCurrentFabric(CommandHandler * aCommandHandler)
const FabricInfo * RetrieveCurrentFabric(CommandHandler * aCommandHandler)
{
FabricIndex index = aCommandHandler->GetAccessingFabricIndex();
ChipLogDetail(Zcl, "OpCreds: Finding fabric with fabricIndex 0x%x", static_cast<unsigned>(index));
Expand Down Expand Up @@ -282,7 +282,7 @@ void FailSafeCleanup(const chip::DeviceLayer::ChipDeviceEvent * event)
CASESessionManager * caseSessionManager = Server::GetInstance().GetCASESessionManager();
if (caseSessionManager)
{
FabricInfo * fabricInfo = Server::GetInstance().GetFabricTable().FindFabricWithIndex(fabricIndex);
const FabricInfo * fabricInfo = Server::GetInstance().GetFabricTable().FindFabricWithIndex(fabricIndex);
VerifyOrReturn(fabricInfo != nullptr);

caseSessionManager->ReleaseSessionsForFabric(fabricInfo->GetFabricIndex());
Expand Down Expand Up @@ -489,7 +489,7 @@ bool emberAfOperationalCredentialsClusterUpdateFabricLabelCallback(app::CommandH
CHIP_ERROR err = CHIP_ERROR_INTERNAL;

// Fetch current fabric
FabricInfo * fabric = RetrieveCurrentFabric(commandObj);
const FabricInfo * fabric = RetrieveCurrentFabric(commandObj);
if (fabric == nullptr)
{
SendNOCResponse(commandObj, commandPath, OperationalCertStatus::kInsufficientPrivilege, ourFabricIndex,
Expand Down Expand Up @@ -607,8 +607,8 @@ bool emberAfOperationalCredentialsClusterAddNOCCallback(app::CommandHandler * co
CHIP_ERROR err = CHIP_NO_ERROR;
FabricIndex newFabricIndex = kUndefinedFabricIndex;
Credentials::GroupDataProvider::KeySet keyset;
FabricInfo * newFabricInfo = nullptr;
auto & fabricTable = Server::GetInstance().GetFabricTable();
const FabricInfo * newFabricInfo = nullptr;
auto & fabricTable = Server::GetInstance().GetFabricTable();

auto * secureSession = commandObj->GetExchangeContext()->GetSessionHandle()->AsSecureSession();
auto & failSafeContext = Server::GetInstance().GetFailSafeContext();
Expand Down Expand Up @@ -781,9 +781,9 @@ bool emberAfOperationalCredentialsClusterUpdateNOCCallback(app::CommandHandler *

ChipLogProgress(Zcl, "OpCreds: Received an UpdateNOC command");

auto & fabricTable = Server::GetInstance().GetFabricTable();
auto & failSafeContext = Server::GetInstance().GetFailSafeContext();
FabricInfo * fabricInfo = RetrieveCurrentFabric(commandObj);
auto & fabricTable = Server::GetInstance().GetFabricTable();
auto & failSafeContext = Server::GetInstance().GetFailSafeContext();
const FabricInfo * fabricInfo = RetrieveCurrentFabric(commandObj);

bool csrWasForUpdateNoc = false; //< Output param of HasPendingOperationalKey
bool hasPendingKey = fabricTable.HasPendingOperationalKey(csrWasForUpdateNoc);
Expand Down Expand Up @@ -1011,7 +1011,7 @@ bool emberAfOperationalCredentialsClusterCSRRequestCallback(app::CommandHandler
commandObj->GetExchangeContext()->GetSessionHandle()->AsSecureSession()->GetCryptoContext().GetAttestationChallenge();

failSafeContext.SetCsrRequestForUpdateNoc(isForUpdateNoc);
FabricInfo * fabricInfo = RetrieveCurrentFabric(commandObj);
const FabricInfo * fabricInfo = RetrieveCurrentFabric(commandObj);

VerifyOrExit(CSRNonce.size() == Credentials::kExpectedAttestationNonceSize, finalStatus = Status::InvalidCommand);

Expand Down
6 changes: 3 additions & 3 deletions src/app/clusters/ota-requestor/DefaultOTARequestor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -363,7 +363,7 @@ void DefaultOTARequestor::ConnectToProvider(OnConnectedAction onConnectedAction)
return;
}

FabricInfo * fabricInfo = mServer->GetFabricTable().FindFabricWithIndex(mProviderLocation.Value().fabricIndex);
const FabricInfo * fabricInfo = mServer->GetFabricTable().FindFabricWithIndex(mProviderLocation.Value().fabricIndex);

if (fabricInfo == nullptr)
{
Expand Down Expand Up @@ -393,7 +393,7 @@ void DefaultOTARequestor::DisconnectFromProvider()
return;
}

FabricInfo * fabricInfo = mServer->GetFabricTable().FindFabricWithIndex(mProviderLocation.Value().fabricIndex);
const FabricInfo * fabricInfo = mServer->GetFabricTable().FindFabricWithIndex(mProviderLocation.Value().fabricIndex);
if (fabricInfo == nullptr)
{
ChipLogError(SoftwareUpdate, "Cannot find fabric");
Expand Down Expand Up @@ -726,7 +726,7 @@ CHIP_ERROR DefaultOTARequestor::GenerateUpdateToken()
VerifyOrReturnError(mServer != nullptr, CHIP_ERROR_INCORRECT_STATE);
VerifyOrReturnError(mProviderLocation.HasValue(), CHIP_ERROR_INCORRECT_STATE);

FabricInfo * fabricInfo = mServer->GetFabricTable().FindFabricWithIndex(mProviderLocation.Value().fabricIndex);
const FabricInfo * fabricInfo = mServer->GetFabricTable().FindFabricWithIndex(mProviderLocation.Value().fabricIndex);
VerifyOrReturnError(fabricInfo != nullptr, CHIP_ERROR_INCORRECT_STATE);

static_assert(sizeof(NodeId) == sizeof(uint64_t), "Unexpected NodeId size");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ CHIP_ERROR ExtendedOTARequestorDriver::GetUserConsentSubject(chip::ota::UserCons
return CHIP_ERROR_INTERNAL;
}

FabricInfo * fabricInfo = Server::GetInstance().GetFabricTable().FindFabricWithIndex(subject.fabricIndex);
const FabricInfo * fabricInfo = Server::GetInstance().GetFabricTable().FindFabricWithIndex(subject.fabricIndex);
if (fabricInfo == nullptr)
{
ChipLogError(SoftwareUpdate, "Cannot find fabric");
Expand Down
4 changes: 2 additions & 2 deletions src/app/server/Server.h
Original file line number Diff line number Diff line change
Expand Up @@ -370,7 +370,7 @@ class Server

void OnGroupAdded(chip::FabricIndex fabric_index, const Credentials::GroupDataProvider::GroupInfo & new_group) override
{
FabricInfo * fabric = mServer->GetFabricTable().FindFabricWithIndex(fabric_index);
const FabricInfo * fabric = mServer->GetFabricTable().FindFabricWithIndex(fabric_index);
if (fabric == nullptr)
{
ChipLogError(AppServer, "Group added to nonexistent fabric?");
Expand All @@ -386,7 +386,7 @@ class Server

void OnGroupRemoved(chip::FabricIndex fabric_index, const Credentials::GroupDataProvider::GroupInfo & old_group) override
{
FabricInfo * fabric = mServer->GetFabricTable().FindFabricWithIndex(fabric_index);
const FabricInfo * fabric = mServer->GetFabricTable().FindFabricWithIndex(fabric_index);
if (fabric == nullptr)
{
ChipLogError(AppServer, "Group added to nonexistent fabric?");
Expand Down
4 changes: 2 additions & 2 deletions src/app/tests/TestOperationalDeviceProxy.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -51,8 +51,8 @@ void TestOperationalDeviceProxy_EstablishSessionDirectly(nlTestSuite * inSuite,
System::LayerImpl systemLayer;
// Heap-allocate the fairly large FabricTable so we don't end up with a huge
// stack.
FabricTable * fabrics = Platform::New<FabricTable>();
FabricInfo * fabric = fabrics->FindFabricWithIndex(1);
FabricTable * fabrics = Platform::New<FabricTable>();
const FabricInfo * fabric = fabrics->FindFabricWithIndex(1);
VerifyOrDie(fabric != nullptr);
secure_channel::MessageCounterManager messageCounterManager;
chip::TestPersistentStorageDelegate deviceStorage;
Expand Down
16 changes: 8 additions & 8 deletions src/credentials/FabricTable.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -467,7 +467,7 @@ const FabricInfo * FabricTable::FindFabric(const Crypto::P256PublicKey & rootPub
return nullptr;
}

FabricInfo * FabricTable::FindFabricWithIndex(FabricIndex fabricIndex)
FabricInfo * FabricTable::GetMutableFabricByIndex(FabricIndex fabricIndex)
{
// Try to match pending fabric first if available
if (HasPendingFabricUpdate() && (mPendingFabric.GetFabricIndex() == fabricIndex))
Expand Down Expand Up @@ -870,13 +870,13 @@ CHIP_ERROR FabricTable::Delete(FabricIndex fabricIndex)
VerifyOrReturnError(mStorage != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
VerifyOrReturnError(IsValidFabricIndex(fabricIndex), CHIP_ERROR_INVALID_ARGUMENT);

FabricInfo * fabricInfo = FindFabricWithIndex(fabricIndex);
FabricInfo * fabricInfo = GetMutableFabricByIndex(fabricIndex);
if (fabricInfo == &mPendingFabric)
{
// Asked to Delete while pending an update: reset the pending state and
// get back to the underlying fabric data for existing fabric.
RevertPendingFabricData();
fabricInfo = FindFabricWithIndex(fabricIndex);
fabricInfo = GetMutableFabricByIndex(fabricIndex);
}

bool fabricIsInitialized = fabricInfo != nullptr && fabricInfo->IsInitialized();
Expand Down Expand Up @@ -1053,7 +1053,7 @@ void FabricTable::Forget(FabricIndex fabricIndex)
{
ChipLogProgress(FabricProvisioning, "Forgetting fabric 0x%x", static_cast<unsigned>(fabricIndex));

auto * fabricInfo = FindFabricWithIndex(fabricIndex);
auto * fabricInfo = GetMutableFabricByIndex(fabricIndex);
VerifyOrReturn(fabricInfo != nullptr);

RevertPendingFabricData();
Expand Down Expand Up @@ -1741,7 +1741,7 @@ CHIP_ERROR FabricTable::CommitPendingFabricData()
}

// Make sure we actually have a pending fabric
FabricInfo * pendingFabricEntry = FindFabricWithIndex(fabricIndexBeingCommitted);
FabricInfo * pendingFabricEntry = GetMutableFabricByIndex(fabricIndexBeingCommitted);

if (isUpdating && hasPending && !hasInvalidInternalState)
{
Expand Down Expand Up @@ -1828,7 +1828,7 @@ CHIP_ERROR FabricTable::CommitPendingFabricData()
if (isUpdating)
{
// This will get the non-pending fabric
FabricInfo * existingFabricToUpdate = FindFabricWithIndex(fabricIndexBeingCommitted);
FabricInfo * existingFabricToUpdate = GetMutableFabricByIndex(fabricIndexBeingCommitted);

// Multiple interlocks validated the below, so it's fatal if we are somehow incoherent here
VerifyOrDie((existingFabricToUpdate != nullptr) && (existingFabricToUpdate != &mPendingFabric));
Expand All @@ -1839,7 +1839,7 @@ CHIP_ERROR FabricTable::CommitPendingFabricData()
}

// Store pending metadata first
FabricInfo * liveFabricEntry = FindFabricWithIndex(fabricIndexBeingCommitted);
FabricInfo * liveFabricEntry = GetMutableFabricByIndex(fabricIndexBeingCommitted);
VerifyOrDie(liveFabricEntry != nullptr);

CHIP_ERROR metadataErr = StoreFabricMetadata(liveFabricEntry);
Expand Down Expand Up @@ -2000,7 +2000,7 @@ CHIP_ERROR FabricTable::SetFabricLabel(FabricIndex fabricIndex, const CharSpan &

ReturnErrorCodeIf(fabricLabel.size() > kFabricLabelMaxLengthInBytes, CHIP_ERROR_INVALID_ARGUMENT);

FabricInfo * fabricInfo = FindFabricWithIndex(fabricIndex);
FabricInfo * fabricInfo = GetMutableFabricByIndex(fabricIndex);
bool fabricIsInitialized = (fabricInfo != nullptr) && fabricInfo->IsInitialized();
VerifyOrReturnError(fabricIsInitialized, CHIP_ERROR_INCORRECT_STATE);

Expand Down
Loading

0 comments on commit 1356518

Please sign in to comment.