Ensure service type ends with a dot in discovery am: 2f82fcdd6c

Original change: https://android-review.googlesource.com/c/platform/packages/modules/Connectivity/+/2584551 Change-Id: I30b3d1ebe49be670f70b52a7aa75603234f99564 Signed-off-by: Automerger Merge Worker <android-build-automerger-merge-worker@system.gserviceaccount.com>
2023-05-12 08:29:08 +00:00
parent 8bdeebe280 2f82fcdd6c
commit ed0eb5f385
3 changed files with 80 additions and 30 deletions
--- 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<String, String> attrs = serviceInfo.getAttributes();
--- 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<ServiceFound> {
+            val serviceFound = expectCallbackEventually<ServiceFound> {
                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<ServiceRegistered>(
                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<DiscoveryStarted>()

        // 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<ServiceResolved>().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<ServiceUnregistered>()

        // Expect a callback for service lost
-        discoveryRecord.expectCallbackEventually<ServiceLost> {
+        val lostCb = discoveryRecord.expectCallbackEventually<ServiceLost> {
            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<ServiceFound>()
            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<ServiceFound>()
            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<ServiceFound>()
            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)
--- 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());