From 2f82fcdd6ca82928ae24533cd8616561a4737df1 Mon Sep 17 00:00:00 2001 From: Remi NGUYEN VAN Date: Wed, 10 May 2023 13:24:53 +0900 Subject: [PATCH] Ensure service type ends with a dot in discovery Historical behavior of onServiceFound is to include a dot at the end of the service type. To avoid inconsistencies between behavior of new and older backend, which would be hard to predict/handle for apps, ensure that the same format is used. This means a dot must be added when using the new backend. Implement this by creating the callback service types based on MdnsServiceInfo obtained from MdnsDiscoveryManager, rather than the service type that was provided by the calling app. This avoids inconsistencies as the calling app may or may not include dots originally, and it may use subtypes ("_sub._type._tcp.local" or even "_type._tcp.local,_sub"), which would result in callbacks giving invalid service types, or inconsistent with S/T behavior. Fixes: 281776253 Test: atest Change-Id: I677ee885d1ee8e6a09773f21e4dd53e96faee748 --- .../src/com/android/server/NsdService.java | 59 ++++++++++++++----- .../net/src/android/net/cts/NsdManagerTest.kt | 40 +++++++++---- .../com/android/server/NsdServiceTest.java | 11 +++- 3 files changed, 80 insertions(+), 30 deletions(-) diff --git a/service-t/src/com/android/server/NsdService.java b/service-t/src/com/android/server/NsdService.java index 1e0d544b68..c136d4c69a 100644 --- a/service-t/src/com/android/server/NsdService.java +++ b/service-t/src/com/android/server/NsdService.java @@ -19,6 +19,7 @@ package com.android.server; import static android.net.ConnectivityManager.NETID_UNSET; import static android.net.nsd.NsdManager.MDNS_DISCOVERY_MANAGER_EVENT; import static android.net.nsd.NsdManager.MDNS_SERVICE_EVENT; +import static android.net.nsd.NsdManager.RESOLVE_SERVICE_SUCCEEDED; import static android.provider.DeviceConfig.NAMESPACE_TETHERING; import static com.android.modules.utils.build.SdkLevel.isAtLeastU; @@ -83,6 +84,7 @@ import java.net.NetworkInterface; import java.net.SocketException; import java.net.UnknownHostException; import java.util.ArrayList; +import java.util.Arrays; import java.util.HashMap; import java.util.List; import java.util.Map; @@ -242,14 +244,14 @@ public class NsdService extends INsdManager.Stub { public void onServiceNameDiscovered(@NonNull MdnsServiceInfo serviceInfo) { mNsdStateMachine.sendMessage(MDNS_DISCOVERY_MANAGER_EVENT, mTransactionId, NsdManager.SERVICE_FOUND, - new MdnsEvent(mClientId, mReqServiceInfo.getServiceType(), serviceInfo)); + new MdnsEvent(mClientId, serviceInfo)); } @Override public void onServiceNameRemoved(@NonNull MdnsServiceInfo serviceInfo) { mNsdStateMachine.sendMessage(MDNS_DISCOVERY_MANAGER_EVENT, mTransactionId, NsdManager.SERVICE_LOST, - new MdnsEvent(mClientId, mReqServiceInfo.getServiceType(), serviceInfo)); + new MdnsEvent(mClientId, serviceInfo)); } } @@ -264,7 +266,7 @@ public class NsdService extends INsdManager.Stub { public void onServiceFound(MdnsServiceInfo serviceInfo) { mNsdStateMachine.sendMessage(MDNS_DISCOVERY_MANAGER_EVENT, mTransactionId, NsdManager.RESOLVE_SERVICE_SUCCEEDED, - new MdnsEvent(mClientId, mReqServiceInfo.getServiceType(), serviceInfo)); + new MdnsEvent(mClientId, serviceInfo)); } } @@ -279,21 +281,21 @@ public class NsdService extends INsdManager.Stub { public void onServiceFound(@NonNull MdnsServiceInfo serviceInfo) { mNsdStateMachine.sendMessage(MDNS_DISCOVERY_MANAGER_EVENT, mTransactionId, NsdManager.SERVICE_UPDATED, - new MdnsEvent(mClientId, mReqServiceInfo.getServiceType(), serviceInfo)); + new MdnsEvent(mClientId, serviceInfo)); } @Override public void onServiceUpdated(@NonNull MdnsServiceInfo serviceInfo) { mNsdStateMachine.sendMessage(MDNS_DISCOVERY_MANAGER_EVENT, mTransactionId, NsdManager.SERVICE_UPDATED, - new MdnsEvent(mClientId, mReqServiceInfo.getServiceType(), serviceInfo)); + new MdnsEvent(mClientId, serviceInfo)); } @Override public void onServiceRemoved(@NonNull MdnsServiceInfo serviceInfo) { mNsdStateMachine.sendMessage(MDNS_DISCOVERY_MANAGER_EVENT, mTransactionId, NsdManager.SERVICE_UPDATED_LOST, - new MdnsEvent(mClientId, mReqServiceInfo.getServiceType(), serviceInfo)); + new MdnsEvent(mClientId, serviceInfo)); } } @@ -303,14 +305,10 @@ public class NsdService extends INsdManager.Stub { private static class MdnsEvent { final int mClientId; @NonNull - final String mRequestedServiceType; - @NonNull final MdnsServiceInfo mMdnsServiceInfo; - MdnsEvent(int clientId, @NonNull String requestedServiceType, - @NonNull MdnsServiceInfo mdnsServiceInfo) { + MdnsEvent(int clientId, @NonNull MdnsServiceInfo mdnsServiceInfo) { mClientId = clientId; - mRequestedServiceType = requestedServiceType; mMdnsServiceInfo = mdnsServiceInfo; } } @@ -1104,9 +1102,38 @@ public class NsdService extends INsdManager.Stub { return true; } - private NsdServiceInfo buildNsdServiceInfoFromMdnsEvent(final MdnsEvent event) { + @Nullable + private NsdServiceInfo buildNsdServiceInfoFromMdnsEvent( + final MdnsEvent event, int code) { final MdnsServiceInfo serviceInfo = event.mMdnsServiceInfo; - final String serviceType = event.mRequestedServiceType; + final String[] typeArray = serviceInfo.getServiceType(); + final String joinedType; + if (typeArray.length == 0 + || !typeArray[typeArray.length - 1].equals(LOCAL_DOMAIN_NAME)) { + Log.wtf(TAG, "MdnsServiceInfo type does not end in .local: " + + Arrays.toString(typeArray)); + return null; + } else { + joinedType = TextUtils.join(".", + Arrays.copyOfRange(typeArray, 0, typeArray.length - 1)); + } + final String serviceType; + switch (code) { + case NsdManager.SERVICE_FOUND: + case NsdManager.SERVICE_LOST: + // For consistency with historical behavior, discovered service types have + // a dot at the end. + serviceType = joinedType + "."; + break; + case RESOLVE_SERVICE_SUCCEEDED: + // For consistency with historical behavior, resolved service types have + // a dot at the beginning. + serviceType = "." + joinedType; + break; + default: + serviceType = joinedType; + break; + } final String serviceName = serviceInfo.getServiceInstanceName(); final NsdServiceInfo servInfo = new NsdServiceInfo(serviceName, serviceType); final Network network = serviceInfo.getNetwork(); @@ -1131,7 +1158,9 @@ public class NsdService extends INsdManager.Stub { final MdnsEvent event = (MdnsEvent) obj; final int clientId = event.mClientId; - final NsdServiceInfo info = buildNsdServiceInfoFromMdnsEvent(event); + final NsdServiceInfo info = buildNsdServiceInfoFromMdnsEvent(event, code); + // Errors are already logged if null + if (info == null) return false; if (DBG) { Log.d(TAG, String.format("MdnsDiscoveryManager event code=%s transactionId=%d", NsdManager.nameOf(code), transactionId)); @@ -1150,8 +1179,6 @@ public class NsdService extends INsdManager.Stub { break; } final MdnsServiceInfo serviceInfo = event.mMdnsServiceInfo; - // Add '.' in front of the service type that aligns with historical behavior - info.setServiceType("." + event.mRequestedServiceType); info.setPort(serviceInfo.getPort()); Map attrs = serviceInfo.getAttributes(); diff --git a/tests/cts/net/src/android/net/cts/NsdManagerTest.kt b/tests/cts/net/src/android/net/cts/NsdManagerTest.kt index db7f38cb79..88b9baff1e 100644 --- a/tests/cts/net/src/android/net/cts/NsdManagerTest.kt +++ b/tests/cts/net/src/android/net/cts/NsdManagerTest.kt @@ -282,13 +282,17 @@ class NsdManagerTest { fun waitForServiceDiscovered( serviceName: String, + serviceType: String, expectedNetwork: Network? = null ): NsdServiceInfo { - return expectCallbackEventually { + val serviceFound = expectCallbackEventually { it.serviceInfo.serviceName == serviceName && (expectedNetwork == null || expectedNetwork == nsdShim.getNetwork(it.serviceInfo)) }.serviceInfo + // Discovered service types have a dot at the end + assertEquals("$serviceType.", serviceFound.serviceType) + return serviceFound } } @@ -497,6 +501,10 @@ class NsdManagerTest { val registeredInfo = registrationRecord.expectCallback( REGISTRATION_TIMEOUT_MS).serviceInfo + // Only service name is included in ServiceRegistered callbacks + assertNull(registeredInfo.serviceType) + assertEquals(si.serviceName, registeredInfo.serviceName) + val discoveryRecord = NsdDiscoveryRecord() // Test discovering without an Executor nsdManager.discoverServices(serviceType, NsdManager.PROTOCOL_DNS_SD, discoveryRecord) @@ -505,12 +513,15 @@ class NsdManagerTest { discoveryRecord.expectCallback() // Expect a service record to be discovered - val foundInfo = discoveryRecord.waitForServiceDiscovered(registeredInfo.serviceName) + val foundInfo = discoveryRecord.waitForServiceDiscovered( + registeredInfo.serviceName, serviceType) // Test resolving without an Executor val resolveRecord = NsdResolveRecord() nsdManager.resolveService(foundInfo, resolveRecord) val resolvedService = resolveRecord.expectCallback().serviceInfo + assertEquals(".$serviceType", resolvedService.serviceType) + assertEquals(registeredInfo.serviceName, resolvedService.serviceName) // Check Txt attributes assertEquals(8, resolvedService.attributes.size) @@ -538,9 +549,11 @@ class NsdManagerTest { registrationRecord.expectCallback() // Expect a callback for service lost - discoveryRecord.expectCallbackEventually { + val lostCb = discoveryRecord.expectCallbackEventually { it.serviceInfo.serviceName == serviceName } + // Lost service types have a dot at the end + assertEquals("$serviceType.", lostCb.serviceInfo.serviceType) // Register service again to see if NsdManager can discover it val si2 = NsdServiceInfo() @@ -554,7 +567,8 @@ class NsdManagerTest { // Expect a service record to be discovered (and filter the ones // that are unrelated to this test) - val foundInfo2 = discoveryRecord.waitForServiceDiscovered(registeredInfo2.serviceName) + val foundInfo2 = discoveryRecord.waitForServiceDiscovered( + registeredInfo2.serviceName, serviceType) // Resolve the service val resolveRecord2 = NsdResolveRecord() @@ -591,7 +605,7 @@ class NsdManagerTest { testNetwork1.network, Executor { it.run() }, discoveryRecord) val foundInfo = discoveryRecord.waitForServiceDiscovered( - serviceName, testNetwork1.network) + serviceName, serviceType, testNetwork1.network) assertEquals(testNetwork1.network, nsdShim.getNetwork(foundInfo)) // Rewind to ensure the service is not found on the other interface @@ -638,6 +652,8 @@ class NsdManagerTest { val serviceDiscovered = discoveryRecord.expectCallback() assertEquals(registeredInfo1.serviceName, serviceDiscovered.serviceInfo.serviceName) + // Discovered service types have a dot at the end + assertEquals("$serviceType.", serviceDiscovered.serviceInfo.serviceType) assertEquals(testNetwork1.network, nsdShim.getNetwork(serviceDiscovered.serviceInfo)) // Unregister, then register the service back: it should be lost and found again @@ -650,6 +666,7 @@ class NsdManagerTest { val registeredInfo2 = registerService(registrationRecord, si, executor) val serviceDiscovered2 = discoveryRecord.expectCallback() assertEquals(registeredInfo2.serviceName, serviceDiscovered2.serviceInfo.serviceName) + assertEquals("$serviceType.", serviceDiscovered2.serviceInfo.serviceType) assertEquals(testNetwork1.network, nsdShim.getNetwork(serviceDiscovered2.serviceInfo)) // Teardown, then bring back up a network on the test interface: the service should @@ -665,6 +682,7 @@ class NsdManagerTest { val newNetwork = newAgent.network ?: fail("Registered agent should have a network") val serviceDiscovered3 = discoveryRecord.expectCallback() assertEquals(registeredInfo2.serviceName, serviceDiscovered3.serviceInfo.serviceName) + assertEquals("$serviceType.", serviceDiscovered3.serviceInfo.serviceType) assertEquals(newNetwork, nsdShim.getNetwork(serviceDiscovered3.serviceInfo)) } cleanupStep { nsdManager.stopServiceDiscovery(discoveryRecord) @@ -740,12 +758,12 @@ class NsdManagerTest { nsdManager.discoverServices(serviceType, NsdManager.PROTOCOL_DNS_SD, discoveryRecord) val foundInfo1 = discoveryRecord.waitForServiceDiscovered( - serviceName, testNetwork1.network) + serviceName, serviceType, testNetwork1.network) assertEquals(testNetwork1.network, nsdShim.getNetwork(foundInfo1)) // Rewind as the service could be found on each interface in any order discoveryRecord.nextEvents.rewind(0) val foundInfo2 = discoveryRecord.waitForServiceDiscovered( - serviceName, testNetwork2.network) + serviceName, serviceType, testNetwork2.network) assertEquals(testNetwork2.network, nsdShim.getNetwork(foundInfo2)) nsdShim.resolveService(nsdManager, foundInfo1, Executor { it.run() }, resolveRecord) @@ -790,7 +808,7 @@ class NsdManagerTest { testNetwork1.network, Executor { it.run() }, discoveryRecord) // Expect that service is found on testNetwork1 val foundInfo = discoveryRecord.waitForServiceDiscovered( - serviceName, testNetwork1.network) + serviceName, serviceType, testNetwork1.network) assertEquals(testNetwork1.network, nsdShim.getNetwork(foundInfo)) // Discover service on testNetwork2. @@ -805,7 +823,7 @@ class NsdManagerTest { null as Network? /* network */, Executor { it.run() }, discoveryRecord3) // Expect that service is found on testNetwork1 val foundInfo3 = discoveryRecord3.waitForServiceDiscovered( - serviceName, testNetwork1.network) + serviceName, serviceType, testNetwork1.network) assertEquals(testNetwork1.network, nsdShim.getNetwork(foundInfo3)) } cleanupStep { nsdManager.stopServiceDiscovery(discoveryRecord2) @@ -835,7 +853,7 @@ class NsdManagerTest { nsdManager.discoverServices( serviceType, NsdManager.PROTOCOL_DNS_SD, discoveryRecord ) - val foundInfo = discoveryRecord.waitForServiceDiscovered(serviceNames) + val foundInfo = discoveryRecord.waitForServiceDiscovered(serviceNames, serviceType) // Expect that resolving the service name works properly even service name contains // non-standard characters. @@ -985,7 +1003,7 @@ class NsdManagerTest { nsdShim.discoverServices(nsdManager, serviceType, NsdManager.PROTOCOL_DNS_SD, testNetwork1.network, Executor { it.run() }, discoveryRecord) val foundInfo = discoveryRecord.waitForServiceDiscovered( - serviceName, testNetwork1.network) + serviceName, serviceType, testNetwork1.network) // Register service callback and check the addresses are the same as network addresses nsdShim.registerServiceInfoCallback(nsdManager, foundInfo, { it.run() }, cbRecord) diff --git a/tests/unit/java/com/android/server/NsdServiceTest.java b/tests/unit/java/com/android/server/NsdServiceTest.java index 14b4280029..5ca393475f 100644 --- a/tests/unit/java/com/android/server/NsdServiceTest.java +++ b/tests/unit/java/com/android/server/NsdServiceTest.java @@ -908,7 +908,8 @@ public class NsdServiceTest { listener.onServiceNameDiscovered(foundInfo); verify(discListener, timeout(TIMEOUT_MS)).onServiceFound(argThat(info -> info.getServiceName().equals(SERVICE_NAME) - && info.getServiceType().equals(SERVICE_TYPE) + // Service type in discovery callbacks has a dot at the end + && info.getServiceType().equals(SERVICE_TYPE + ".") && info.getNetwork().equals(network))); final MdnsServiceInfo removedInfo = new MdnsServiceInfo( @@ -927,7 +928,8 @@ public class NsdServiceTest { listener.onServiceNameRemoved(removedInfo); verify(discListener, timeout(TIMEOUT_MS)).onServiceLost(argThat(info -> info.getServiceName().equals(SERVICE_NAME) - && info.getServiceType().equals(SERVICE_TYPE) + // Service type in discovery callbacks has a dot at the end + && info.getServiceType().equals(SERVICE_TYPE + ".") && info.getNetwork().equals(network))); client.stopServiceDiscovery(discListener); @@ -982,6 +984,8 @@ public class NsdServiceTest { client.resolveService(request, resolveListener); waitForIdle(); verify(mSocketProvider).startMonitoringSockets(); + // TODO(b/266167702): this is a bug, as registerListener should be done _service._tcp, and + // _sub should be in the list of subtypes in the options. verify(mDiscoveryManager).registerListener(eq(constructedServiceType), listenerCaptor.capture(), argThat(options -> network.equals(options.getNetwork()) @@ -1009,7 +1013,8 @@ public class NsdServiceTest { verify(resolveListener, timeout(TIMEOUT_MS)).onServiceResolved(infoCaptor.capture()); final NsdServiceInfo info = infoCaptor.getValue(); assertEquals(SERVICE_NAME, info.getServiceName()); - assertEquals("." + serviceType, info.getServiceType()); + // TODO(b/266167702): this should be ._service._tcp (as per legacy behavior) + assertEquals("._nsd._sub._service._tcp", info.getServiceType()); assertEquals(PORT, info.getPort()); assertTrue(info.getAttributes().containsKey("key")); assertEquals(1, info.getAttributes().size());