Skip to content

Commit

Permalink
Optionally avoid global constructors (#29071)
Browse files Browse the repository at this point in the history
* Use SystemClock() instead of creating a separate instance

* Add chip::Global to make initialization of globals configurable

The behavior of chip::Global can be configured via the platform
configuration via the settings

  CHIP_CONFIG_GLOBALS_LAZY_INIT
  CHIP_CONFIG_GLOBALS_NO_DESTRUCT

Both default to 0, retaining normal C++ global init behavior.

* Darwin: Adopt chip::Global for platform / framework

* Adopt chip::Global for core singletons with non-trivial constructors / destructors

Note that there is a slight change to the API of ArrayAttestationTrustStore
to make it easier to create and pass a constexpr array of root certificates.

* Make Global::get() public and remove operator*

* Avoid changing ArrayAttestationTrustStore API...

... by moving kTestAttestationTrustStoreRoots (formerly kTestPaaRoots) into
CHIPAttCert_test_vectors.cpp where the certificate spans themselves are
defined.

* Comment wording

Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>

---------

Co-authored-by: Boris Zbarsky <bzbarsky@apple.com>
  • Loading branch information
ksperling-apple and bzbarsky-apple committed Sep 19, 2023
1 parent 053b0cc commit 3232412
Show file tree
Hide file tree
Showing 22 changed files with 237 additions and 74 deletions.
15 changes: 8 additions & 7 deletions src/access/AccessControl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -23,18 +23,19 @@

#include "AccessControl.h"

#include <lib/core/Global.h>

namespace chip {
namespace Access {

using chip::CATValues;
using chip::FabricIndex;
using chip::NodeId;
using namespace chip::Access;

namespace {

AccessControl defaultAccessControl;
AccessControl * globalAccessControl = &defaultAccessControl;
Global<AccessControl> defaultAccessControl;
AccessControl * globalAccessControl = nullptr; // lazily defaulted to defaultAccessControl in GetAccessControl

static_assert(((unsigned(Privilege::kAdminister) & unsigned(Privilege::kManage)) == 0) &&
((unsigned(Privilege::kAdminister) & unsigned(Privilege::kOperate)) == 0) &&
Expand Down Expand Up @@ -174,8 +175,8 @@ char GetPrivilegeStringForLogging(Privilege privilege)

} // namespace

AccessControl::Entry::Delegate AccessControl::Entry::mDefaultDelegate;
AccessControl::EntryIterator::Delegate AccessControl::EntryIterator::mDefaultDelegate;
Global<AccessControl::Entry::Delegate> AccessControl::Entry::mDefaultDelegate;
Global<AccessControl::EntryIterator::Delegate> AccessControl::EntryIterator::mDefaultDelegate;

CHIP_ERROR AccessControl::Init(AccessControl::Delegate * delegate, DeviceTypeResolver & deviceTypeResolver)
{
Expand Down Expand Up @@ -631,7 +632,7 @@ void AccessControl::NotifyEntryChanged(const SubjectDescriptor * subjectDescript

AccessControl & GetAccessControl()
{
return *globalAccessControl;
return (globalAccessControl) ? *globalAccessControl : defaultAccessControl.get();
}

void SetAccessControl(AccessControl & accessControl)
Expand All @@ -642,7 +643,7 @@ void SetAccessControl(AccessControl & accessControl)

void ResetAccessControlToDefault()
{
globalAccessControl = &defaultAccessControl;
globalAccessControl = nullptr;
}

} // namespace Access
Expand Down
21 changes: 11 additions & 10 deletions src/access/AccessControl.h
Original file line number Diff line number Diff line change
Expand Up @@ -22,8 +22,9 @@
#include "RequestPath.h"
#include "SubjectDescriptor.h"

#include "lib/support/CodeUtils.h"
#include <lib/core/CHIPCore.h>
#include <lib/core/Global.h>
#include <lib/support/CodeUtils.h>

// Dump function for use during development only (0 for disabled, non-zero for enabled).
#define CHIP_ACCESS_CONTROL_DUMP_ENABLED 0
Expand Down Expand Up @@ -104,15 +105,15 @@ class AccessControl

Entry() = default;

Entry(Entry && other) : mDelegate(other.mDelegate) { other.mDelegate = &mDefaultDelegate; }
Entry(Entry && other) : mDelegate(other.mDelegate) { other.mDelegate = &mDefaultDelegate.get(); }

Entry & operator=(Entry && other)
{
if (this != &other)
{
mDelegate->Release();
mDelegate = other.mDelegate;
other.mDelegate = &mDefaultDelegate;
other.mDelegate = &mDefaultDelegate.get();
}
return *this;
}
Expand Down Expand Up @@ -208,7 +209,7 @@ class AccessControl
*/
CHIP_ERROR RemoveTarget(size_t index) { return mDelegate->RemoveTarget(index); }

bool HasDefaultDelegate() const { return mDelegate == &mDefaultDelegate; }
bool HasDefaultDelegate() const { return mDelegate == &mDefaultDelegate.get(); }

const Delegate & GetDelegate() const { return *mDelegate; }

Expand All @@ -223,12 +224,12 @@ class AccessControl
void ResetDelegate()
{
mDelegate->Release();
mDelegate = &mDefaultDelegate;
mDelegate = &mDefaultDelegate.get();
}

private:
static Delegate mDefaultDelegate;
Delegate * mDelegate = &mDefaultDelegate;
static Global<Delegate> mDefaultDelegate;
Delegate * mDelegate = &mDefaultDelegate.get();
};

/**
Expand Down Expand Up @@ -276,12 +277,12 @@ class AccessControl
void ResetDelegate()
{
mDelegate->Release();
mDelegate = &mDefaultDelegate;
mDelegate = &mDefaultDelegate.get();
}

private:
static Delegate mDefaultDelegate;
Delegate * mDelegate = &mDefaultDelegate;
static Global<Delegate> mDefaultDelegate;
Delegate * mDelegate = &mDefaultDelegate.get();
};

/**
Expand Down
5 changes: 3 additions & 2 deletions src/app/InteractionModelEngine.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
#include <app/RequiredPrivilege.h>
#include <app/util/af-types.h>
#include <app/util/endpoint-config-api.h>
#include <lib/core/Global.h>
#include <lib/core/TLVUtilities.h>
#include <lib/support/CodeUtils.h>
#include <lib/support/FibonacciUtils.h>
Expand All @@ -41,13 +42,13 @@ namespace app {

using Protocols::InteractionModel::Status;

InteractionModelEngine sInteractionModelEngine;
Global<InteractionModelEngine> sInteractionModelEngine;

InteractionModelEngine::InteractionModelEngine() {}

InteractionModelEngine * InteractionModelEngine::GetInstance()
{
return &sInteractionModelEngine;
return &sInteractionModelEngine.get();
}

CHIP_ERROR InteractionModelEngine::Init(Messaging::ExchangeManager * apExchangeMgr, FabricTable * apFabricTable,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -370,8 +370,7 @@ Status Delegate::OnResetCondition()
if (emberAfContainsAttribute(mInstance->GetEndpointId(), mInstance->GetClusterId(), Attributes::LastChangedTime::Id))
{
System::Clock::Milliseconds64 currentUnixTimeMS;
System::Clock::ClockImpl clock;
CHIP_ERROR err = clock.GetClock_RealTimeMS(currentUnixTimeMS);
CHIP_ERROR err = System::SystemClock().GetClock_RealTimeMS(currentUnixTimeMS);
if (err == CHIP_NO_ERROR)
{
System::Clock::Seconds32 currentUnixTime = std::chrono::duration_cast<System::Clock::Seconds32>(currentUnixTimeMS);
Expand Down
15 changes: 12 additions & 3 deletions src/app/dynamic_server/AccessControl.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#include <app-common/zap-generated/ids/Clusters.h>
#include <app/InteractionModelEngine.h>
#include <lib/core/CHIPError.h>
#include <lib/core/Global.h>

using namespace chip;
using namespace chip::Access;
Expand All @@ -40,7 +41,7 @@ class DeviceTypeResolver : public Access::AccessControl::DeviceTypeResolver
{
return app::IsDeviceTypeOnEndpoint(deviceType, endpoint);
}
} gDeviceTypeResolver;
};

// TODO: Make the policy more configurable by consumers.
class AccessControlDelegate : public Access::AccessControl::Delegate
Expand Down Expand Up @@ -73,15 +74,23 @@ class AccessControlDelegate : public Access::AccessControl::Delegate
}
};

AccessControlDelegate gDelegate;
struct ControllerAccessControl
{
DeviceTypeResolver mDeviceTypeResolver;
AccessControlDelegate mDelegate;
ControllerAccessControl() { GetAccessControl().Init(&mDelegate, mDeviceTypeResolver); }
};

Global<ControllerAccessControl> gControllerAccessControl;

} // anonymous namespace

namespace chip {
namespace app {
namespace dynamic_server {
void InitAccessControl()
{
GetAccessControl().Init(&gDelegate, gDeviceTypeResolver);
gControllerAccessControl.get(); // force initialization
}
} // namespace dynamic_server
} // namespace app
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,21 +22,21 @@
#include <credentials/DeviceAttestationConstructor.h>
#include <credentials/DeviceAttestationVendorReserved.h>
#include <crypto/CHIPCryptoPAL.h>
#include <string.h>

#include <lib/core/CHIPError.h>
#include <lib/core/Global.h>
#include <lib/support/CodeUtils.h>
#include <lib/support/ScopedBuffer.h>
#include <lib/support/Span.h>

namespace chip {
namespace TestCerts {
extern const ByteSpan sTestCert_PAA_FFF1_Cert;
extern const ByteSpan sTestCert_PAA_NoVID_Cert;
extern const Span<const ByteSpan> kTestAttestationTrustStoreRoots;
} // namespace TestCerts
} // namespace chip

using namespace chip::Crypto;
using chip::TestCerts::kTestAttestationTrustStoreRoots;

namespace chip {
namespace Credentials {
Expand All @@ -46,11 +46,6 @@ namespace {
// As per specifications section 11.22.5.1. Constant RESP_MAX
constexpr size_t kMaxResponseLength = 900;

static const ByteSpan kTestPaaRoots[] = {
TestCerts::sTestCert_PAA_FFF1_Cert,
TestCerts::sTestCert_PAA_NoVID_Cert,
};

// Test CD Signing Key from `credentials/test/certification-declaration/Chip-Test-CD-Signing-Cert.pem`
// used to verify any in-SDK development CDs. The associated keypair to do actual signing is in
// `credentials/test/certification-declaration/Chip-Test-CD-Signing-Key.pem`.
Expand Down Expand Up @@ -268,7 +263,7 @@ struct MatterCDSigningKey
const P256PublicKeySpan mPubkey;
};

std::array<MatterCDSigningKey, 6> gCdSigningKeys = { {
constexpr std::array<MatterCDSigningKey, 6> gCdSigningKeys = { {
{ FixedByteSpan<20>{ gTestCdPubkeyKid }, FixedByteSpan<65>{ gTestCdPubkeyBytes } },
{ FixedByteSpan<20>{ gCdSigningKey001Kid }, FixedByteSpan<65>{ gCdSigningKey001PubkeyBytes } },
{ FixedByteSpan<20>{ gCdSigningKey002Kid }, FixedByteSpan<65>{ gCdSigningKey002PubkeyBytes } },
Expand All @@ -277,7 +272,13 @@ std::array<MatterCDSigningKey, 6> gCdSigningKeys = { {
{ FixedByteSpan<20>{ gCdSigningKey005Kid }, FixedByteSpan<65>{ gCdSigningKey005PubkeyBytes } },
} };

const ArrayAttestationTrustStore kTestAttestationTrustStore{ &kTestPaaRoots[0], ArraySize(kTestPaaRoots) };
struct TestAttestationTrustStore final : public ArrayAttestationTrustStore
{
TestAttestationTrustStore() :
ArrayAttestationTrustStore(kTestAttestationTrustStoreRoots.data(), kTestAttestationTrustStoreRoots.size())
{}
};
Global<TestAttestationTrustStore> gTestAttestationTrustStore;

AttestationVerificationResult MapError(CertificateChainValidationResult certificateChainValidationResult)
{
Expand Down Expand Up @@ -397,7 +398,7 @@ void DefaultDACVerifier::VerifyAttestationInformation(const DeviceAttestationVer

if (err == CHIP_ERROR_NOT_IMPLEMENTED)
{
VerifyOrExit(kTestAttestationTrustStore.GetProductAttestationAuthorityCert(akid, paaDerBuffer) == CHIP_NO_ERROR,
VerifyOrExit(gTestAttestationTrustStore->GetProductAttestationAuthorityCert(akid, paaDerBuffer) == CHIP_NO_ERROR,
attestationError = AttestationVerificationResult::kPaaNotFound);
}

Expand Down Expand Up @@ -686,7 +687,7 @@ CHIP_ERROR CsaCdKeysTrustStore::LookupVerifyingKey(const ByteSpan & kid, Crypto:

const AttestationTrustStore * GetTestAttestationTrustStore()
{
return &kTestAttestationTrustStore;
return &gTestAttestationTrustStore.get();
}

DeviceAttestationVerifier * GetDefaultDACVerifier(const AttestationTrustStore * paaRootStore)
Expand Down
5 changes: 5 additions & 0 deletions src/credentials/tests/CHIPAttCert_test_vectors.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -4289,5 +4289,10 @@ constexpr uint8_t sTestCert_PAI_FFF2_NoPID_Resigned_SKID_Array[] = {

extern const ByteSpan sTestCert_PAI_FFF2_NoPID_Resigned_SKID = ByteSpan(sTestCert_PAI_FFF2_NoPID_Resigned_SKID_Array);

extern constexpr Span<const ByteSpan> kTestAttestationTrustStoreRoots((const ByteSpan[]){
sTestCert_PAA_FFF1_Cert,
sTestCert_PAA_NoVID_Cert,
});

} // namespace TestCerts
} // namespace chip
3 changes: 3 additions & 0 deletions src/credentials/tests/CHIPAttCert_test_vectors.h
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,9 @@
namespace chip {
namespace TestCerts {

// Root CA certs for chip::Credentials::GetTestAttestationTrustStore()
extern const Span<const ByteSpan> kTestAttestationTrustStoreRoots;

extern const ByteSpan sTestCert_DAC_FFF1_8000_0000_2CDPs_Cert;
extern const ByteSpan sTestCert_DAC_FFF1_8000_0000_2CDPs_SKID;
extern const ByteSpan sTestCert_DAC_FFF1_8000_0000_2CDPs_PublicKey;
Expand Down
19 changes: 10 additions & 9 deletions src/darwin/Framework/CHIP/MTROTAProviderDelegateBridge.mm
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@

#include <app/clusters/ota-provider/ota-provider.h>
#include <controller/CHIPDeviceController.h>
#include <lib/core/Global.h>
#include <lib/support/TypeTraits.h>
#include <platform/PlatformManager.h>
#include <protocols/interaction_model/Constants.h>
Expand Down Expand Up @@ -492,29 +493,29 @@ CHIP_ERROR ConfigureState(chip::FabricIndex fabricIndex, chip::NodeId nodeId)
};

namespace {
BdxOTASender gOtaSender;
Global<BdxOTASender> gOtaSender;

NSInteger const kOtaProviderEndpoint = 0;
NSInteger constexpr kOtaProviderEndpoint = 0;
} // anonymous namespace

MTROTAProviderDelegateBridge::MTROTAProviderDelegateBridge() { Clusters::OTAProvider::SetDelegate(kOtaProviderEndpoint, this); }

MTROTAProviderDelegateBridge::~MTROTAProviderDelegateBridge()
{
gOtaSender.ResetState();
gOtaSender->ResetState();
Clusters::OTAProvider::SetDelegate(kOtaProviderEndpoint, nullptr);
}

CHIP_ERROR MTROTAProviderDelegateBridge::Init(System::Layer * systemLayer, Messaging::ExchangeManager * exchangeManager)
{
return gOtaSender.Init(systemLayer, exchangeManager);
return gOtaSender->Init(systemLayer, exchangeManager);
}

void MTROTAProviderDelegateBridge::Shutdown() { gOtaSender.Shutdown(); }
void MTROTAProviderDelegateBridge::Shutdown() { gOtaSender->Shutdown(); }

void MTROTAProviderDelegateBridge::ControllerShuttingDown(MTRDeviceController * controller)
{
gOtaSender.ControllerShuttingDown(controller);
gOtaSender->ControllerShuttingDown(controller);
}

namespace {
Expand Down Expand Up @@ -675,7 +676,7 @@ bool GetPeerNodeInfo(CommandHandler * commandHandler, const ConcreteCommandPath
}

// If there is an update available, try to prepare for a transfer.
CHIP_ERROR err = gOtaSender.PrepareForTransfer(fabricIndex, nodeId);
CHIP_ERROR err = gOtaSender->PrepareForTransfer(fabricIndex, nodeId);
if (CHIP_NO_ERROR != err) {

// Handle busy error separately as we have a query image response status that maps to busy
Expand All @@ -698,7 +699,7 @@ bool GetPeerNodeInfo(CommandHandler * commandHandler, const ConcreteCommandPath
handle.Release();
// We need to reset state here to clean up any initialization we might have done including starting the BDX
// timeout timer while preparing for transfer if any failure occurs afterwards.
gOtaSender.ResetState();
gOtaSender->ResetState();
return;
}

Expand All @@ -709,7 +710,7 @@ bool GetPeerNodeInfo(CommandHandler * commandHandler, const ConcreteCommandPath
LogErrorOnFailure(err);
handler->AddStatus(cachedCommandPath, StatusIB(err).mStatus);
handle.Release();
gOtaSender.ResetState();
gOtaSender->ResetState();
return;
}
delegateResponse.imageURI.SetValue(uri);
Expand Down
Loading

0 comments on commit 3232412

Please sign in to comment.