Fix cache flush always causing response change

When the cache flush bit is set on host address records in a response,
known records are cleared and replaced with the contents of the packet.
However even if the response contains the same records as before, this
caused it to be marked as modified, and response modified callbacks are
sent for every incoming packet.

Instead, ensure that the response is only marked as modified if a
previously unknown address record is added, or if a previously known
address record is removed.

The issue wasn't found by the existing testDecodeWithNoChange test,
because it used a service record for testhost2 instead of testhost1, so
the address records were ignored.

Original change: https://r.android.com/2619297

Bug: 285997766
Bug: 285084489
Test: atest CtsNetTest FrameworksNetTests

Change-Id: I178ed8398afe4354c09bdb3fa5c221377385417e
Merged-In: Ic0f19adf5d9bde7cdab766e49cf677b319a2219b

service-t/src/com/android/server/connectivity/mdns/MdnsResponse.java

@@ -95,6 +95,14 @@ public class MdnsResponse {
         return !isSame;
     }
 
+    /**
+     * @return True if this response contains an identical (original TTL included) record.
+     */
+    public boolean hasIdenticalRecord(@NonNull MdnsRecord record) {
+        final int existing = records.indexOf(record);
+        return existing >= 0 && recordsAreSame(record, records.get(existing));
+    }
+
     /**
      * Adds a pointer record.
      *

service-t/src/com/android/server/connectivity/mdns/MdnsResponseDecoder.java

@@ -20,6 +20,7 @@ import android.annotation.NonNull;
 import android.annotation.Nullable;
 import android.net.Network;
 import android.os.SystemClock;
+import android.util.ArrayMap;
 import android.util.ArraySet;
 import android.util.Pair;
 
@@ -139,8 +140,11 @@ public class MdnsResponseDecoder {
 
         final ArraySet<MdnsResponse> modified = new ArraySet<>();
         final ArrayList<MdnsResponse> responses = new ArrayList<>(existingResponses.size());
+        final ArrayMap<MdnsResponse, MdnsResponse> augmentedToOriginal = new ArrayMap<>();
         for (MdnsResponse existing : existingResponses) {
-            responses.add(new MdnsResponse(existing));
+            final MdnsResponse copy = new MdnsResponse(existing);
+            responses.add(copy);
+            augmentedToOriginal.put(copy, existing);
         }
         // The response records are structured in a hierarchy, where some records reference
         // others, as follows:
@@ -166,7 +170,7 @@ public class MdnsResponseDecoder {
         // A: host name -> IP address
 
         // Loop 1: find PTR records, which identify distinct service instances.
-        long now = SystemClock.elapsedRealtime();
+        long now = clock.elapsedRealtime();
         for (MdnsRecord record : records) {
             if (record instanceof MdnsPointerRecord) {
                 String[] name = record.getName();
@@ -262,19 +266,37 @@ public class MdnsResponseDecoder {
                         findResponsesWithHostName(responses, inetRecord.getName());
                 for (MdnsResponse response : matchingResponses) {
                     if (assignInetRecord(response, inetRecord)) {
+                        final MdnsResponse originalResponse = augmentedToOriginal.get(response);
+                        if (originalResponse == null
+                                || !originalResponse.hasIdenticalRecord(inetRecord)) {
                             modified.add(response);
                         }
                     }
+                }
             } else {
                 MdnsResponse response =
                         findResponseWithHostName(responses, inetRecord.getName());
                 if (response != null) {
                     if (assignInetRecord(response, inetRecord)) {
+                        final MdnsResponse originalResponse = augmentedToOriginal.get(response);
+                        if (originalResponse == null
+                                || !originalResponse.hasIdenticalRecord(inetRecord)) {
                             modified.add(response);
                         }
                     }
                 }
             }
+        }
+
+        // Only responses that have new or modified address records were added to the modified set.
+        // Make sure responses that have lost address records are added to the set too.
+        for (int i = 0; i < augmentedToOriginal.size(); i++) {
+            final MdnsResponse augmented = augmentedToOriginal.keyAt(i);
+            final MdnsResponse original = augmentedToOriginal.valueAt(i);
+            if (augmented.getRecords().size() != original.getRecords().size()) {
+                modified.add(augmented);
+            }
+        }
 
         return Pair.create(modified, responses);
     }

tests/unit/java/com/android/server/connectivity/mdns/MdnsResponseDecoderTests.java

@@ -28,6 +28,7 @@ import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
+import static org.mockito.Mockito.doReturn;
 import static org.mockito.Mockito.mock;
 
 import android.net.Network;
@@ -406,6 +407,31 @@ public class MdnsResponseDecoderTests {
                 response.getInet4AddressRecord().getInet4Address());
     }
 
+    @Test
+    public void testDecodeWithIpv4AddressRemove() throws IOException {
+        MdnsResponse response = makeMdnsResponse(0, DATAIN_SERVICE_NAME_1, List.of(
+                new PacketAndRecordClass(DATAIN_PTR_1,
+                        MdnsPointerRecord.class),
+                new PacketAndRecordClass(DATAIN_SERVICE_1,
+                        MdnsServiceRecord.class),
+                new PacketAndRecordClass(DATAIN_IPV4_1,
+                        MdnsInet4AddressRecord.class),
+                new PacketAndRecordClass(DATAIN_IPV4_2,
+                        MdnsInet4AddressRecord.class)));
+        // Now update the response removing the second v4 address
+        final MdnsResponseDecoder decoder = new MdnsResponseDecoder(mClock, null);
+
+        final byte[] removedAddrResponse = makeResponsePacket(
+                List.of(DATAIN_PTR_1, DATAIN_SERVICE_1, DATAIN_IPV4_1));
+        final ArraySet<MdnsResponse> changes = decode(
+                decoder, removedAddrResponse, List.of(response));
+
+        assertEquals(1, changes.size());
+        assertEquals(1, changes.valueAt(0).getInet4AddressRecords().size());
+        assertEquals(parseNumericAddress("10.1.2.3"),
+                changes.valueAt(0).getInet4AddressRecords().get(0).getInet4Address());
+    }
+
     @Test
     public void testDecodeWithIpv6AddressChange() throws IOException {
         MdnsResponse response = makeMdnsResponse(0, DATAIN_SERVICE_NAME_1, List.of(
@@ -426,6 +452,29 @@ public class MdnsResponseDecoderTests {
                 response.getInet6AddressRecord().getInet6Address());
     }
 
+    @Test
+    public void testDecodeWithIpv6AddressRemoved() throws IOException {
+        MdnsResponse response = makeMdnsResponse(0, DATAIN_SERVICE_NAME_1, List.of(
+                new PacketAndRecordClass(DATAIN_PTR_1,
+                        MdnsPointerRecord.class),
+                new PacketAndRecordClass(DATAIN_SERVICE_1,
+                        MdnsServiceRecord.class),
+                new PacketAndRecordClass(DATAIN_IPV6_1,
+                        MdnsInet6AddressRecord.class),
+                new PacketAndRecordClass(DATAIN_IPV6_2,
+                        MdnsInet6AddressRecord.class)));
+        // Now update the response adding an address
+        final MdnsResponseDecoder decoder = new MdnsResponseDecoder(mClock, null);
+        final byte[] removedAddrResponse = makeResponsePacket(
+                List.of(DATAIN_PTR_1, DATAIN_SERVICE_1, DATAIN_IPV6_1));
+        final ArraySet<MdnsResponse> updatedResponses = decode(
+                decoder, removedAddrResponse, List.of(response));
+        assertEquals(1, updatedResponses.size());
+        assertEquals(1, updatedResponses.valueAt(0).getInet6AddressRecords().size());
+        assertEquals(parseNumericAddress("aabb:ccdd:1122:3344:a0b0:c0d0:1020:3040"),
+                updatedResponses.valueAt(0).getInet6AddressRecords().get(0).getInet6Address());
+    }
+
     @Test
     public void testDecodeWithChangeOnText() throws IOException {
         MdnsResponse response = makeMdnsResponse(0, DATAIN_SERVICE_NAME_1, List.of(
@@ -493,11 +542,12 @@ public class MdnsResponseDecoderTests {
                         new PacketAndRecordClass(DATAIN_IPV4_1, MdnsInet4AddressRecord.class),
                         new PacketAndRecordClass(DATAIN_IPV6_1, MdnsInet6AddressRecord.class),
                         new PacketAndRecordClass(DATAIN_PTR_1, MdnsPointerRecord.class),
-                        new PacketAndRecordClass(DATAIN_SERVICE_2, MdnsServiceRecord.class),
+                        new PacketAndRecordClass(DATAIN_SERVICE_1, MdnsServiceRecord.class),
                         new PacketAndRecordClass(DATAIN_TEXT_1, MdnsTextRecord.class));
         // Create a two identical responses.
-        MdnsResponse response = makeMdnsResponse(0, DATAIN_SERVICE_NAME_1, recordList);
+        MdnsResponse response = makeMdnsResponse(12L /* time */, DATAIN_SERVICE_NAME_1, recordList);
 
