diff --git a/service-t/src/com/android/server/NsdService.java b/service-t/src/com/android/server/NsdService.java index c136d4c69a..9a2cc5f6b6 100644 --- a/service-t/src/com/android/server/NsdService.java +++ b/service-t/src/com/android/server/NsdService.java @@ -55,6 +55,7 @@ import android.os.RemoteException; import android.os.UserHandle; import android.text.TextUtils; import android.util.Log; +import android.util.Pair; import android.util.SparseArray; import com.android.internal.annotations.VisibleForTesting; @@ -599,7 +600,10 @@ public class NsdService extends INsdManager.Stub { final NsdServiceInfo info = args.serviceInfo; id = getUniqueId(); - final String serviceType = constructServiceType(info.getServiceType()); + final Pair typeAndSubtype = + parseTypeAndSubtype(info.getServiceType()); + final String serviceType = typeAndSubtype == null + ? null : typeAndSubtype.first; if (clientInfo.mUseJavaBackend || mDeps.isMdnsDiscoveryManagerEnabled(mContext) || useDiscoveryManagerForType(serviceType)) { @@ -613,12 +617,17 @@ public class NsdService extends INsdManager.Stub { maybeStartMonitoringSockets(); final MdnsListener listener = new DiscoveryListener(clientId, id, info, listenServiceType); - final MdnsSearchOptions options = MdnsSearchOptions.newBuilder() - .setNetwork(info.getNetwork()) - .setIsPassiveMode(true) - .build(); + final MdnsSearchOptions.Builder optionsBuilder = + MdnsSearchOptions.newBuilder() + .setNetwork(info.getNetwork()) + .setIsPassiveMode(true); + if (typeAndSubtype.second != null) { + // The parsing ensures subtype starts with an underscore. + // MdnsSearchOptions expects the underscore to not be present. + optionsBuilder.addSubtype(typeAndSubtype.second.substring(1)); + } mMdnsDiscoveryManager.registerListener( - listenServiceType, listener, options); + listenServiceType, listener, optionsBuilder.build()); storeDiscoveryManagerRequestMap(clientId, id, listener, clientInfo); clientInfo.onDiscoverServicesStarted(clientId, info); clientInfo.log("Register a DiscoveryListener " + id @@ -697,7 +706,9 @@ public class NsdService extends INsdManager.Stub { id = getUniqueId(); final NsdServiceInfo serviceInfo = args.serviceInfo; final String serviceType = serviceInfo.getServiceType(); - final String registerServiceType = constructServiceType(serviceType); + final Pair typeSubtype = parseTypeAndSubtype(serviceType); + final String registerServiceType = typeSubtype == null + ? null : typeSubtype.first; if (clientInfo.mUseJavaBackend || mDeps.isMdnsAdvertiserEnabled(mContext) || useAdvertiserForType(registerServiceType)) { @@ -712,6 +723,10 @@ public class NsdService extends INsdManager.Stub { serviceInfo.getServiceName())); maybeStartMonitoringSockets(); + // TODO: pass in the subtype as well. Including the subtype in the + // service type would generate service instance names like + // Name._subtype._sub._type._tcp, which is incorrect + // (it should be Name._type._tcp). mAdvertiser.addService(id, serviceInfo); storeAdvertiserRequestMap(clientId, id, clientInfo); } else { @@ -778,7 +793,10 @@ public class NsdService extends INsdManager.Stub { final NsdServiceInfo info = args.serviceInfo; id = getUniqueId(); - final String serviceType = constructServiceType(info.getServiceType()); + final Pair typeSubtype = + parseTypeAndSubtype(info.getServiceType()); + final String serviceType = typeSubtype == null + ? null : typeSubtype.first; if (clientInfo.mUseJavaBackend || mDeps.isMdnsDiscoveryManagerEnabled(mContext) || useDiscoveryManagerForType(serviceType)) { @@ -871,7 +889,10 @@ public class NsdService extends INsdManager.Stub { final NsdServiceInfo info = args.serviceInfo; id = getUniqueId(); - final String serviceType = constructServiceType(info.getServiceType()); + final Pair typeAndSubtype = + parseTypeAndSubtype(info.getServiceType()); + final String serviceType = typeAndSubtype == null + ? null : typeAndSubtype.first; if (serviceType == null) { clientInfo.onServiceInfoCallbackRegistrationFailed(clientId, NsdManager.FAILURE_BAD_PARAMETERS); @@ -1315,28 +1336,39 @@ public class NsdService extends INsdManager.Stub { * Check the given service type is valid and construct it to a service type * which can use for discovery / resolution service. * - *

The valid service type should be 2 labels, or 3 labels if the query is for a + *

The valid service type should be 2 labels, or 3 labels if the query is for a * subtype (see RFC6763 7.1). Each label is up to 63 characters and must start with an * underscore; they are alphanumerical characters or dashes or underscore, except the * last one that is just alphanumerical. The last label must be _tcp or _udp. * + *

The subtype may also be specified with a comma after the service type, for example + * _type._tcp,_subtype. + * * @param serviceType the request service type for discovery / resolution service * @return constructed service type or null if the given service type is invalid. */ @Nullable - public static String constructServiceType(String serviceType) { + public static Pair parseTypeAndSubtype(String serviceType) { if (TextUtils.isEmpty(serviceType)) return null; + final String typeOrSubtypePattern = "_[a-zA-Z0-9-_]{1,61}[a-zA-Z0-9]"; final Pattern serviceTypePattern = Pattern.compile( - "^(_[a-zA-Z0-9-_]{1,61}[a-zA-Z0-9]\\.)?" - + "(_[a-zA-Z0-9-_]{1,61}[a-zA-Z0-9]\\._(?:tcp|udp))" + // Optional leading subtype (_subtype._type._tcp) + // (?: xxx) is a non-capturing parenthesis, don't capture the dot + "^(?:(" + typeOrSubtypePattern + ")\\.)?" + // Actual type (_type._tcp.local) + + "(" + typeOrSubtypePattern + "\\._(?:tcp|udp))" // Drop '.' at the end of service type that is compatible with old backend. - + "\\.?$"); + // e.g. allow "_type._tcp.local." + + "\\.?" + // Optional subtype after comma, for "_type._tcp,_subtype" format + + "(?:,(" + typeOrSubtypePattern + "))?" + + "$"); final Matcher matcher = serviceTypePattern.matcher(serviceType); if (!matcher.matches()) return null; - return matcher.group(1) == null - ? matcher.group(2) - : matcher.group(1) + "_sub." + matcher.group(2); + // Use the subtype either at the beginning or after the comma + final String subtype = matcher.group(1) != null ? matcher.group(1) : matcher.group(3); + return new Pair<>(matcher.group(2), subtype); } @VisibleForTesting diff --git a/service-t/src/com/android/server/connectivity/mdns/MdnsServiceTypeClient.java b/service-t/src/com/android/server/connectivity/mdns/MdnsServiceTypeClient.java index 35f9b0475d..bb41594728 100644 --- a/service-t/src/com/android/server/connectivity/mdns/MdnsServiceTypeClient.java +++ b/service-t/src/com/android/server/connectivity/mdns/MdnsServiceTypeClient.java @@ -28,13 +28,16 @@ import android.util.Pair; import com.android.internal.annotations.GuardedBy; import com.android.internal.annotations.VisibleForTesting; +import com.android.net.module.util.CollectionUtils; import com.android.net.module.util.SharedLog; +import com.android.server.connectivity.mdns.util.MdnsUtils; import java.net.Inet4Address; import java.net.Inet6Address; import java.util.ArrayList; import java.util.Arrays; import java.util.Collection; +import java.util.Collections; import java.util.HashMap; import java.util.Iterator; import java.util.List; @@ -205,9 +208,22 @@ public class MdnsServiceTypeClient { private boolean responseMatchesOptions(@NonNull MdnsResponse response, @NonNull MdnsSearchOptions options) { - if (options.getResolveInstanceName() == null) return true; - // DNS is case-insensitive, so ignore case in the comparison - return options.getResolveInstanceName().equalsIgnoreCase(response.getServiceInstanceName()); + final boolean matchesInstanceName = options.getResolveInstanceName() == null + // DNS is case-insensitive, so ignore case in the comparison + || MdnsUtils.equalsIgnoreDnsCase(options.getResolveInstanceName(), + response.getServiceInstanceName()); + + // If discovery is requiring some subtypes, the response must have one that matches a + // requested one. + final List responseSubtypes = response.getSubtypes() == null + ? Collections.emptyList() : response.getSubtypes(); + final boolean matchesSubtype = options.getSubtypes().size() == 0 + || CollectionUtils.any(options.getSubtypes(), requiredSub -> + CollectionUtils.any(responseSubtypes, actualSub -> + MdnsUtils.equalsIgnoreDnsCase( + MdnsConstants.SUBTYPE_PREFIX + requiredSub, actualSub))); + + return matchesInstanceName && matchesSubtype; } /** diff --git a/tests/unit/java/com/android/server/NsdServiceTest.java b/tests/unit/java/com/android/server/NsdServiceTest.java index 5ca393475f..322b4d293a 100644 --- a/tests/unit/java/com/android/server/NsdServiceTest.java +++ b/tests/unit/java/com/android/server/NsdServiceTest.java @@ -23,7 +23,7 @@ import static android.net.nsd.NsdManager.FAILURE_BAD_PARAMETERS; import static android.net.nsd.NsdManager.FAILURE_INTERNAL_ERROR; import static android.net.nsd.NsdManager.FAILURE_OPERATION_NOT_RUNNING; -import static com.android.server.NsdService.constructServiceType; +import static com.android.server.NsdService.parseTypeAndSubtype; import static com.android.testutils.ContextUtils.mockService; import static libcore.junit.util.compat.CoreCompatChangeRule.DisableCompatChanges; @@ -77,6 +77,7 @@ import android.os.IBinder; import android.os.Looper; import android.os.Message; import android.os.RemoteException; +import android.util.Pair; import androidx.annotation.NonNull; import androidx.test.filters.SmallTest; @@ -84,6 +85,7 @@ import androidx.test.filters.SmallTest; import com.android.server.NsdService.Dependencies; import com.android.server.connectivity.mdns.MdnsAdvertiser; import com.android.server.connectivity.mdns.MdnsDiscoveryManager; +import com.android.server.connectivity.mdns.MdnsSearchOptions; import com.android.server.connectivity.mdns.MdnsServiceBrowserListener; import com.android.server.connectivity.mdns.MdnsServiceInfo; import com.android.server.connectivity.mdns.MdnsSocketProvider; @@ -104,6 +106,7 @@ import org.mockito.MockitoAnnotations; import java.net.InetAddress; import java.net.UnknownHostException; +import java.util.Collections; import java.util.LinkedList; import java.util.List; import java.util.Objects; @@ -968,6 +971,35 @@ public class NsdServiceTest { .onStartDiscoveryFailed(serviceTypeWithoutTcpOrUdpEnding, FAILURE_INTERNAL_ERROR); } + @Test + @EnableCompatChanges(ENABLE_PLATFORM_MDNS_BACKEND) + public void testDiscoveryWithMdnsDiscoveryManager_UsesSubtypes() { + final String typeWithSubtype = SERVICE_TYPE + ",_subtype"; + final NsdManager client = connectClient(mService); + final NsdServiceInfo regInfo = new NsdServiceInfo("Instance", typeWithSubtype); + final Network network = new Network(999); + regInfo.setHostAddresses(List.of(parseNumericAddress("192.0.2.123"))); + regInfo.setPort(12345); + regInfo.setNetwork(network); + + final RegistrationListener regListener = mock(RegistrationListener.class); + client.registerService(regInfo, NsdManager.PROTOCOL_DNS_SD, Runnable::run, regListener); + waitForIdle(); + // TODO: also pass the subtype to MdnsAdvertiser + verify(mAdvertiser).addService(anyInt(), argThat(s -> + "Instance".equals(s.getServiceName()) + && SERVICE_TYPE.equals(s.getServiceType()))); + + final DiscoveryListener discListener = mock(DiscoveryListener.class); + client.discoverServices(typeWithSubtype, PROTOCOL, network, Runnable::run, discListener); + waitForIdle(); + final ArgumentCaptor optionsCaptor = + ArgumentCaptor.forClass(MdnsSearchOptions.class); + verify(mDiscoveryManager).registerListener(eq(SERVICE_TYPE + ".local"), any(), + optionsCaptor.capture()); + assertEquals(Collections.singletonList("subtype"), optionsCaptor.getValue().getSubtypes()); + } + @Test public void testResolutionWithMdnsDiscoveryManager() throws UnknownHostException { setMdnsDiscoveryManagerEnabled(); @@ -976,7 +1008,7 @@ public class NsdServiceTest { final ResolveListener resolveListener = mock(ResolveListener.class); final Network network = new Network(999); final String serviceType = "_nsd._service._tcp"; - final String constructedServiceType = "_nsd._sub._service._tcp.local"; + final String constructedServiceType = "_service._tcp.local"; final ArgumentCaptor listenerCaptor = ArgumentCaptor.forClass(MdnsServiceBrowserListener.class); final NsdServiceInfo request = new NsdServiceInfo(SERVICE_NAME, serviceType); @@ -984,12 +1016,14 @@ 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. + final ArgumentCaptor optionsCaptor = + ArgumentCaptor.forClass(MdnsSearchOptions.class); verify(mDiscoveryManager).registerListener(eq(constructedServiceType), - listenerCaptor.capture(), argThat(options -> - network.equals(options.getNetwork()) - && SERVICE_NAME.equals(options.getResolveInstanceName()))); + listenerCaptor.capture(), + optionsCaptor.capture()); + assertEquals(network, optionsCaptor.getValue().getNetwork()); + // Subtypes are not used for resolution, only for discovery + assertEquals(Collections.emptyList(), optionsCaptor.getValue().getSubtypes()); final MdnsServiceBrowserListener listener = listenerCaptor.getValue(); final MdnsServiceInfo mdnsServiceInfo = new MdnsServiceInfo( @@ -1013,8 +1047,7 @@ public class NsdServiceTest { verify(resolveListener, timeout(TIMEOUT_MS)).onServiceResolved(infoCaptor.capture()); final NsdServiceInfo info = infoCaptor.getValue(); assertEquals(SERVICE_NAME, info.getServiceName()); - // TODO(b/266167702): this should be ._service._tcp (as per legacy behavior) - assertEquals("._nsd._sub._service._tcp", info.getServiceType()); + assertEquals("._service._tcp", info.getServiceType()); assertEquals(PORT, info.getPort()); assertTrue(info.getAttributes().containsKey("key")); assertEquals(1, info.getAttributes().size()); @@ -1222,7 +1255,7 @@ public class NsdServiceTest { final ResolveListener resolveListener = mock(ResolveListener.class); final Network network = new Network(999); final String serviceType = "_nsd._service._tcp"; - final String constructedServiceType = "_nsd._sub._service._tcp.local"; + final String constructedServiceType = "_service._tcp.local"; final ArgumentCaptor listenerCaptor = ArgumentCaptor.forClass(MdnsServiceBrowserListener.class); final NsdServiceInfo request = new NsdServiceInfo(SERVICE_NAME, serviceType); @@ -1230,8 +1263,14 @@ public class NsdServiceTest { client.resolveService(request, resolveListener); waitForIdle(); verify(mSocketProvider).startMonitoringSockets(); + final ArgumentCaptor optionsCaptor = + ArgumentCaptor.forClass(MdnsSearchOptions.class); verify(mDiscoveryManager).registerListener(eq(constructedServiceType), - listenerCaptor.capture(), argThat(options -> network.equals(options.getNetwork()))); + listenerCaptor.capture(), + optionsCaptor.capture()); + assertEquals(network, optionsCaptor.getValue().getNetwork()); + // Subtypes are not used for resolution, only for discovery + assertEquals(Collections.emptyList(), optionsCaptor.getValue().getSubtypes()); client.stopServiceResolution(resolveListener); waitForIdle(); @@ -1246,16 +1285,22 @@ public class NsdServiceTest { } @Test - public void testConstructServiceType() { + public void testParseTypeAndSubtype() { final String serviceType1 = "test._tcp"; final String serviceType2 = "_test._quic"; - final String serviceType3 = "_123._udp."; - final String serviceType4 = "_TEST._999._tcp."; + final String serviceType3 = "_test._quic,_test1,_test2"; + final String serviceType4 = "_123._udp."; + final String serviceType5 = "_TEST._999._tcp."; + final String serviceType6 = "_998._tcp.,_TEST"; + final String serviceType7 = "_997._tcp,_TEST"; - assertEquals(null, constructServiceType(serviceType1)); - assertEquals(null, constructServiceType(serviceType2)); - assertEquals("_123._udp", constructServiceType(serviceType3)); - assertEquals("_TEST._sub._999._tcp", constructServiceType(serviceType4)); + assertNull(parseTypeAndSubtype(serviceType1)); + assertNull(parseTypeAndSubtype(serviceType2)); + assertNull(parseTypeAndSubtype(serviceType3)); + assertEquals(new Pair<>("_123._udp", null), parseTypeAndSubtype(serviceType4)); + assertEquals(new Pair<>("_999._tcp", "_TEST"), parseTypeAndSubtype(serviceType5)); + assertEquals(new Pair<>("_998._tcp", "_TEST"), parseTypeAndSubtype(serviceType6)); + assertEquals(new Pair<>("_997._tcp", "_TEST"), parseTypeAndSubtype(serviceType7)); } @Test diff --git a/tests/unit/java/com/android/server/connectivity/mdns/MdnsServiceTypeClientTests.java b/tests/unit/java/com/android/server/connectivity/mdns/MdnsServiceTypeClientTests.java index 2fcdff24d6..5c13f141b1 100644 --- a/tests/unit/java/com/android/server/connectivity/mdns/MdnsServiceTypeClientTests.java +++ b/tests/unit/java/com/android/server/connectivity/mdns/MdnsServiceTypeClientTests.java @@ -41,6 +41,7 @@ import android.net.InetAddresses; import android.net.Network; import android.text.TextUtils; +import com.android.net.module.util.CollectionUtils; import com.android.net.module.util.SharedLog; import com.android.server.connectivity.mdns.MdnsServiceInfo.TextEntry; import com.android.server.connectivity.mdns.MdnsServiceTypeClient.QueryTaskConfig; @@ -52,6 +53,7 @@ import org.junit.Ignore; import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.ArgumentCaptor; +import org.mockito.ArgumentMatcher; import org.mockito.Captor; import org.mockito.InOrder; import org.mockito.Mock; @@ -1056,6 +1058,87 @@ public class MdnsServiceTypeClientTests { inOrder.verify(mockListenerTwo).onServiceRemoved(matchServiceName(otherInstance)); } + @Test + public void testProcessResponse_SubtypeDiscoveryLimitedToSubtype() { + client = new MdnsServiceTypeClient( + SERVICE_TYPE, mockSocketClient, currentThreadExecutor, mockNetwork, mockSharedLog); + + final String matchingInstance = "instance1"; + final String subtype = "_subtype"; + final String otherInstance = "instance2"; + final String ipV4Address = "192.0.2.0"; + final String ipV6Address = "2001:db8::"; + + final MdnsSearchOptions options = MdnsSearchOptions.newBuilder() + // Search with different case. Note MdnsSearchOptions subtype doesn't start with "_" + .addSubtype("Subtype").build(); + + client.startSendAndReceive(mockListenerOne, options); + client.startSendAndReceive(mockListenerTwo, MdnsSearchOptions.getDefaultOptions()); + + // Complete response from instanceName + final MdnsPacket packetWithoutSubtype = createResponse( + matchingInstance, ipV4Address, 5353, SERVICE_TYPE_LABELS, + Collections.emptyMap() /* textAttributes */, TEST_TTL); + final MdnsPointerRecord originalPtr = (MdnsPointerRecord) CollectionUtils.findFirst( + packetWithoutSubtype.answers, r -> r instanceof MdnsPointerRecord); + + // Add a subtype PTR record + final ArrayList newAnswers = new ArrayList<>(packetWithoutSubtype.answers); + newAnswers.add(new MdnsPointerRecord( + // PTR should be _subtype._sub._type._tcp.local -> instance1._type._tcp.local + Stream.concat(Stream.of(subtype, "_sub"), Arrays.stream(SERVICE_TYPE_LABELS)) + .toArray(String[]::new), + originalPtr.getReceiptTime(), originalPtr.getCacheFlush(), originalPtr.getTtl(), + originalPtr.getPointer())); + final MdnsPacket packetWithSubtype = new MdnsPacket( + packetWithoutSubtype.flags, + packetWithoutSubtype.questions, + newAnswers, + packetWithoutSubtype.authorityRecords, + packetWithoutSubtype.additionalRecords); + client.processResponse(packetWithSubtype, INTERFACE_INDEX, mockNetwork); + + // Complete response from otherInstanceName, without subtype + client.processResponse(createResponse( + otherInstance, ipV4Address, 5353, SERVICE_TYPE_LABELS, + Collections.emptyMap() /* textAttributes */, TEST_TTL), + INTERFACE_INDEX, mockNetwork); + + // Address update from otherInstanceName + client.processResponse(createResponse( + otherInstance, ipV6Address, 5353, SERVICE_TYPE_LABELS, + Collections.emptyMap(), TEST_TTL), INTERFACE_INDEX, mockNetwork); + + // Goodbye from otherInstanceName + client.processResponse(createResponse( + otherInstance, ipV6Address, 5353, SERVICE_TYPE_LABELS, + Collections.emptyMap(), 0L /* ttl */), INTERFACE_INDEX, mockNetwork); + + // mockListenerOne gets notified for the requested instance + final ArgumentMatcher subtypeInstanceMatcher = info -> + info.getServiceInstanceName().equals(matchingInstance) + && info.getSubtypes().equals(Collections.singletonList(subtype)); + verify(mockListenerOne).onServiceNameDiscovered(argThat(subtypeInstanceMatcher)); + verify(mockListenerOne).onServiceFound(argThat(subtypeInstanceMatcher)); + + // ...but does not get any callback for the other instance + verify(mockListenerOne, never()).onServiceFound(matchServiceName(otherInstance)); + verify(mockListenerOne, never()).onServiceNameDiscovered(matchServiceName(otherInstance)); + verify(mockListenerOne, never()).onServiceUpdated(matchServiceName(otherInstance)); + verify(mockListenerOne, never()).onServiceRemoved(matchServiceName(otherInstance)); + + // mockListenerTwo gets notified for both though + final InOrder inOrder = inOrder(mockListenerTwo); + inOrder.verify(mockListenerTwo).onServiceNameDiscovered(argThat(subtypeInstanceMatcher)); + inOrder.verify(mockListenerTwo).onServiceFound(argThat(subtypeInstanceMatcher)); + + inOrder.verify(mockListenerTwo).onServiceNameDiscovered(matchServiceName(otherInstance)); + inOrder.verify(mockListenerTwo).onServiceFound(matchServiceName(otherInstance)); + inOrder.verify(mockListenerTwo).onServiceUpdated(matchServiceName(otherInstance)); + inOrder.verify(mockListenerTwo).onServiceRemoved(matchServiceName(otherInstance)); + } + @Test public void testNotifyAllServicesRemoved() { client = new MdnsServiceTypeClient(