Skip to content

Commit

Permalink
Add a ResolveProxy class to keep track of which controller is issuing… (
Browse files Browse the repository at this point in the history
#12481)

* Add a ResolveProxy class to keep track of which controller is issuing a dns request

* Use ReferenceCount<ResolverDelegateProxy> to make sure the stack is not trying to access a dangling pointer once a response is received
  • Loading branch information
vivien-apple authored and pull[bot] committed Jan 26, 2022
1 parent e20ecc4 commit 870befa
Show file tree
Hide file tree
Showing 18 changed files with 368 additions and 185 deletions.
6 changes: 3 additions & 3 deletions src/app/CASESessionManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ CHIP_ERROR CASESessionManager::FindOrEstablishSession(NodeId nodeId, Callback::C
session->OnNodeIdResolved(resolutionData);
}

CHIP_ERROR err = session->Connect(onConnection, onFailure);
CHIP_ERROR err = session->Connect(onConnection, onFailure, mConfig.dnsResolver);
if (err != CHIP_NO_ERROR)
{
ReleaseSession(session);
Expand All @@ -69,8 +69,8 @@ void CASESessionManager::ReleaseSession(NodeId nodeId)

CHIP_ERROR CASESessionManager::ResolveDeviceAddress(NodeId nodeId)
{
return Dnssd::Resolver::Instance().ResolveNodeId(GetFabricInfo()->GetPeerIdForNode(nodeId), Inet::IPAddressType::kAny,
Dnssd::Resolver::CacheBypass::On);
return mConfig.dnsResolver->ResolveNodeId(GetFabricInfo()->GetPeerIdForNode(nodeId), Inet::IPAddressType::kAny,
Dnssd::Resolver::CacheBypass::On);
}

void CASESessionManager::OnNodeIdResolved(const Dnssd::ResolvedNodeData & nodeData)
Expand Down
3 changes: 2 additions & 1 deletion src/app/CASESessionManager.h
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@
#include <lib/support/Pool.h>
#include <transport/SessionDelegate.h>

#include <lib/dnssd/Resolver.h>
#include <lib/dnssd/ResolverProxy.h>

namespace chip {

Expand All @@ -36,6 +36,7 @@ struct CASESessionManagerConfig
DeviceProxyInitParams sessionInitParams;
Dnssd::DnssdCache<CHIP_CONFIG_MDNS_CACHE_SIZE> * dnsCache = nullptr;
OperationalDeviceProxyPoolDelegate * devicePool = nullptr;
Dnssd::ResolverProxy * dnsResolver = nullptr;
};

/**
Expand Down
6 changes: 4 additions & 2 deletions src/app/OperationalDeviceProxy.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,8 @@ using namespace chip::Callback;
namespace chip {

CHIP_ERROR OperationalDeviceProxy::Connect(Callback::Callback<OnDeviceConnected> * onConnection,
Callback::Callback<OnDeviceConnectionFailure> * onFailure)
Callback::Callback<OnDeviceConnectionFailure> * onFailure,
Dnssd::ResolverProxy * resolver)
{
CHIP_ERROR err = CHIP_NO_ERROR;

Expand All @@ -54,7 +55,8 @@ CHIP_ERROR OperationalDeviceProxy::Connect(Callback::Callback<OnDeviceConnected>
break;

case State::NeedsAddress:
err = Dnssd::Resolver::Instance().ResolveNodeId(mPeerId, chip::Inet::IPAddressType::kAny);
VerifyOrReturnError(resolver != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
err = resolver->ResolveNodeId(mPeerId, chip::Inet::IPAddressType::kAny);
EnqueueConnectionCallbacks(onConnection, onFailure);
break;

Expand Down
6 changes: 4 additions & 2 deletions src/app/OperationalDeviceProxy.h
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@
#include <transport/raw/MessageHeader.h>
#include <transport/raw/UDP.h>

#include <lib/dnssd/Resolver.h>
#include <lib/dnssd/ResolverProxy.h>

namespace chip {

Expand Down Expand Up @@ -109,9 +109,11 @@ class DLL_EXPORT OperationalDeviceProxy : public DeviceProxy, SessionReleaseDele
* session setup fails, `onFailure` will be called.
*
* If the session already exists, `onConnection` will be called immediately.
* If the resolver is null and the device state is State::NeedsAddress, CHIP_ERROR_INVALID_ARGUMENT will be
* returned.
*/
CHIP_ERROR Connect(Callback::Callback<OnDeviceConnected> * onConnection,
Callback::Callback<OnDeviceConnectionFailure> * onFailure);
Callback::Callback<OnDeviceConnectionFailure> * onFailure, Dnssd::ResolverProxy * resolver);

bool IsConnected() const { return mState == State::SecureConnected; }

Expand Down
1 change: 1 addition & 0 deletions src/app/clusters/ota-requestor/OTARequestor.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -235,6 +235,7 @@ CHIP_ERROR OTARequestor::SetupCASESessionManager(FabricIndex fabricIndex)
.sessionInitParams = initParams,
.dnsCache = nullptr,
.devicePool = mServer->GetDevicePool(),
.dnsResolver = nullptr,
};

