Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Filter received mdns records to chip only. #7527

Merged
merged 1 commit into from
Jun 11, 2021
Merged

Filter received mdns records to chip only. #7527

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
62 changes: 33 additions & 29 deletions src/lib/mdns/Resolver_ImplMinimalMdns.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,23 +145,6 @@ void PacketDataReporter::OnOperationalSrvRecord(SerializedQNameIterator name, co
return;
}

// Before attempting to parse hex values for node/fabrid, validate
// that he response is indeed from a chip tcp service.
{
SerializedQNameIterator suffix = name;

constexpr const char * kExpectedSuffix[] = { "_chip", "_tcp", "local" };

if (suffix != FullQName(kExpectedSuffix))
{
#ifdef MINMDNS_RESOLVER_OVERLY_VERBOSE
ChipLogError(Discovery, "mDNS packet is not for a CHIP device");
#endif
mHasNodePort = false;
return;
}
}

if (ExtractIdFromInstanceName(name.Value(), &mNodeData.mPeerId) != CHIP_NO_ERROR)
{
ChipLogError(Discovery, "Failed to parse peer id from %s", name.Value());
Expand All @@ -171,11 +154,6 @@ void PacketDataReporter::OnOperationalSrvRecord(SerializedQNameIterator name, co

mNodeData.mPort = srv.GetPort();
mHasNodePort = true;

if (mHasIP)
{
mDelegate->OnNodeIdResolved(mNodeData);
}
}

void PacketDataReporter::OnCommissionableNodeSrvRecord(SerializedQNameIterator name, const SrvRecord & srv)
Expand All @@ -199,11 +177,6 @@ void PacketDataReporter::OnOperationalIPAddress(const chip::Inet::IPAddress & ad
// (if multi-admin decides to use unique ports for every ecosystem).
mNodeData.mAddress = addr;
mHasIP = true;

if (mHasNodePort)
{
mDelegate->OnNodeIdResolved(mNodeData);
}
}

void PacketDataReporter::OnCommissionableNodeIPAddress(const chip::Inet::IPAddress & addr)
Expand All @@ -215,6 +188,18 @@ void PacketDataReporter::OnCommissionableNodeIPAddress(const chip::Inet::IPAddre
mCommissionableNodeData.ipAddress[mCommissionableNodeData.numIPs++] = addr;
}

bool HasQNamePart(SerializedQNameIterator qname, QNamePart part)
andy31415 marked this conversation as resolved.
Show resolved Hide resolved
{
while (qname.Next())
{
if (strcmp(qname.Value(), part) == 0)
{
return true;
}
}
return false;
}

void PacketDataReporter::OnResource(ResourceType type, const ResourceData & data)
{
if (!mValid)
Expand All @@ -238,11 +223,26 @@ void PacketDataReporter::OnResource(ResourceType type, const ResourceData & data
}
else if (mDiscoveryType == DiscoveryType::kOperational)
{
OnOperationalSrvRecord(data.GetName(), srv);
// Ensure this is our record.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: reword comment? I am unsure if this helps provide more context or information beyond reading the code.

if (HasQNamePart(data.GetName(), kOperationalServiceName))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

previous code was checking that the suffix was fully _chip._tcp.local.

updated code seems to check only that "_chip" occurs somewhere in the qname. Is this ok and sufficient? Maybe a comment of "note: this does not fully validate path" may be useful if we at some point decide we really need to fully validate the path.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should fix this too.

{
OnOperationalSrvRecord(data.GetName(), srv);
}
else
{
mValid = false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is marking the whole response as invalid even though the record we're looking for might be later in the packet, causing #9099. In fact this is almost always happening in some test scenarios with thread devices.

}
}
else if (mDiscoveryType == DiscoveryType::kCommissionableNode)
{
OnCommissionableNodeSrvRecord(data.GetName(), srv);
if (HasQNamePart(data.GetName(), kCommissionableServiceName))
{
OnCommissionableNodeSrvRecord(data.GetName(), srv);
}
else
{
mValid = false;
}
}
break;
}
Expand Down Expand Up @@ -304,6 +304,10 @@ void PacketDataReporter::OnComplete()
{
mDelegate->OnCommissionableNodeFound(mCommissionableNodeData);
}
else if (mDiscoveryType == DiscoveryType::kOperational && mHasIP && mHasNodePort)
{
mDelegate->OnNodeIdResolved(mNodeData);
}
}

class MinMdnsResolver : public Resolver, public MdnsPacketDelegate
Expand Down