From 1a8ee102d3cc9829b669b09685b17ea92815b671 Mon Sep 17 00:00:00 2001 From: Remi NGUYEN VAN Date: Mon, 30 May 2022 12:42:24 +0900 Subject: [PATCH] Fix service resolve on tethering downstreams Tethering downstreams do not have NetworkAgents, and although they have a netid of 99, Networks with netId 99 are not usable by apps for most connectivity APIs. Recent refactoring in NsdService adds the Network of a found service into its NsdServiceInfo, and uses that network to resolve the service. In that case the Network has netId 99 and resolving the service fails. Avoid that problem by: - Keeping the Network field null when a service is found on a tethering downstream; this avoids giving apps a confusing and unusable Network with netId 99 - Using the interface index found during discovery to resolve the service, if the app uses the NsdServiceInfo that was obtained from discovery to resolve. If not, all interfaces will be used to resolve, as per legacy APIs. Bug: 233979892 Test: atest NsdServiceTest Also manual test with 2 devices connected via hotspot Change-Id: Idd176153b67ccbd1d4f1b1fd66dafaa2f3a9e27a --- .../src/android/net/nsd/NsdServiceInfo.java | 31 ++++- .../src/com/android/server/NsdService.java | 66 ++++++++--- tests/common/Android.bp | 24 ++++ tests/unit/Android.bp | 2 +- .../android/net/nsd/NsdServiceInfoTest.java | 2 + .../com/android/server/NsdServiceTest.java | 108 ++++++++++++++++++ 6 files changed, 211 insertions(+), 22 deletions(-) diff --git a/framework-t/src/android/net/nsd/NsdServiceInfo.java b/framework-t/src/android/net/nsd/NsdServiceInfo.java index 2621594e98..200c808565 100644 --- a/framework-t/src/android/net/nsd/NsdServiceInfo.java +++ b/framework-t/src/android/net/nsd/NsdServiceInfo.java @@ -53,6 +53,8 @@ public final class NsdServiceInfo implements Parcelable { @Nullable private Network mNetwork; + private int mInterfaceIndex; + public NsdServiceInfo() { } @@ -312,8 +314,11 @@ public final class NsdServiceInfo implements Parcelable { /** * Get the network where the service can be found. * - * This is never null if this {@link NsdServiceInfo} was obtained from - * {@link NsdManager#discoverServices} or {@link NsdManager#resolveService}. + * This is set if this {@link NsdServiceInfo} was obtained from + * {@link NsdManager#discoverServices} or {@link NsdManager#resolveService}, unless the service + * was found on a network interface that does not have a {@link Network} (such as a tethering + * downstream, where services are advertised from devices connected to this device via + * tethering). */ @Nullable public Network getNetwork() { @@ -329,6 +334,26 @@ public final class NsdServiceInfo implements Parcelable { mNetwork = network; } + /** + * Get the index of the network interface where the service was found. + * + * This is only set when the service was found on an interface that does not have a usable + * Network, in which case {@link #getNetwork()} returns null. + * @return The interface index as per {@link java.net.NetworkInterface#getIndex}, or 0 if unset. + * @hide + */ + public int getInterfaceIndex() { + return mInterfaceIndex; + } + + /** + * Set the index of the network interface where the service was found. + * @hide + */ + public void setInterfaceIndex(int interfaceIndex) { + mInterfaceIndex = interfaceIndex; + } + @Override public String toString() { StringBuilder sb = new StringBuilder(); @@ -375,6 +400,7 @@ public final class NsdServiceInfo implements Parcelable { } dest.writeParcelable(mNetwork, 0); + dest.writeInt(mInterfaceIndex); } /** Implement the Parcelable interface */ @@ -405,6 +431,7 @@ public final class NsdServiceInfo implements Parcelable { info.mTxtRecord.put(in.readString(), valueArray); } info.mNetwork = in.readParcelable(null, Network.class); + info.mInterfaceIndex = in.readInt(); return info; } diff --git a/service-t/src/com/android/server/NsdService.java b/service-t/src/com/android/server/NsdService.java index 6def44f3c0..95e6114255 100644 --- a/service-t/src/com/android/server/NsdService.java +++ b/service-t/src/com/android/server/NsdService.java @@ -23,6 +23,7 @@ import android.content.Context; import android.content.Intent; import android.content.pm.PackageManager; import android.net.ConnectivityManager; +import android.net.INetd; import android.net.LinkProperties; import android.net.Network; import android.net.mdns.aidl.DiscoveryInfo; @@ -466,7 +467,7 @@ public class NsdService extends INsdManager.Stub { // interfaces that do not have an associated Network. break; } - servInfo.setNetwork(new Network(foundNetId)); + setServiceNetworkForCallback(servInfo, info.netId, info.interfaceIdx); clientInfo.onServiceFound(clientId, servInfo); break; } @@ -476,10 +477,11 @@ public class NsdService extends INsdManager.Stub { final String type = info.registrationType; final int lostNetId = info.netId; servInfo = new NsdServiceInfo(name, type); - // The network could be null if it was torn down when the service is lost - // TODO: avoid returning null in that case, possibly by remembering found - // services on the same interface index and their network at the time - servInfo.setNetwork(lostNetId == 0 ? null : new Network(lostNetId)); + // The network could be set to null (netId 0) if it was torn down when the + // service is lost + // TODO: avoid returning null in that case, possibly by remembering + // found services on the same interface index and their network at the time + setServiceNetworkForCallback(servInfo, lostNetId, info.interfaceIdx); clientInfo.onServiceLost(clientId, servInfo); break; } @@ -557,7 +559,6 @@ public class NsdService extends INsdManager.Stub { final GetAddressInfo info = (GetAddressInfo) obj; final String address = info.address; final int netId = info.netId; - final Network network = netId == NETID_UNSET ? null : new Network(netId); InetAddress serviceHost = null; try { serviceHost = InetAddress.getByName(address); @@ -568,9 +569,10 @@ public class NsdService extends INsdManager.Stub { // If the resolved service is on an interface without a network, consider it // as a failure: it would not be usable by apps as they would need // privileged permissions. - if (network != null && serviceHost != null) { + if (netId != NETID_UNSET && serviceHost != null) { clientInfo.mResolvedService.setHost(serviceHost); - clientInfo.mResolvedService.setNetwork(network); + setServiceNetworkForCallback(clientInfo.mResolvedService, + netId, info.interfaceIdx); clientInfo.onResolveServiceSucceeded( clientId, clientInfo.mResolvedService); } else { @@ -590,6 +592,26 @@ public class NsdService extends INsdManager.Stub { } } + private static void setServiceNetworkForCallback(NsdServiceInfo info, int netId, int ifaceIdx) { + switch (netId) { + case NETID_UNSET: + info.setNetwork(null); + break; + case INetd.LOCAL_NET_ID: + // Special case for LOCAL_NET_ID: Networks on netId 99 are not generally + // visible / usable for apps, so do not return it. Store the interface + // index instead, so at least if the client tries to resolve the service + // with that NsdServiceInfo, it will be done on the same interface. + // If they recreate the NsdServiceInfo themselves, resolution would be + // done on all interfaces as before T, which should also work. + info.setNetwork(null); + info.setInterfaceIndex(ifaceIdx); + break; + default: + info.setNetwork(new Network(netId)); + } + } + // The full service name is escaped from standard DNS rules on mdnsresponder, making it suitable // for passing to standard system DNS APIs such as res_query() . Thus, make the service name // unescape for getting right service address. See "Notes on DNS Name Escaping" on @@ -767,9 +789,8 @@ public class NsdService extends INsdManager.Stub { String type = service.getServiceType(); int port = service.getPort(); byte[] textRecord = service.getTxtRecord(); - final Network network = service.getNetwork(); - final int registerInterface = getNetworkInterfaceIndex(network); - if (network != null && registerInterface == IFACE_IDX_ANY) { + final int registerInterface = getNetworkInterfaceIndex(service); + if (service.getNetwork() != null && registerInterface == IFACE_IDX_ANY) { Log.e(TAG, "Interface to register service on not found"); return false; } @@ -781,10 +802,9 @@ public class NsdService extends INsdManager.Stub { } private boolean discoverServices(int discoveryId, NsdServiceInfo serviceInfo) { - final Network network = serviceInfo.getNetwork(); final String type = serviceInfo.getServiceType(); - final int discoverInterface = getNetworkInterfaceIndex(network); - if (network != null && discoverInterface == IFACE_IDX_ANY) { + final int discoverInterface = getNetworkInterfaceIndex(serviceInfo); + if (serviceInfo.getNetwork() != null && discoverInterface == IFACE_IDX_ANY) { Log.e(TAG, "Interface to discover service on not found"); return false; } @@ -798,9 +818,8 @@ public class NsdService extends INsdManager.Stub { private boolean resolveService(int resolveId, NsdServiceInfo service) { final String name = service.getServiceName(); final String type = service.getServiceType(); - final Network network = service.getNetwork(); - final int resolveInterface = getNetworkInterfaceIndex(network); - if (network != null && resolveInterface == IFACE_IDX_ANY) { + final int resolveInterface = getNetworkInterfaceIndex(service); + if (service.getNetwork() != null && resolveInterface == IFACE_IDX_ANY) { Log.e(TAG, "Interface to resolve service on not found"); return false; } @@ -816,8 +835,17 @@ public class NsdService extends INsdManager.Stub { * this is to support the legacy mdnsresponder implementation, which historically resolved * services on an unspecified network. */ - private int getNetworkInterfaceIndex(Network network) { - if (network == null) return IFACE_IDX_ANY; + private int getNetworkInterfaceIndex(NsdServiceInfo serviceInfo) { + final Network network = serviceInfo.getNetwork(); + if (network == null) { + // Fallback to getInterfaceIndex if present (typically if the NsdServiceInfo was + // provided by NsdService from discovery results, and the service was found on an + // interface that has no app-usable Network). + if (serviceInfo.getInterfaceIndex() != 0) { + return serviceInfo.getInterfaceIndex(); + } + return IFACE_IDX_ANY; + } final ConnectivityManager cm = mContext.getSystemService(ConnectivityManager.class); if (cm == null) { diff --git a/tests/common/Android.bp b/tests/common/Android.bp index 509e881a8b..58731e0c74 100644 --- a/tests/common/Android.bp +++ b/tests/common/Android.bp @@ -140,6 +140,30 @@ java_defaults { ], } +// defaults for tests that need to build against framework-connectivity's @hide APIs, but also +// using fully @hide classes that are jarjared (because they have no API member). Similar to +// framework-connectivity-test-defaults above but uses pre-jarjar class names. +// Only usable from targets that have visibility on framework-connectivity-pre-jarjar, and apply +// connectivity jarjar rules so that references to jarjared classes still match: this is limited to +// connectivity internal tests only. +java_defaults { + name: "framework-connectivity-internal-test-defaults", + sdk_version: "core_platform", // tests can use @CorePlatformApi's + libs: [ + // order matters: classes in framework-connectivity are resolved before framework, + // meaning @hide APIs in framework-connectivity are resolved before @SystemApi + // stubs in framework + "framework-connectivity-pre-jarjar", + "framework-connectivity-t-pre-jarjar", + "framework-tethering.impl", + "framework", + + // if sdk_version="" this gets automatically included, but here we need to add manually. + "framework-res", + ], + defaults_visibility: ["//packages/modules/Connectivity/tests:__subpackages__"], +} + // Defaults for tests that want to run in mainline-presubmit. // Not widely used because many of our tests have AndroidTest.xml files and // use the mainline-param config-descriptor metadata in AndroidTest.xml. diff --git a/tests/unit/Android.bp b/tests/unit/Android.bp index 5b926de36d..5a7208cf2b 100644 --- a/tests/unit/Android.bp +++ b/tests/unit/Android.bp @@ -108,7 +108,7 @@ java_defaults { name: "FrameworksNetTestsDefaults", min_sdk_version: "30", defaults: [ - "framework-connectivity-test-defaults", + "framework-connectivity-internal-test-defaults", ], srcs: [ "java/**/*.java", diff --git a/tests/unit/java/android/net/nsd/NsdServiceInfoTest.java b/tests/unit/java/android/net/nsd/NsdServiceInfoTest.java index e5e7ebce80..892e1401cc 100644 --- a/tests/unit/java/android/net/nsd/NsdServiceInfoTest.java +++ b/tests/unit/java/android/net/nsd/NsdServiceInfoTest.java @@ -125,6 +125,7 @@ public class NsdServiceInfoTest { fullInfo.setPort(4242); fullInfo.setHost(LOCALHOST); fullInfo.setNetwork(new Network(123)); + fullInfo.setInterfaceIndex(456); checkParcelable(fullInfo); NsdServiceInfo noHostInfo = new NsdServiceInfo(); @@ -175,6 +176,7 @@ public class NsdServiceInfoTest { assertEquals(original.getHost(), result.getHost()); assertTrue(original.getPort() == result.getPort()); assertEquals(original.getNetwork(), result.getNetwork()); + assertEquals(original.getInterfaceIndex(), result.getInterfaceIndex()); // Assert equality of attribute map. Map originalMap = original.getAttributes(); diff --git a/tests/unit/java/com/android/server/NsdServiceTest.java b/tests/unit/java/com/android/server/NsdServiceTest.java index 3c228d0609..ed9e930500 100644 --- a/tests/unit/java/com/android/server/NsdServiceTest.java +++ b/tests/unit/java/com/android/server/NsdServiceTest.java @@ -19,7 +19,9 @@ package com.android.server; import static libcore.junit.util.compat.CoreCompatChangeRule.DisableCompatChanges; import static libcore.junit.util.compat.CoreCompatChangeRule.EnableCompatChanges; +import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertNotNull; +import static org.junit.Assert.assertNull; import static org.mockito.ArgumentMatchers.anyInt; import static org.mockito.ArgumentMatchers.anyString; import static org.mockito.Mockito.any; @@ -36,6 +38,12 @@ import static org.mockito.Mockito.when; import android.compat.testing.PlatformCompatChangeRule; import android.content.ContentResolver; import android.content.Context; +import android.net.INetd; +import android.net.InetAddresses; +import android.net.mdns.aidl.DiscoveryInfo; +import android.net.mdns.aidl.GetAddressInfo; +import android.net.mdns.aidl.IMDnsEventListener; +import android.net.mdns.aidl.ResolutionInfo; import android.net.nsd.INsdManagerCallback; import android.net.nsd.INsdServiceConnector; import android.net.nsd.MDnsManager; @@ -63,6 +71,7 @@ import org.junit.Test; import org.junit.rules.TestRule; import org.junit.runner.RunWith; import org.mockito.AdditionalAnswers; +import org.mockito.ArgumentCaptor; import org.mockito.Mock; import org.mockito.MockitoAnnotations; @@ -275,6 +284,105 @@ public class NsdServiceTest { verify(mMockMDnsM, never()).stopDaemon(); } + @Test + public void testDiscoverOnTetheringDownstream() throws Exception { + NsdService service = makeService(); + NsdManager client = connectClient(service); + + final String serviceType = "a_type"; + final String serviceName = "a_name"; + final String domainName = "mytestdevice.local"; + final int interfaceIdx = 123; + final NsdManager.DiscoveryListener discListener = mock(NsdManager.DiscoveryListener.class); + client.discoverServices(serviceType, NsdManager.PROTOCOL_DNS_SD, discListener); + waitForIdle(); + + final ArgumentCaptor listenerCaptor = + ArgumentCaptor.forClass(IMDnsEventListener.class); + verify(mMockMDnsM).registerEventListener(listenerCaptor.capture()); + final ArgumentCaptor discIdCaptor = ArgumentCaptor.forClass(Integer.class); + verify(mMockMDnsM).discover(discIdCaptor.capture(), eq(serviceType), + eq(0) /* interfaceIdx */); + // NsdManager uses a separate HandlerThread to dispatch callbacks (on ServiceHandler), so + // this needs to use a timeout + verify(discListener, timeout(TIMEOUT_MS)).onDiscoveryStarted(serviceType); + + final DiscoveryInfo discoveryInfo = new DiscoveryInfo( + discIdCaptor.getValue(), + IMDnsEventListener.SERVICE_FOUND, + serviceName, + serviceType, + domainName, + interfaceIdx, + INetd.LOCAL_NET_ID); // LOCAL_NET_ID (99) used on tethering downstreams + final IMDnsEventListener eventListener = listenerCaptor.getValue(); + eventListener.onServiceDiscoveryStatus(discoveryInfo); + waitForIdle(); + + final ArgumentCaptor discoveredInfoCaptor = + ArgumentCaptor.forClass(NsdServiceInfo.class); + verify(discListener, timeout(TIMEOUT_MS)).onServiceFound(discoveredInfoCaptor.capture()); + final NsdServiceInfo foundInfo = discoveredInfoCaptor.getValue(); + assertEquals(serviceName, foundInfo.getServiceName()); + assertEquals(serviceType, foundInfo.getServiceType()); + assertNull(foundInfo.getHost()); + assertNull(foundInfo.getNetwork()); + assertEquals(interfaceIdx, foundInfo.getInterfaceIndex()); + + // After discovering the service, verify resolving it + final NsdManager.ResolveListener resolveListener = mock(NsdManager.ResolveListener.class); + client.resolveService(foundInfo, resolveListener); + waitForIdle(); + + final ArgumentCaptor resolvIdCaptor = ArgumentCaptor.forClass(Integer.class); + verify(mMockMDnsM).resolve(resolvIdCaptor.capture(), eq(serviceName), eq(serviceType), + eq("local.") /* domain */, eq(interfaceIdx)); + + final int servicePort = 10123; + final String serviceFullName = serviceName + "." + serviceType; + final ResolutionInfo resolutionInfo = new ResolutionInfo( + resolvIdCaptor.getValue(), + IMDnsEventListener.SERVICE_RESOLVED, + null /* serviceName */, + null /* serviceType */, + null /* domain */, + serviceFullName, + domainName, + servicePort, + new byte[0] /* txtRecord */, + interfaceIdx); + + doReturn(true).when(mMockMDnsM).getServiceAddress(anyInt(), any(), anyInt()); + eventListener.onServiceResolutionStatus(resolutionInfo); + waitForIdle(); + + final ArgumentCaptor getAddrIdCaptor = ArgumentCaptor.forClass(Integer.class); + verify(mMockMDnsM).getServiceAddress(getAddrIdCaptor.capture(), eq(domainName), + eq(interfaceIdx)); + + final String serviceAddress = "192.0.2.123"; + final GetAddressInfo addressInfo = new GetAddressInfo( + getAddrIdCaptor.getValue(), + IMDnsEventListener.SERVICE_GET_ADDR_SUCCESS, + serviceFullName, + serviceAddress, + interfaceIdx, + INetd.LOCAL_NET_ID); + eventListener.onGettingServiceAddressStatus(addressInfo); + waitForIdle(); + + final ArgumentCaptor resInfoCaptor = + ArgumentCaptor.forClass(NsdServiceInfo.class); + verify(resolveListener, timeout(TIMEOUT_MS)).onServiceResolved(resInfoCaptor.capture()); + final NsdServiceInfo resolvedService = resInfoCaptor.getValue(); + assertEquals(serviceName, resolvedService.getServiceName()); + assertEquals("." + serviceType, resolvedService.getServiceType()); + assertEquals(InetAddresses.parseNumericAddress(serviceAddress), resolvedService.getHost()); + assertEquals(servicePort, resolvedService.getPort()); + assertNull(resolvedService.getNetwork()); + assertEquals(interfaceIdx, resolvedService.getInterfaceIndex()); + } + private void waitForIdle() { HandlerUtils.waitForIdle(mHandler, TIMEOUT_MS); }