mCASESessionManager = Platform::New<CASESessionManager>(sessionManagerConfig);
Expand Down
9 changes: 0 additions & 9 deletions src/controller/AbstractDnssdDiscoveryController.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -19,10 +19,6 @@
// module header, comes first
#include <controller/AbstractDnssdDiscoveryController.h>

#if CONFIG_DEVICE_LAYER
#include <platform/CHIPDeviceLayer.h>
#endif

#include <lib/core/CHIPEncoding.h>
#include <lib/support/logging/CHIPLogging.h>

Expand Down Expand Up @@ -66,11 +62,6 @@ void AbstractDnssdDiscoveryController::OnNodeDiscoveryComplete(const chip::Dnssd

CHIP_ERROR AbstractDnssdDiscoveryController::SetUpNodeDiscovery()
{
#if CONFIG_DEVICE_LAYER
ReturnErrorOnFailure(mResolver->Init(&DeviceLayer::InetLayer()));
#endif
mResolver->SetResolverDelegate(this);

auto discoveredNodes = GetDiscoveredNodes();
for (auto & discoveredNode : discoveredNodes)
{
Expand Down
12 changes: 4 additions & 8 deletions src/controller/AbstractDnssdDiscoveryController.h
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@
#pragma once

#include <controller/DeviceDiscoveryDelegate.h>
#include <lib/dnssd/Resolver.h>
#include <lib/dnssd/ResolverProxy.h>
#include <lib/support/Span.h>
#include <platform/CHIPDeviceConfig.h>

Expand All @@ -39,11 +39,7 @@ namespace Controller {
class DLL_EXPORT AbstractDnssdDiscoveryController : public Dnssd::ResolverDelegate
{
public:
AbstractDnssdDiscoveryController(chip::Dnssd::Resolver * resolver = &chip::Dnssd::Resolver::Instance())
{
mResolver = resolver;
}

AbstractDnssdDiscoveryController() {}
virtual ~AbstractDnssdDiscoveryController() {}

void OnNodeDiscoveryComplete(const chip::Dnssd::DiscoveredNodeData & nodeData) override;
Expand All @@ -52,9 +48,9 @@ class DLL_EXPORT AbstractDnssdDiscoveryController : public Dnssd::ResolverDelega
using DiscoveredNodeList = FixedSpan<Dnssd::DiscoveredNodeData, CHIP_DEVICE_CONFIG_MAX_DISCOVERED_NODES>;
CHIP_ERROR SetUpNodeDiscovery();
const Dnssd::DiscoveredNodeData * GetDiscoveredNode(int idx);
virtual DiscoveredNodeList GetDiscoveredNodes() = 0;
chip::Dnssd::Resolver * mResolver;
virtual DiscoveredNodeList GetDiscoveredNodes() = 0;
DeviceDiscoveryDelegate * mDeviceDiscoveryDelegate = nullptr;
Dnssd::ResolverProxy mDNSResolver;
};

} // namespace Controller
Expand Down
21 changes: 20 additions & 1 deletion src/controller/CHIPCommissionableNodeController.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@
// module header, comes first
#include <controller/CHIPCommissionableNodeController.h>

#if CONFIG_DEVICE_LAYER
#include <platform/CHIPDeviceLayer.h>
#endif

#include <lib/support/CodeUtils.h>

namespace chip {
Expand All @@ -27,7 +31,22 @@ namespace Controller {
CHIP_ERROR CommissionableNodeController::DiscoverCommissioners(Dnssd::DiscoveryFilter discoveryFilter)
{
ReturnErrorOnFailure(SetUpNodeDiscovery());
return mResolver->FindCommissioners(discoveryFilter);

if (mResolver == nullptr)
{
#if CONFIG_DEVICE_LAYER
ReturnErrorOnFailure(mDNSResolver.Init(&DeviceLayer::InetLayer()));
#endif
mDNSResolver.SetResolverDelegate(this);
return mDNSResolver.FindCommissioners(discoveryFilter);
}
else
{
#if CONFIG_DEVICE_LAYER
ReturnErrorOnFailure(mResolver->Init(&DeviceLayer::InetLayer()));
#endif
return mResolver->FindCommissioners(discoveryFilter);
}
}

const Dnssd::DiscoveredNodeData * CommissionableNodeController::GetDiscoveredCommissioner(int idx)
Expand Down
5 changes: 2 additions & 3 deletions src/controller/CHIPCommissionableNodeController.h
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
#pragma once

#include <controller/AbstractDnssdDiscoveryController.h>
#include <lib/dnssd/Resolver.h>
#include <lib/support/logging/CHIPLogging.h>
#include <platform/CHIPDeviceConfig.h>

Expand All @@ -37,8 +36,7 @@ namespace Controller {
class DLL_EXPORT CommissionableNodeController : public AbstractDnssdDiscoveryController
{
public:
CommissionableNodeController(chip::Dnssd::Resolver * resolver = &chip::Dnssd::Resolver::Instance()) :
AbstractDnssdDiscoveryController(resolver){};
CommissionableNodeController(chip::Dnssd::Resolver * resolver = nullptr) : mResolver(resolver) {}
virtual ~CommissionableNodeController() {}

CHIP_ERROR DiscoverCommissioners(Dnssd::DiscoveryFilter discoveryFilter = Dnssd::DiscoveryFilter());
Expand Down Expand Up @@ -66,6 +64,7 @@ class DLL_EXPORT CommissionableNodeController : public AbstractDnssdDiscoveryCon
DiscoveredNodeList GetDiscoveredNodes() override { return DiscoveredNodeList(mDiscoveredCommissioners); }

private:
Dnssd::Resolver * mResolver = nullptr;
Dnssd::DiscoveredNodeData mDiscoveredCommissioners[CHIP_DEVICE_CONFIG_MAX_DISCOVERED_NODES];
};

Expand Down
24 changes: 11 additions & 13 deletions src/controller/CHIPDeviceController.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@

#include <app/InteractionModelEngine.h>
#include <app/OperationalDeviceProxy.h>
#include <app/util/DataModelHandler.h>
#include <app/util/error-mapping.h>
#include <credentials/CHIPCert.h>
#include <credentials/DeviceAttestationCredsProvider.h>
Expand Down Expand Up @@ -132,14 +131,12 @@ CHIP_ERROR DeviceController::Init(ControllerInitParams params)
VerifyOrReturnError(params.systemState->TransportMgr() != nullptr, CHIP_ERROR_INVALID_ARGUMENT);

#if CHIP_DEVICE_CONFIG_ENABLE_DNSSD
Dnssd::Resolver::Instance().Init(params.systemState->InetLayer());
Dnssd::Resolver::Instance().SetResolverDelegate(this);
ReturnErrorOnFailure(mDNSResolver.Init(params.systemState->InetLayer()));
mDNSResolver.SetResolverDelegate(this);
RegisterDeviceAddressUpdateDelegate(params.deviceAddressUpdateDelegate);
RegisterDeviceDiscoveryDelegate(params.deviceDiscoveryDelegate);
#endif // CHIP_DEVICE_CONFIG_ENABLE_DNSSD

InitDataModelHandler(params.systemState->ExchangeMgr());

VerifyOrReturnError(params.operationalCredentialsDelegate != nullptr, CHIP_ERROR_INVALID_ARGUMENT);
mOperationalCredentialsDelegate = params.operationalCredentialsDelegate;

Expand All @@ -160,6 +157,11 @@ CHIP_ERROR DeviceController::Init(ControllerInitParams params)
.sessionInitParams = deviceInitParams,
.dnsCache = &mDNSCache,
.devicePool = &mDevicePool,
#if CHIP_DEVICE_CONFIG_ENABLE_DNSSD
.dnsResolver = &mDNSResolver,
#else
.dnsResolver = nullptr,
#endif
};

mCASESessionManager = chip::Platform::New<CASESessionManager>(sessionManagerConfig);
Expand Down Expand Up @@ -227,18 +229,14 @@ CHIP_ERROR DeviceController::Shutdown()

mState = State::NotInitialized;

#if CHIP_DEVICE_CONFIG_ENABLE_DNSSD
Dnssd::Resolver::Instance().Shutdown();
#endif // CHIP_DEVICE_CONFIG_ENABLE_DNSSD

mStorageDelegate = nullptr;

mSystemState->Fabrics()->ReleaseFabricIndex(mFabricIndex);
mSystemState->Release();
mSystemState = nullptr;

#if CHIP_DEVICE_CONFIG_ENABLE_DNSSD
Dnssd::Resolver::Instance().SetResolverDelegate(nullptr);
mDNSResolver.Shutdown();
mDeviceAddressUpdateDelegate = nullptr;
mDeviceDiscoveryDelegate = nullptr;
#endif // CHIP_DEVICE_CONFIG_ENABLE_DNSSD
Expand Down Expand Up @@ -1571,7 +1569,7 @@ void DeviceCommissioner::OnSessionEstablishmentTimeoutCallback(System::Layer * a
CHIP_ERROR DeviceCommissioner::DiscoverCommissionableNodes(Dnssd::DiscoveryFilter filter)
{
ReturnErrorOnFailure(SetUpNodeDiscovery());
return chip::Dnssd::Resolver::Instance().FindCommissionableNodes(filter);
return mDNSResolver.FindCommissionableNodes(filter);
}

const Dnssd::DiscoveredNodeData * DeviceCommissioner::GetDiscoveredDevice(int idx)
Expand Down Expand Up @@ -1837,7 +1835,7 @@ void DeviceCommissioner::AdvanceCommissioningStage(CHIP_ERROR err)
#if CONFIG_DEVICE_LAYER
status = DeviceLayer::ConfigurationMgr().GetCountryCode(countryCodeStr, kMaxCountryCodeSize, actualCountryCodeSize);
#else
status = CHIP_ERROR_NOT_IMPLEMENTED;
status = CHIP_ERROR_NOT_IMPLEMENTED;
#endif
if (status != CHIP_NO_ERROR)
{
Expand Down Expand Up @@ -1894,7 +1892,7 @@ void DeviceCommissioner::AdvanceCommissioningStage(CHIP_ERROR err)
RendezvousCleanup(CHIP_NO_ERROR);
#if CHIP_DEVICE_CONFIG_ENABLE_DNSSD
ChipLogProgress(Controller, "Finding node on operational network");
Dnssd::Resolver::Instance().ResolveNodeId(peerId, Inet::IPAddressType::kAny);
mDNSResolver.ResolveNodeId(peerId, Inet::IPAddressType::kAny);
#endif
}
break;
Expand Down
1 change: 1 addition & 0 deletions src/controller/CHIPDeviceController.h
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@
#include <controller/DeviceAddressUpdateDelegate.h>
#include <controller/DeviceDiscoveryDelegate.h>
#include <lib/dnssd/Resolver.h>
#include <lib/dnssd/ResolverProxy.h>
#endif

namespace chip {
Expand Down
11 changes: 11 additions & 0 deletions src/controller/CHIPDeviceControllerFactory.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@

#include <controller/CHIPDeviceControllerFactory.h>

#include <app/util/DataModelHandler.h>
#include <lib/support/ErrorStr.h>

#if CONFIG_DEVICE_LAYER
Expand Down Expand Up @@ -140,9 +141,15 @@ CHIP_ERROR DeviceControllerFactory::InitSystemState(FactoryInitParams params)
ReturnErrorOnFailure(stateParams.exchangeMgr->Init(stateParams.sessionMgr));
ReturnErrorOnFailure(stateParams.messageCounterManager->Init(stateParams.exchangeMgr));

InitDataModelHandler(stateParams.exchangeMgr);

stateParams.imDelegate = params.imDelegate;
ReturnErrorOnFailure(chip::app::InteractionModelEngine::GetInstance()->Init(stateParams.exchangeMgr, stateParams.imDelegate));

#if CHIP_DEVICE_CONFIG_ENABLE_DNSSD
ReturnErrorOnFailure(Dnssd::Resolver::Instance().Init(stateParams.inetLayer));
#endif // CHIP_DEVICE_CONFIG_ENABLE_DNSSD

// store the system state
mSystemState = chip::Platform::New<DeviceControllerSystemState>(stateParams);
ChipLogDetail(Controller, "System State Initialized...");
Expand Down Expand Up @@ -220,6 +227,10 @@ CHIP_ERROR DeviceControllerSystemState::Shutdown()

ChipLogDetail(Controller, "Shutting down the System State, this will teardown the CHIP Stack");

#if CHIP_DEVICE_CONFIG_ENABLE_DNSSD
Dnssd::Resolver::Instance().Shutdown();
#endif // CHIP_DEVICE_CONFIG_ENABLE_DNSSD

// Shut down the interaction model
app::InteractionModelEngine::GetInstance()->Shutdown();

Expand Down
19 changes: 18 additions & 1 deletion src/controller/tests/TestCommissionableNodeController.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -180,9 +180,26 @@ const nlTest sTests[] =

} // namespace

int TestCommissionableNodeController_Setup(void * inContext)
{
if (CHIP_NO_ERROR != chip::Platform::MemoryInit())
{
return FAILURE;
}

return SUCCESS;
}

int TestCommissionableNodeController_Teardown(void * inContext)
{
chip::Platform::MemoryShutdown();
return SUCCESS;
}

int TestCommissionableNodeController()
{
nlTestSuite theSuite = { "CommissionableNodeController", &sTests[0], NULL, NULL };
nlTestSuite theSuite = { "CommissionableNodeController", &sTests[0], TestCommissionableNodeController_Setup,
TestCommissionableNodeController_Teardown };
nlTestRunner(&theSuite, nullptr);
return nlTestRunnerStats(&theSuite);
}
Expand Down
Loading

0 comments on commit 870befa

Please sign in to comment.