From 43997a7d2aaf0d80e808d532d67d968e40ccaa7b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Damian=20Kr=C3=B3lik?= <66667989+Damian-Nordic@users.noreply.github.com> Date: Mon, 9 Aug 2021 20:53:27 +0200 Subject: [PATCH] [srp-server] fix adding service with subtypes to existing host (#6901) When a new service with subtypes is added to an existing host, the SRP server incorrectly merges the subtype service copies so that the TXT information is lost, i.e. 0-length TXT record is created. Moreover, the OT DNS client cannot parse services with 0-length TXT record (instead of ignoring the TXT record itself) causing that such a service cannot be examined at all. The problem is caused by incorrect merging of the service description structure which is shared across all services with the same instance name (i.e. among the base type service and its subtypes). Signed-off-by: Damian Krolik --- src/core/net/srp_server.cpp | 9 ++++++++- tests/scripts/thread-cert/test_dnssd.py | 26 +++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 1 deletion(-) diff --git a/src/core/net/srp_server.cpp b/src/core/net/srp_server.cpp index 02863ccda..8f7af84c0 100644 --- a/src/core/net/srp_server.cpp +++ b/src/core/net/srp_server.cpp @@ -1781,7 +1781,14 @@ Error Server::Host::MergeServicesAndResourcesFrom(Host &aHost) newService->mIsDeleted = false; newService->mIsCommitted = true; newService->mTimeLastUpdate = TimerMilli::GetNow(); - newService->mDescription.TakeResourcesFrom(service->mDescription); + + if (!service->mIsSubType) + { + // (1) Service description is shared across a base type and all its subtypes. + // (2) `TakeResourcesFrom()` releases resources pinned to its argument. + // Therefore, make sure the function is called only for the base type. + newService->mDescription.TakeResourcesFrom(service->mDescription); + } newService->Log((existingService != nullptr) ? Service::kUpdateExisting : Service::kAddNew); } diff --git a/tests/scripts/thread-cert/test_dnssd.py b/tests/scripts/thread-cert/test_dnssd.py index a508d47a0..98dbb20de 100755 --- a/tests/scripts/thread-cert/test_dnssd.py +++ b/tests/scripts/thread-cert/test_dnssd.py @@ -156,6 +156,18 @@ class TestDnssd(thread_cert.TestCase): 'aaaa_ttl': lambda x: x > 0, } + instance4_verify_info = { + 'port': 44444, + 'priority': 4, + 'weight': 4, + 'host': 'host3.default.service.arpa.', + 'address': client3_addrs, + 'txt_data': 'KEY=414243', # KEY=ABC + 'srv_ttl': lambda x: x > 0, + 'txt_ttl': lambda x: x > 0, + 'aaaa_ttl': lambda x: x > 0, + } + # Browse for main service service_instances = client1.dns_browse(f'{SERVICE}.{DOMAIN}', server.get_mleid(), 53) self.assertEqual({'ins1', 'ins2', 'ins3'}, set(service_instances.keys())) @@ -182,6 +194,20 @@ class TestDnssd(thread_cert.TestCase): service_instance = client1.dns_resolve_service('ins2', f'{SERVICE}.{DOMAIN}', server.get_mleid(), 53) self._assert_service_instance_equal(service_instance, instance2_verify_info) + #--------------------------------------------------------------- + # Add another service with TXT entries to the existing host and + # verify that it is properly merged. + + client3.srp_client_add_service('ins4', SERVICE + ",_s1", 44444, 4, 4, txt_entries=['KEY=ABC']) + self.simulator.go(5) + + service_instances = client1.dns_browse(f'{SERVICE}.{DOMAIN}', server.get_mleid(), 53) + self.assertEqual({'ins1', 'ins2', 'ins3', 'ins4'}, set(service_instances.keys())) + self._assert_service_instance_equal(service_instances['ins1'], instance1_verify_info) + self._assert_service_instance_equal(service_instances['ins2'], instance2_verify_info) + self._assert_service_instance_equal(service_instances['ins3'], instance3_verify_info) + self._assert_service_instance_equal(service_instances['ins4'], instance4_verify_info) + def _assert_service_instance_equal(self, instance, info): for f in ('port', 'priority', 'weight', 'host', 'txt_data'): self.assertEqual(instance[f], info[f], instance)