From 9eff7616b39547024a1630aa8cbdecd76184425b Mon Sep 17 00:00:00 2001 From: kangping Date: Fri, 21 May 2021 00:07:39 +0800 Subject: [PATCH] [srp-server] fix removing services when the host is expired (#6649) The SRP server tries to notify the advertising proxy when a service is expired. The expired service is firstly removed from the service list and later added back to the head of the service list if the service name should be retailed. This will result in deadloop when there are two or more services on the same host. This commit fixes this issue by keeping the expired service in the service list by simply mark the service deleted and pass all services of the host to the advertising proxy. The advertising proxy will republish those services that are not expired but this should be fine since service expiration is not a common case. Also update the advertising proxy test case to cover multiple services. --- src/core/net/srp_server.cpp | 21 +++------- .../border_router/test_advertising_proxy.py | 41 +++++++++++++------ 2 files changed, 33 insertions(+), 29 deletions(-) diff --git a/src/core/net/srp_server.cpp b/src/core/net/srp_server.cpp index c78ae87f5..2cf8a1d45 100644 --- a/src/core/net/srp_server.cpp +++ b/src/core/net/srp_server.cpp @@ -1223,11 +1223,12 @@ void Server::HandleLeaseTimer(void) otLogInfoSrp("[server] LEASE of host %s expired", host->GetFullName()); // If the host expired, delete all resources of this host and its services. - RemoveHost(host, /* aRetainName */ true, /* aNotifyServiceHandler */ true); while ((service = host->GetNextService(service)) != nullptr) { - host->RemoveService(service, /* aRetainName */ true, /* aNotifyServiceHandler */ true); + // Don't need to notify the service handler as `RemoveHost` at below will do. + host->RemoveService(service, /* aRetainName */ true, /* aNotifyServiceHandler */ false); } + RemoveHost(host, /* aRetainName */ true, /* aNotifyServiceHandler */ true); earliestExpireTime = OT_MIN(earliestExpireTime, host->GetKeyExpireTime()); } @@ -1576,15 +1577,8 @@ void Server::Host::RemoveService(Service *aService, bool aRetainName, bool aNoti otLogInfoSrp("[server] fully remove service '%s'", aService->mFullName); } - IgnoreError(mServices.Remove(*aService)); - if (aNotifyServiceHandler && server.mServiceUpdateHandler != nullptr) { - LinkedList remainingServices = mServices; - - mServices.Clear(); - IgnoreError(mServices.Add(*aService)); - server.mServiceUpdateHandler(server.AllocateId(), this, kDefaultEventsHandlerTimeout, server.mServiceUpdateHandlerContext); // We don't wait for the reply from the service update handler, @@ -1592,16 +1586,11 @@ void Server::Host::RemoveService(Service *aService, bool aRetainName, bool aNoti // Because removing a service should fail only when there is system // failure of the platform mDNS implementation and in which case the // service is not expected to be still registered. - - mServices = remainingServices; } - if (aRetainName) - { - IgnoreError(mServices.Add(*aService)); - } - else + if (!aRetainName) { + IgnoreError(mServices.Remove(*aService)); aService->Free(); } diff --git a/tests/scripts/thread-cert/border_router/test_advertising_proxy.py b/tests/scripts/thread-cert/border_router/test_advertising_proxy.py index 0944148e8..8d6d052ed 100755 --- a/tests/scripts/thread-cert/border_router/test_advertising_proxy.py +++ b/tests/scripts/thread-cert/border_router/test_advertising_proxy.py @@ -97,17 +97,20 @@ class SingleHostAndService(thread_cert.TestCase): client.srp_client_set_host_name('my-host') client.srp_client_set_host_address('2001::1') client.srp_client_add_service('my-service', '_ipps._tcp', 12345) + client.srp_client_add_service('my-service-1', '_ipps._tcp', 12345) client.srp_client_enable_auto_start_mode() self.simulator.go(2) - self.check_host_and_service(server, client, '2001::1') + self.check_host_and_service(server, client, '2001::1', 'my-service') + self.check_host_and_service(server, client, '2001::1', 'my-service-1') # # 2. Discover the service by the HOST on the ethernet. This makes sure # the Advertising Proxy multicasts the same service on ethernet. # - self.host_check_mdns_service(host, '2001::1') + self.host_check_mdns_service(host, '2001::1', 'my-service') + self.host_check_mdns_service(host, '2001::1', 'my-service-1') # # 3. Check if the Advertising Proxy removes the service from ethernet @@ -118,6 +121,7 @@ class SingleHostAndService(thread_cert.TestCase): self.simulator.go(2) self.assertIsNone(host.discover_mdns_service('my-service', '_ipps._tcp', 'my-host')) + self.assertIsNone(host.discover_mdns_service('my-service-1', '_ipps._tcp', 'my-host')) # # 4. Check if we can discover the mDNS service again when re-registering the @@ -127,10 +131,13 @@ class SingleHostAndService(thread_cert.TestCase): client.srp_client_set_host_name('my-host') client.srp_client_set_host_address('2001::1') client.srp_client_add_service('my-service', '_ipps._tcp', 12345) + client.srp_client_add_service('my-service-1', '_ipps._tcp', 12345) self.simulator.go(2) - self.check_host_and_service(server, client, '2001::1') - self.host_check_mdns_service(host, '2001::1') + self.check_host_and_service(server, client, '2001::1', 'my-service') + self.check_host_and_service(server, client, '2001::1', 'my-service-1') + self.host_check_mdns_service(host, '2001::1', 'my-service') + self.host_check_mdns_service(host, '2001::1', 'my-service-1') # # 5. Update the SRP host address and make sure the Advertising Proxy @@ -140,8 +147,10 @@ class SingleHostAndService(thread_cert.TestCase): client.srp_client_set_host_address('2001::2') self.simulator.go(8) - self.check_host_and_service(server, client, '2001::2') - self.host_check_mdns_service(host, '2001::2') + self.check_host_and_service(server, client, '2001::2', 'my-service') + self.check_host_and_service(server, client, '2001::2', 'my-service-1') + self.host_check_mdns_service(host, '2001::2', 'my-service') + self.host_check_mdns_service(host, '2001::2', 'my-service-1') # # 6. Check if the service is removed by the Advertising Proxy when the SRP server is stopped. @@ -153,12 +162,15 @@ class SingleHostAndService(thread_cert.TestCase): self.assertEqual(len(server.srp_server_get_hosts()), 0) self.assertEqual(len(server.srp_server_get_services()), 0) self.assertIsNone(host.discover_mdns_service('my-service', '_ipps._tcp', 'my-host')) + self.assertIsNone(host.discover_mdns_service('my-service-1', '_ipps._tcp', 'my-host')) server.srp_server_set_enabled(True) self.simulator.go(LEASE) - self.check_host_and_service(server, client, '2001::2') - self.host_check_mdns_service(host, '2001::2') + self.check_host_and_service(server, client, '2001::2', 'my-service') + self.check_host_and_service(server, client, '2001::2', 'my-service-1') + self.host_check_mdns_service(host, '2001::2', 'my-service') + self.host_check_mdns_service(host, '2001::2', 'my-service-1') # # 7. Check if the expired service is removed by the Advertising Proxy. @@ -168,11 +180,12 @@ class SingleHostAndService(thread_cert.TestCase): self.simulator.go(LEASE + 2) self.assertIsNone(host.discover_mdns_service('my-service', '_ipps._tcp', 'my-host')) + self.assertIsNone(host.discover_mdns_service('my-service-1', '_ipps._tcp', 'my-host')) - def host_check_mdns_service(self, host, host_addr): - service = host.discover_mdns_service('my-service', '_ipps._tcp', 'my-host') + def host_check_mdns_service(self, host, host_addr, service_instance): + service = host.discover_mdns_service(service_instance, '_ipps._tcp', 'my-host') self.assertIsNotNone(service) - self.assertEqual(service['instance'], 'my-service') + self.assertEqual(service['instance'], service_instance) self.assertEqual(service['name'], '_ipps._tcp') self.assertEqual(service['port'], 12345) self.assertEqual(service['priority'], 0) @@ -181,17 +194,18 @@ class SingleHostAndService(thread_cert.TestCase): self.assertEqual(ipaddress.ip_address(service['addresses'][0]), ipaddress.ip_address(host_addr)) self.assertEqual(len(service['addresses']), 1) - def check_host_and_service(self, server, client, host_addr): + def check_host_and_service(self, server, client, host_addr, service_instance): """Check that we have properly registered host and service instance. """ client_services = client.srp_client_get_services() print(client_services) + client_services = [service for service in client_services if service['instance'] == service_instance] self.assertEqual(len(client_services), 1) client_service = client_services[0] # Verify that the client possesses correct service resources. - self.assertEqual(client_service['instance'], 'my-service') + self.assertEqual(client_service['instance'], service_instance) self.assertEqual(client_service['name'], '_ipps._tcp') self.assertEqual(int(client_service['port']), 12345) self.assertEqual(int(client_service['priority']), 0) @@ -202,6 +216,7 @@ class SingleHostAndService(thread_cert.TestCase): server_services = server.srp_server_get_services() print(server_services) + server_services = [service for service in server_services if service['instance'] == service_instance] self.assertEqual(len(server_services), 1) server_service = server_services[0]