Implement proper discovery with subtypes
Apps may want to discover a particular subtype, such as
_color._sub._printer._tcp.local, which may return services like
Printer1._printer._tcp.local. The previous code was trying to discover,
and would return service types named _color._sub._printer._tcp.local,
even though the actual service type is still _printer._tcp.local. This
is a regression compared to S/T.
Fix this by passing the subtype to MdnsDiscoveryManager in its options
instead of the service type, and ensure that MdnsDiscoveryManager only
sends callbacks to callers that have requested the given subtype (a
color that asked for _color._sub._printer._tcp.local should not receive
BlackAndWhite._printer._tcp.local).
Bug: 266167702
Test: atest
(cherry picked from https://android-review.googlesource.com/q/commit:f2d064112c34eb703dabdd5a48008ba4b3183a0b)
Merged-In: I21367c66534078667718a9b54dfc858b12ba7103
Change-Id: I21367c66534078667718a9b54dfc858b12ba7103
diff --git a/service-t/src/com/android/server/NsdService.java b/service-t/src/com/android/server/NsdService.java
index c136d4c..9a2cc5f 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.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 @@
final NsdServiceInfo info = args.serviceInfo;
id = getUniqueId();
- final String serviceType = constructServiceType(info.getServiceType());
+ final Pair<String, String> typeAndSubtype =
+ parseTypeAndSubtype(info.getServiceType());
+ final String serviceType = typeAndSubtype == null
+ ? null : typeAndSubtype.first;
if (clientInfo.mUseJavaBackend
|| mDeps.isMdnsDiscoveryManagerEnabled(mContext)
|| useDiscoveryManagerForType(serviceType)) {
@@ -613,12 +617,17 @@
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 @@
id = getUniqueId();
final NsdServiceInfo serviceInfo = args.serviceInfo;
final String serviceType = serviceInfo.getServiceType();
- final String registerServiceType = constructServiceType(serviceType);
+ final Pair<String, String> typeSubtype = parseTypeAndSubtype(serviceType);
+ final String registerServiceType = typeSubtype == null
+ ? null : typeSubtype.first;
if (clientInfo.mUseJavaBackend
|| mDeps.isMdnsAdvertiserEnabled(mContext)
|| useAdvertiserForType(registerServiceType)) {
@@ -712,6 +723,10 @@
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 @@
final NsdServiceInfo info = args.serviceInfo;
id = getUniqueId();
- final String serviceType = constructServiceType(info.getServiceType());
+ final Pair<String, String> typeSubtype =
+ parseTypeAndSubtype(info.getServiceType());
+ final String serviceType = typeSubtype == null
+ ? null : typeSubtype.first;
if (clientInfo.mUseJavaBackend
|| mDeps.isMdnsDiscoveryManagerEnabled(mContext)
|| useDiscoveryManagerForType(serviceType)) {
@@ -871,7 +889,10 @@
final NsdServiceInfo info = args.serviceInfo;
id = getUniqueId();
- final String serviceType = constructServiceType(info.getServiceType());
+ final Pair<String, String> 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 @@
* Check the given service type is valid and construct it to a service type
* which can use for discovery / resolution service.
*
- * <p> The valid service type should be 2 labels, or 3 labels if the query is for a
+ * <p>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.
*
+ * <p>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<String, String> 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 35f9b04..bb41594 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 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 @@
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<String> 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 5ca3934..322b4d2 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_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.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 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 java.net.InetAddress;
import java.net.UnknownHostException;
+import java.util.Collections;
import java.util.LinkedList;
import java.util.List;
import java.util.Objects;
@@ -969,6 +972,35 @@
}
@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<MdnsSearchOptions> 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 @@
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<MdnsServiceBrowserListener> listenerCaptor =
ArgumentCaptor.forClass(MdnsServiceBrowserListener.class);
final NsdServiceInfo request = new NsdServiceInfo(SERVICE_NAME, serviceType);
@@ -984,12 +1016,14 @@
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<MdnsSearchOptions> 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 @@
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 @@
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<MdnsServiceBrowserListener> listenerCaptor =
ArgumentCaptor.forClass(MdnsServiceBrowserListener.class);
final NsdServiceInfo request = new NsdServiceInfo(SERVICE_NAME, serviceType);
@@ -1230,8 +1263,14 @@
client.resolveService(request, resolveListener);
waitForIdle();
verify(mSocketProvider).startMonitoringSockets();
+ final ArgumentCaptor<MdnsSearchOptions> 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 @@
}
@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 2fcdff2..5c13f14 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.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.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;
@@ -1057,6 +1059,87 @@
}
@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<MdnsRecord> 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<MdnsServiceInfo> 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(
SERVICE_TYPE, mockSocketClient, currentThreadExecutor, mockNetwork, mockSharedLog);