Skip to content

Commit

Permalink
[mdns-mdnssd] handle deallocated ServiceRefs as processing run loop
Browse files Browse the repository at this point in the history
  • Loading branch information
@abtink
abtink committed Sep 11, 2023
1 parent 4e75260 commit 0ecc768
Show file tree
Hide file tree
Showing 2 changed files with 64 additions and 28 deletions.
63 changes: 48 additions & 15 deletions src/mdns/mdns_mdnssd.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -249,6 +249,7 @@ void PublisherMDnsSd::Stop(void)

if (mHostsRef != nullptr)
{
HandleServiceRefDeallocating(mHostsRef);
DNSServiceRefDeallocate(mHostsRef);
otbrLogDebug("Deallocated DNSServiceRef for hosts: %p", mHostsRef);
mHostsRef = nullptr;
Expand Down Expand Up @@ -305,7 +306,7 @@ void PublisherMDnsSd::Update(MainloopContext &aMainloop)

void PublisherMDnsSd::Process(const MainloopContext &aMainloop)
{
std::vector<DNSServiceRef> readyServices;
mServiceRefsToProcess.clear();

for (auto &kv : mServiceRegistrations)
{
Expand All @@ -314,7 +315,7 @@ void PublisherMDnsSd::Process(const MainloopContext &aMainloop)

if (FD_ISSET(fd, &aMainloop.mReadFdSet))
{
readyServices.push_back(serviceReg.GetServiceRef());
mServiceRefsToProcess.push_back(serviceReg.GetServiceRef());
}
}

Expand All @@ -324,23 +325,41 @@ void PublisherMDnsSd::Process(const MainloopContext &aMainloop)

if (FD_ISSET(fd, &aMainloop.mReadFdSet))
{
readyServices.push_back(mHostsRef);
mServiceRefsToProcess.push_back(mHostsRef);
}
}

for (const auto &service : mSubscribedServices)
{
service->ProcessAll(aMainloop, readyServices);
service->ProcessAll(aMainloop, mServiceRefsToProcess);
}

for (const auto &host : mSubscribedHosts)
{
host->Process(aMainloop, readyServices);
host->Process(aMainloop, mServiceRefsToProcess);
}

for (DNSServiceRef serviceRef : readyServices)
for (DNSServiceRef serviceRef : mServiceRefsToProcess)
{
DNSServiceErrorType error = DNSServiceProcessResult(serviceRef);
DNSServiceErrorType error;

// As we go through the list of `mServiceRefsToProcess` the call
// to `DNSServiceProcessResult()` can itself invoke callbacks
// into `PublisherMDnsSd` and OT, which in turn, may change the
// state of `Publisher` and potentially trigger a previously
// valid `ServiceRef` in the list to be deallocated. We use
// `HandleServiceRefDeallocating()` which is called whenever a
// `ServcieRef` is being deallocated and from this we update
// the entry in `mServiceRefsToProcess` list to `nullptr` so to
// avoid calling `DNSServiceProcessResult()` on an already
// freed `ServcieRef`.

if (serviceRef == nullptr)
{
continue;
}

error = DNSServiceProcessResult(serviceRef);

if (error != kDNSServiceErr_NoError)
{
Expand All @@ -360,10 +379,22 @@ void PublisherMDnsSd::Process(const MainloopContext &aMainloop)
return;
}

void PublisherMDnsSd::HandleServiceRefDeallocating(const DNSServiceRef &aServiceRef)
{
for (DNSServiceRef &entry : mServiceRefsToProcess)
{
if (entry == aServiceRef)
{
entry = nullptr;
}
}
}

PublisherMDnsSd::DnssdServiceRegistration::~DnssdServiceRegistration(void)
{
if (mServiceRef != nullptr)
{
GetPublisher().HandleServiceRefDeallocating(mServiceRef);
DNSServiceRefDeallocate(mServiceRef);
}
}
Expand Down Expand Up @@ -538,6 +569,7 @@ otbrError PublisherMDnsSd::PublishServiceImpl(const std::string &aHostName,

if (serviceRef != nullptr)
{
HandleServiceRefDeallocating(serviceRef);
DNSServiceRefDeallocate(serviceRef);
}
std::move(aCallback)(ret);
Expand Down Expand Up @@ -794,6 +826,7 @@ void PublisherMDnsSd::ServiceRef::DeallocateServiceRef(void)
{
if (mServiceRef != nullptr)
{
mPublisher.HandleServiceRefDeallocating(mServiceRef);
DNSServiceRefDeallocate(mServiceRef);
mServiceRef = nullptr;
}
Expand Down Expand Up @@ -875,13 +908,13 @@ void PublisherMDnsSd::ServiceSubscription::HandleBrowseResult(DNSServiceRef
}
else
{
mMDnsSd->OnServiceRemoved(aInterfaceIndex, mType, aInstanceName);
mPublisher.OnServiceRemoved(aInterfaceIndex, mType, aInstanceName);
}

exit:
if (aErrorCode != kDNSServiceErr_NoError)
{
mMDnsSd->OnServiceResolveFailed(mType, mInstanceName, aErrorCode);
mPublisher.OnServiceResolveFailed(mType, mInstanceName, aErrorCode);
Release();
}
}
Expand Down Expand Up @@ -934,7 +967,7 @@ void PublisherMDnsSd::ServiceInstanceResolution::Resolve(void)
{
assert(mServiceRef == nullptr);

mSubscription->mMDnsSd->mServiceInstanceResolutionBeginTime[std::make_pair(mInstanceName, mTypeEndWithDot)] =
mSubscription->mPublisher.mServiceInstanceResolutionBeginTime[std::make_pair(mInstanceName, mTypeEndWithDot)] =
Clock::now();

otbrLogInfo("DNSServiceResolve %s %s inf %u", mInstanceName.c_str(), mTypeEndWithDot.c_str(), mNetifIndex);
Expand Down Expand Up @@ -999,7 +1032,7 @@ void PublisherMDnsSd::ServiceInstanceResolution::HandleResolveResult(DNSServiceR

if (aErrorCode != kDNSServiceErr_NoError || error != OTBR_ERROR_NONE)
{
mSubscription->mMDnsSd->OnServiceResolveFailed(mSubscription->mType, mInstanceName, aErrorCode);
mSubscription->mPublisher.OnServiceResolveFailed(mSubscription->mType, mInstanceName, aErrorCode);
FinishResolution();
}
}
Expand Down Expand Up @@ -1084,7 +1117,7 @@ void PublisherMDnsSd::ServiceInstanceResolution::FinishResolution(void)
subscription->RemoveInstanceResolution(*this);

// NOTE: The `ServiceSubscription` object may be freed in `OnServiceResolved`.
subscription->mMDnsSd->OnServiceResolved(serviceName, instanceInfo);
subscription->mPublisher.OnServiceResolved(serviceName, instanceInfo);
}

void PublisherMDnsSd::HostSubscription::Resolve(void)
Expand All @@ -1093,7 +1126,7 @@ void PublisherMDnsSd::HostSubscription::Resolve(void)

assert(mServiceRef == nullptr);

mMDnsSd->mHostResolutionBeginTime[mHostName] = Clock::now();
mPublisher.mHostResolutionBeginTime[mHostName] = Clock::now();

otbrLogInfo("DNSServiceGetAddrInfo %s inf %d", fullHostName.c_str(), kDNSServiceInterfaceIndexAny);

Expand Down Expand Up @@ -1146,12 +1179,12 @@ void PublisherMDnsSd::HostSubscription::HandleResolveResult(DNSServiceRef
otbrLogInfo("DNSServiceGetAddrInfo reply: address=%s, ttl=%" PRIu32, address.ToString().c_str(), aTtl);

// NOTE: This `HostSubscription` object may be freed in `OnHostResolved`.
mMDnsSd->OnHostResolved(mHostName, mHostInfo);
mPublisher.OnHostResolved(mHostName, mHostInfo);

exit:
if (aErrorCode != kDNSServiceErr_NoError)
{
mMDnsSd->OnHostResolveFailed(aHostName, aErrorCode);
mPublisher.OnHostResolveFailed(aHostName, aErrorCode);
}
}

Expand Down
29 changes: 16 additions & 13 deletions src/mdns/mdns_mdnssd.hpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,7 @@ class PublisherMDnsSd : public MainloopProcessor, public Publisher

~DnssdServiceRegistration(void) override;
const DNSServiceRef &GetServiceRef() const { return mServiceRef; }
PublisherMDnsSd &GetPublisher(void) { return *static_cast<PublisherMDnsSd *>(mPublisher); }

private:
DNSServiceRef mServiceRef;
Expand Down Expand Up @@ -164,10 +165,12 @@ class PublisherMDnsSd : public MainloopProcessor, public Publisher

struct ServiceRef : private ::NonCopyable
{
DNSServiceRef mServiceRef;
DNSServiceRef mServiceRef;
PublisherMDnsSd &mPublisher;

explicit ServiceRef(void)
explicit ServiceRef(PublisherMDnsSd &aPublisher)
: mServiceRef(nullptr)
, mPublisher(aPublisher)
{
}

Expand All @@ -188,7 +191,7 @@ class PublisherMDnsSd : public MainloopProcessor, public Publisher
std::string aType,
std::string aDomain,
uint32_t aNetifIndex)
: ServiceRef()
: ServiceRef(aSubscription.mPublisher)
, mSubscription(&aSubscription)
, mInstanceName(std::move(aInstanceName))
, mTypeEndWithDot(std::move(aType))
Expand Down Expand Up @@ -246,9 +249,8 @@ class PublisherMDnsSd : public MainloopProcessor, public Publisher

struct ServiceSubscription : public ServiceRef
{
explicit ServiceSubscription(PublisherMDnsSd &aMDnsSd, std::string aType, std::string aInstanceName)
: ServiceRef()
, mMDnsSd(&aMDnsSd)
explicit ServiceSubscription(PublisherMDnsSd &aPublisher, std::string aType, std::string aInstanceName)
: ServiceRef(aPublisher)
, mType(std::move(aType))
, mInstanceName(std::move(aInstanceName))
{
Expand Down Expand Up @@ -279,18 +281,16 @@ class PublisherMDnsSd : public MainloopProcessor, public Publisher
const char *aType,
const char *aDomain);

PublisherMDnsSd *mMDnsSd;
std::string mType;
std::string mInstanceName;
std::string mType;
std::string mInstanceName;

std::vector<std::unique_ptr<ServiceInstanceResolution>> mResolvingInstances;
};

struct HostSubscription : public ServiceRef
{
explicit HostSubscription(PublisherMDnsSd &aMDnsSd, std::string aHostName)
: ServiceRef()
, mMDnsSd(&aMDnsSd)
explicit HostSubscription(PublisherMDnsSd &aPublisher, std::string aHostName)
: ServiceRef(aPublisher)
, mHostName(std::move(aHostName))
{
}
Expand All @@ -312,7 +312,6 @@ class PublisherMDnsSd : public MainloopProcessor, public Publisher
const struct sockaddr *aAddress,
uint32_t aTtl);

PublisherMDnsSd *mMDnsSd;
std::string mHostName;
DiscoveredHostInfo mHostInfo;
};
Expand Down Expand Up @@ -345,6 +344,8 @@ class PublisherMDnsSd : public MainloopProcessor, public Publisher

static std::string MakeRegType(const std::string &aType, SubTypeList aSubTypeList);

void HandleServiceRefDeallocating(const DNSServiceRef &aServiceRef);

ServiceRegistration *FindServiceRegistration(const DNSServiceRef &aServiceRef);
HostRegistration *FindHostRegistration(const DNSServiceRef &aServiceRef, const DNSRecordRef &aRecordRef);

Expand All @@ -354,6 +355,8 @@ class PublisherMDnsSd : public MainloopProcessor, public Publisher

ServiceSubscriptionList mSubscribedServices;
HostSubscriptionList mSubscribedHosts;

std::vector<DNSServiceRef> mServiceRefsToProcess;
};

/**
Expand Down

0 comments on commit 0ecc768

Please sign in to comment.