Skip to content

Commit

Permalink
[darwin-framework-tool] heap-use-after-free when calling chip::app::D…
Browse files Browse the repository at this point in the history
…nssdServer::StartServer at startup (#26000)
  • Loading branch information
vivien-apple authored and pull[bot] committed Jun 30, 2023
1 parent aaf7699 commit 1069988
Show file tree
Hide file tree
Showing 9 changed files with 73 additions and 6 deletions.
24 changes: 24 additions & 0 deletions src/app/server/Dnssd.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -343,6 +343,30 @@ void DnssdServer::StartServer()
return StartServer(mode);
}

void DnssdServer::StopServer()
{
DeviceLayer::PlatformMgr().RemoveEventHandler(OnPlatformEventWrapper, 0);

#if CHIP_DEVICE_CONFIG_ENABLE_EXTENDED_DISCOVERY
if (mExtendedDiscoveryExpiration != kTimeoutCleared)
{
DeviceLayer::SystemLayer().CancelTimer(HandleExtendedDiscoveryExpiration, nullptr);
mExtendedDiscoveryExpiration = kTimeoutCleared;
}
#endif // CHIP_DEVICE_CONFIG_ENABLE_EXTENDED_DISCOVERY

if (Dnssd::ServiceAdvertiser::Instance().IsInitialized())
{
auto err = Dnssd::ServiceAdvertiser::Instance().RemoveServices();
if (err != CHIP_NO_ERROR)
{
ChipLogError(Discovery, "Failed to remove advertised services: %" CHIP_ERROR_FORMAT, err.Format());
}

Dnssd::ServiceAdvertiser::Instance().Shutdown();
}
}

void DnssdServer::StartServer(Dnssd::CommissioningMode mode)
{
ChipLogProgress(Discovery, "Updating services using commissioning mode %d", static_cast<int>(mode));
Expand Down
3 changes: 3 additions & 0 deletions src/app/server/Dnssd.h
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,9 @@ class DLL_EXPORT DnssdServer
/// (Re-)starts the Dnssd server, using the provided commissioning mode.
void StartServer(Dnssd::CommissioningMode mode);

//// Stop the Dnssd server.
void StopServer();

CHIP_ERROR GenerateRotatingDeviceId(char rotatingDeviceIdHexBuffer[], size_t rotatingDeviceIdHexBufferSize);

/// Generates the (random) instance name that a CHIP device is to use for pre-commissioning DNS-SD
Expand Down
15 changes: 15 additions & 0 deletions src/controller/CHIPDeviceControllerFactory.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,21 @@ CHIP_ERROR DeviceControllerFactory::ServiceEvents()
return CHIP_NO_ERROR;
}

void DeviceControllerFactory::RetainSystemState()
{
(void) mSystemState->Retain();
}

void DeviceControllerFactory::ReleaseSystemState()
{
mSystemState->Release();

if (!mSystemState->IsInitialized() && mEnableServerInteractions)
{
app::DnssdServer::Instance().StopServer();
}
}