+        doReturn(34L).when(mClock).elapsedRealtime();
         final MdnsResponseDecoder decoder = new MdnsResponseDecoder(mClock, null);
         final byte[] identicalResponse = makeResponsePacket(
                 recordList.stream().map(p -> p.packetData).collect(Collectors.toList()));

tests/unit/java/com/android/server/connectivity/mdns/MdnsServiceTypeClientTests.java

@@ -1078,21 +1078,17 @@ public class MdnsServiceTypeClientTests {
                 0 /* flags */,
                 Collections.emptyList() /* questions */,
                 List.of(
-                        // TODO: cacheFlush will cause addresses to be cleared and re-added every
-                        //  time, which is considered a change and triggers extra
-                        //  onServiceChanged callbacks. Sets cacheFlush bit to false until the
-                        //  issue is fixed.
                         new MdnsServiceRecord(serviceName, updatedReceiptTime,
-                                false /* cacheFlush */, TEST_TTL, 0 /* servicePriority */,
+                                true /* cacheFlush */, TEST_TTL, 0 /* servicePriority */,
                                 0 /* serviceWeight */, 1234 /* servicePort */, hostname),
                         new MdnsTextRecord(serviceName, updatedReceiptTime,
-                                false /* cacheFlush */, TEST_TTL,
+                                true /* cacheFlush */, TEST_TTL,
                                 Collections.emptyList() /* entries */),
                         new MdnsInetAddressRecord(hostname, updatedReceiptTime,
-                                false /* cacheFlush */, TEST_TTL,
+                                true /* cacheFlush */, TEST_TTL,
                                 InetAddresses.parseNumericAddress(ipV4Address)),
                         new MdnsInetAddressRecord(hostname, updatedReceiptTime,
-                                false /* cacheFlush */, TEST_TTL,
+                                true /* cacheFlush */, TEST_TTL,
                                 InetAddresses.parseNumericAddress(ipV6Address))),
                 Collections.emptyList() /* authorityRecords */,
                 Collections.emptyList() /* additionalRecords */);