Skip to content

Commit

Permalink
MinMdns - do not reset hasIP/hasHostPort on partial failure (#18114)
Browse files Browse the repository at this point in the history
* MinMdns - do not reset hasIP/hasHostPort on partial failure

MinMdns can receive several packets and if any of the packets are ok,
a single valid ip or host/port is sufficient.

In practice, this was failing on mdns on ipv6 only builds when both ipv6
and ipv4 were sent in order. Specifically:
  - MDNS packet contains AAAA followed by A
  - Resolver parses AAAA and sets a valid IP
  - Resolver sees A and fails to parse (no IPv4 support) and resets
    valid IP

As a result, dns resolution was never calling complete (because it
thinks no valid IPs).

* Add some logs that would have helped debugging incomplete/invalid packets

* Add more logging for all paths that may result in "invalid node"

* Code review update and add missing return
  • Loading branch information
andy31415 authored and pull[bot] committed Oct 11, 2023
1 parent 5007ce8 commit 1246903
Showing 1 changed file with 32 additions and 9 deletions.
41 changes: 32 additions & 9 deletions src/lib/dnssd/Resolver_ImplMinimalMdns.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -116,7 +116,9 @@ void PacketDataReporter::OnQuery(const QueryData & data)

void PacketDataReporter::OnHeader(ConstHeaderRef & header)
{
mValid = header.GetFlags().IsResponse();
mValid = header.GetFlags().IsResponse();
mHasIP = false; // will need to get at least one valid IP eventually
mHasNodePort = false; // also need node-port which we do not have yet

if (header.GetFlags().IsTruncated())
{
Expand All @@ -140,14 +142,12 @@ void PacketDataReporter::OnOperationalSrvRecord(SerializedQNameIterator name, co
#ifdef MINMDNS_RESOLVER_OVERLY_VERBOSE
ChipLogError(Discovery, "mDNS packet is missing a valid server name");
#endif
mHasNodePort = false;
return;
}

if (ExtractIdFromInstanceName(name.Value(), &mNodeData.mPeerId) != CHIP_NO_ERROR)
{
ChipLogError(Discovery, "Failed to parse peer id from %s", name.Value());
mHasNodePort = false;
return;
}

Expand Down Expand Up @@ -229,7 +229,6 @@ void PacketDataReporter::OnResource(ResourceType type, const ResourceData & data
if (!srv.Parse(data.GetData(), mPacketRange))
{
ChipLogError(Discovery, "Packet data reporter failed to parse SRV record");
mHasNodePort = false;
}
else if (mDiscoveryType == DiscoveryType::kOperational)
{
Expand All @@ -239,6 +238,10 @@ void PacketDataReporter::OnResource(ResourceType type, const ResourceData & data
{
OnOperationalSrvRecord(data.GetName(), srv);
}
else
{
ChipLogError(Discovery, "Invalid operational srv name: no '%s' part found.", kOperationalServiceName);
}
}
else if (mDiscoveryType == DiscoveryType::kCommissionableNode || mDiscoveryType == DiscoveryType::kCommissionerNode)
{
Expand All @@ -247,6 +250,11 @@ void PacketDataReporter::OnResource(ResourceType type, const ResourceData & data
{
OnCommissionableNodeSrvRecord(data.GetName(), srv);
}
else
{
ChipLogError(Discovery, "Invalid commision srv name: no '%s' or '%s' part found.", kCommissionableServiceName,
kCommissionerServiceName);
}
}
break;
}
Expand Down Expand Up @@ -279,7 +287,6 @@ void PacketDataReporter::OnResource(ResourceType type, const ResourceData & data
if (!ParseARecord(data.GetData(), &addr))
{
ChipLogError(Discovery, "Packet data reporter failed to parse A record");
mHasIP = false;
}
else
{
Expand All @@ -299,7 +306,6 @@ void PacketDataReporter::OnResource(ResourceType type, const ResourceData & data
if (!ParseAAAARecord(data.GetData(), &addr))
{
ChipLogError(Discovery, "Packet data reporter failed to parse AAAA record");
mHasIP = false;
}
else
{
Expand All @@ -321,9 +327,14 @@ void PacketDataReporter::OnResource(ResourceType type, const ResourceData & data

void PacketDataReporter::OnComplete(ActiveResolveAttempts & activeAttempts)
{
if ((mDiscoveryType == DiscoveryType::kCommissionableNode || mDiscoveryType == DiscoveryType::kCommissionerNode) &&
mDiscoveredNodeData.IsValid())
if (mDiscoveryType == DiscoveryType::kCommissionableNode || mDiscoveryType == DiscoveryType::kCommissionerNode)
{
if (!mDiscoveredNodeData.IsValid())
{
ChipLogError(Discovery, "Discovered node data is not valid. Commissioning discovery not complete.");
return;
}

activeAttempts.Complete(mDiscoveredNodeData);
if (mCommissioningDelegate != nullptr)
{
Expand All @@ -334,8 +345,20 @@ void PacketDataReporter::OnComplete(ActiveResolveAttempts & activeAttempts)
ChipLogError(Discovery, "No delegate to report commissioning node discovery");
}
}
else if (mDiscoveryType == DiscoveryType::kOperational && mHasIP && mHasNodePort)
else if (mDiscoveryType == DiscoveryType::kOperational)
{
if (!mHasIP)
{
ChipLogError(Discovery, "Operational discovery has no valid ip address. Resolve not complete.");
return;
}

if (!mHasNodePort)
{
ChipLogError(Discovery, "Operational discovery has no valid node/port. Resolve not complete.");
return;
}

activeAttempts.Complete(mNodeData.mPeerId);
mNodeData.LogNodeIdResolved();

Expand Down

0 comments on commit 1246903

Please sign in to comment.