DeviceControllerFactory::~DeviceControllerFactory()
{
Shutdown();
Expand Down
4 changes: 2 additions & 2 deletions src/controller/CHIPDeviceControllerFactory.h
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,7 @@ class DeviceControllerFactory
// created to permit retention of the underlying system state.
//
// NB: The system state will still be freed in Shutdown() regardless of this call.
void RetainSystemState() { (void) mSystemState->Retain(); }
void RetainSystemState();

//
// To initiate shutdown of the stack upon termination of all resident controllers in the
Expand All @@ -183,7 +183,7 @@ class DeviceControllerFactory
//
// This should only be invoked if a matching call to RetainSystemState() was called prior.
//
void ReleaseSystemState() { mSystemState->Release(); }
void ReleaseSystemState();

//
// Retrieve a read-only pointer to the system state object that contains pointers to key stack
Expand Down
7 changes: 7 additions & 0 deletions src/lib/dnssd/Advertiser.h
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,13 @@ class ServiceAdvertiser
*/
virtual CHIP_ERROR Init(chip::Inet::EndPointManager<chip::Inet::UDPEndPoint> * udpEndPointManager) = 0;

/**
* Returns whether the advertiser has completed the initialization.
*
* Returns true if the advertiser is ready to advertise services.
*/
virtual bool IsInitialized() = 0;

/**
* Shuts down the advertiser.
*/
Expand Down
17 changes: 16 additions & 1 deletion src/lib/dnssd/Advertiser_ImplMinimalMdns.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -181,11 +181,12 @@ class AdvertiserMinMdns : public ServiceAdvertiser,

// Service advertiser
CHIP_ERROR Init(chip::Inet::EndPointManager<chip::Inet::UDPEndPoint> * udpEndPointManager) override;
bool IsInitialized() override { return mIsInitialized; }
void Shutdown() override;
CHIP_ERROR RemoveServices() override;
CHIP_ERROR Advertise(const OperationalAdvertisingParameters & params) override;
CHIP_ERROR Advertise(const CommissionAdvertisingParameters & params) override;
CHIP_ERROR FinalizeServiceUpdate() override { return CHIP_NO_ERROR; }
CHIP_ERROR FinalizeServiceUpdate() override;
CHIP_ERROR GetCommissionableInstanceName(char * instanceName, size_t maxLength) const override;
CHIP_ERROR UpdateCommissionableInstanceName() override;

Expand Down Expand Up @@ -363,6 +364,8 @@ CHIP_ERROR AdvertiserMinMdns::Init(chip::Inet::EndPointManager<chip::Inet::UDPEn

void AdvertiserMinMdns::Shutdown()
{
VerifyOrReturn(mIsInitialized);

AdvertiseRecords(BroadcastAdvertiseType::kRemovingAll);

GlobalMinimalMdnsServer::Server().Shutdown();
Expand All @@ -371,6 +374,8 @@ void AdvertiserMinMdns::Shutdown()

CHIP_ERROR AdvertiserMinMdns::RemoveServices()
{
VerifyOrReturnError(mIsInitialized, CHIP_ERROR_INCORRECT_STATE);

// Send a "goodbye" packet for each RR being removed, as defined in RFC 6762.
// This allows mDNS clients to remove stale cached records which may not be re-added with
// subsequent Advertise() calls. In the case the same records are re-added, this extra
Expand Down Expand Up @@ -446,6 +451,8 @@ OperationalQueryAllocator::Allocator * AdvertiserMinMdns::FindEmptyOperationalAl

CHIP_ERROR AdvertiserMinMdns::Advertise(const OperationalAdvertisingParameters & params)
{
VerifyOrReturnError(mIsInitialized, CHIP_ERROR_INCORRECT_STATE);

char nameBuffer[Operational::kInstanceNameMaxLength + 1] = "";

// need to set server name
Expand Down Expand Up @@ -547,6 +554,12 @@ CHIP_ERROR AdvertiserMinMdns::Advertise(const OperationalAdvertisingParameters &
return CHIP_NO_ERROR;
}

CHIP_ERROR AdvertiserMinMdns::FinalizeServiceUpdate()
{
VerifyOrReturnError(mIsInitialized, CHIP_ERROR_INCORRECT_STATE);
return CHIP_NO_ERROR;
}

CHIP_ERROR AdvertiserMinMdns::GetCommissionableInstanceName(char * instanceName, size_t maxLength) const
{
if (maxLength < (Commission::kInstanceNameMaxLength + 1))
Expand All @@ -568,6 +581,8 @@ CHIP_ERROR AdvertiserMinMdns::UpdateCommissionableInstanceName()

CHIP_ERROR AdvertiserMinMdns::Advertise(const CommissionAdvertisingParameters & params)
{
VerifyOrReturnError(mIsInitialized, CHIP_ERROR_INCORRECT_STATE);

if (params.GetCommissionAdvertiseMode() == CommssionAdvertiseMode::kCommissionableNode)
{
mQueryResponderAllocatorCommissionable.Clear();
Expand Down
2 changes: 2 additions & 0 deletions src/lib/dnssd/Advertiser_ImplNone.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,8 @@ class NoneAdvertiser : public ServiceAdvertiser
return CHIP_ERROR_NOT_IMPLEMENTED;
}

bool IsInitialized() override { return false; }

void Shutdown() override {}

CHIP_ERROR RemoveServices() override
Expand Down
5 changes: 3 additions & 2 deletions src/lib/dnssd/Discovery_ImplPlatform.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -486,8 +486,6 @@ CHIP_ERROR DiscoveryImplPlatform::PublishService(const char * serviceType, TextE
Inet::InterfaceId interfaceId, const chip::ByteSpan & mac,
DnssdServiceProtocol protocol, PeerId peerId)
{
VerifyOrReturnError(mState == State::kInitialized, CHIP_ERROR_INCORRECT_STATE);

DnssdService service;
ReturnErrorOnFailure(MakeHostName(service.mHostName, sizeof(service.mHostName), mac));
ReturnErrorOnFailure(protocol == DnssdServiceProtocol::kDnssdProtocolTcp
Expand Down Expand Up @@ -525,6 +523,7 @@ CHIP_ERROR DiscoveryImplPlatform::PublishService(const char * serviceType, TextE
}

#define PREPARE_RECORDS(Type) \
VerifyOrReturnError(IsInitialized(), CHIP_ERROR_INCORRECT_STATE); \
TextEntry textEntries[Type##AdvertisingParameters::kTxtMaxNumber]; \
size_t textEntrySize = 0; \
const char * subTypes[Type::kSubTypeMaxNumber]; \
Expand Down Expand Up @@ -593,13 +592,15 @@ CHIP_ERROR DiscoveryImplPlatform::Advertise(const CommissionAdvertisingParameter

CHIP_ERROR DiscoveryImplPlatform::RemoveServices()
{
VerifyOrReturnError(IsInitialized(), CHIP_ERROR_INCORRECT_STATE);
ReturnErrorOnFailure(ChipDnssdRemoveServices());

return CHIP_NO_ERROR;
}

CHIP_ERROR DiscoveryImplPlatform::FinalizeServiceUpdate()
{
VerifyOrReturnError(IsInitialized(), CHIP_ERROR_INCORRECT_STATE);
return ChipDnssdFinalizeServiceUpdate();
}

Expand Down
2 changes: 1 addition & 1 deletion src/lib/dnssd/Discovery_ImplPlatform.h
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ class DiscoveryImplPlatform : public ServiceAdvertiser, public Resolver
public:
// Members that implement both ServiceAdveriser and Resolver interfaces.
CHIP_ERROR Init(Inet::EndPointManager<Inet::UDPEndPoint> *) override { return InitImpl(); }
bool IsInitialized() override;
void Shutdown() override;

// Members that implement ServiceAdvertiser interface.
Expand All @@ -49,7 +50,6 @@ class DiscoveryImplPlatform : public ServiceAdvertiser, public Resolver
CHIP_ERROR UpdateCommissionableInstanceName() override;

// Members that implement Resolver interface.
bool IsInitialized() override;
void SetOperationalDelegate(OperationalResolveDelegate * delegate) override { mResolverProxy.SetOperationalDelegate(delegate); }
void SetCommissioningDelegate(CommissioningResolveDelegate * delegate) override
{
Expand Down

0 comments on commit 1069988

Please sign in to comment.