Skip to content

Commit

Permalink
[mdns-mdnssd] fix access of freed mHostsRef in Stop()
Browse files Browse the repository at this point in the history
This commit updates the `Stop()` method to behave differently based on a
`StopMode` input. If called from `DNSServiceProcessResult()` call where
we get a `kDNSServiceErr_ServiceNotRunning` error, we need to restart
the `Publisher` and should immediately de-allocate all `ServiceRefs`,
including `mHostsRef`. Otherwise, during a `kNormalStop`, we first
clear the `m{Host/Service}Registrations` lists before de-allocating the
`mHostsRef`. This ensures that `mHostsRef` and all its related child
`DNSRecordRef`s remain valid when `DnssdHostRegistration` destructor is
called, which accesses and uses them to update the published AAAA (IPv6
address) records.
  • Loading branch information
abtink committed Sep 13, 2023
1 parent 6da0357 commit 968502d
Show file tree
Hide file tree
Showing 2 changed files with 44 additions and 15 deletions.
47 changes: 33 additions & 14 deletions src/mdns/mdns_mdnssd.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ PublisherMDnsSd::PublisherMDnsSd(StateCallback aCallback)

PublisherMDnsSd::~PublisherMDnsSd(void)
{
Stop();
Stop(kNormalStop);
}

otbrError PublisherMDnsSd::Start(void)
Expand All @@ -237,26 +237,31 @@ bool PublisherMDnsSd::IsStarted(void) const
return mState == State::kReady;
}

void PublisherMDnsSd::Stop(void)
void PublisherMDnsSd::Stop(StopMode aStopMode)
{
ServiceRegistrationMap serviceRegistrations;
HostRegistrationMap hostRegistrations;

VerifyOrExit(mState == State::kReady);

std::swap(mServiceRegistrations, serviceRegistrations);
std::swap(mHostRegistrations, hostRegistrations);
// If we get a `kDNSServiceErr_ServiceNotRunning` and need to
// restart the `Publisher`, we should immediately de-allocate
// all `ServiceRef`. Otherwise, we first clear the `Registrations`
// list so that `DnssdHostRegisteration` destructor gets the chance
// to update registered records if needed.

if (mHostsRef != nullptr)
switch (aStopMode)
{
HandleServiceRefDeallocating(mHostsRef);
DNSServiceRefDeallocate(mHostsRef);
otbrLogDebug("Deallocated DNSServiceRef for hosts: %p", mHostsRef);
mHostsRef = nullptr;
case kNormalStop:
break;

case kStopOnServiceNotRunningError:
DeallocateHostsRef();
break;
}

mSubscribedServices.clear();
mServiceRegistrations.clear();
mHostRegistrations.clear();
DeallocateHostsRef();

mSubscribedServices.clear();
mSubscribedHosts.clear();

mState = State::kIdle;
Expand All @@ -265,6 +270,19 @@ void PublisherMDnsSd::Stop(void)
return;
}

void PublisherMDnsSd::DeallocateHostsRef(void)
{
VerifyOrExit(mHostsRef != nullptr);

HandleServiceRefDeallocating(mHostsRef);
DNSServiceRefDeallocate(mHostsRef);
otbrLogDebug("Deallocated DNSServiceRef for hosts: %p", mHostsRef);
mHostsRef = nullptr;

exit:
return;
}

void PublisherMDnsSd::Update(MainloopContext &aMainloop)
{
for (auto &kv : mServiceRegistrations)
Expand Down Expand Up @@ -370,7 +388,7 @@ void PublisherMDnsSd::Process(const MainloopContext &aMainloop)
if (error == kDNSServiceErr_ServiceNotRunning)
{
otbrLogWarning("Need to reconnect to mdnsd");
Stop();
Stop(kStopOnServiceNotRunningError);
Start();
ExitNow();
}
Expand Down Expand Up @@ -403,6 +421,7 @@ PublisherMDnsSd::DnssdHostRegistration::~DnssdHostRegistration(void)
{
int dnsError;

VerifyOrExit(GetPublisher().mHostsRef != nullptr);
VerifyOrExit(mServiceRef != nullptr);

for (const auto &recordRefAndAddress : GetRecordRefMap())
Expand Down
12 changes: 11 additions & 1 deletion src/mdns/mdns_mdnssd.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ class PublisherMDnsSd : public MainloopProcessor, public Publisher
void UnsubscribeHost(const std::string &aHostName) override;
otbrError Start(void) override;
bool IsStarted(void) const override;
void Stop(void) override;
void Stop(void) override { Stop(kNormalStop); }

// Implementation of MainloopProcessor.

Expand All @@ -103,6 +103,12 @@ class PublisherMDnsSd : public MainloopProcessor, public Publisher
private:
static constexpr uint32_t kDefaultTtl = 10;

enum StopMode : uint8_t
{
kNormalStop,
kStopOnServiceNotRunningError,
};

class DnssdServiceRegistration : public ServiceRegistration
{
public:
Expand Down Expand Up @@ -156,6 +162,8 @@ class PublisherMDnsSd : public MainloopProcessor, public Publisher
std::map<DNSRecordRef, Ip6Address> &GetRecordRefMap() { return mRecordRefMap; }

private:
PublisherMDnsSd &GetPublisher(void) { return *static_cast<PublisherMDnsSd *>(mPublisher); }

DNSServiceRef mServiceRef;

public:
Expand Down Expand Up @@ -344,6 +352,8 @@ class PublisherMDnsSd : public MainloopProcessor, public Publisher

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

void Stop(StopMode aStopMode);
void DeallocateHostsRef(void);
void HandleServiceRefDeallocating(const DNSServiceRef &aServiceRef);

ServiceRegistration *FindServiceRegistration(const DNSServiceRef &aServiceRef);
Expand Down

0 comments on commit 968502d

Please sign in to comment.