From 98a44bcb29b0ce3ae6bba0368c7f7748b2d8ebb5 Mon Sep 17 00:00:00 2001 From: Remi NGUYEN VAN Date: Thu, 8 Jun 2023 13:27:14 +0900 Subject: [PATCH] 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. Bug: 285997766 Bug: 285084489 Test: atest CtsNetTest FrameworksNetTests Change-Id: Ic0f19adf5d9bde7cdab766e49cf677b319a2219b --- .../connectivity/mdns/MdnsResponse.java | 8 +++ .../mdns/MdnsResponseDecoder.java | 30 +++++++++-- .../mdns/MdnsResponseDecoderTests.java | 54 ++++++++++++++++++- .../mdns/MdnsServiceTypeClientTests.java | 12 ++--- 4 files changed, 90 insertions(+), 14 deletions(-) diff --git a/service-t/src/com/android/server/connectivity/mdns/MdnsResponse.java b/service-t/src/com/android/server/connectivity/mdns/MdnsResponse.java index ec1e4627fe..e2288c1824 100644 --- a/service-t/src/com/android/server/connectivity/mdns/MdnsResponse.java +++ b/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. * diff --git a/service-t/src/com/android/server/connectivity/mdns/MdnsResponseDecoder.java b/service-t/src/com/android/server/connectivity/mdns/MdnsResponseDecoder.java index 42f610771a..eff18804c5 100644 --- a/service-t/src/com/android/server/connectivity/mdns/MdnsResponseDecoder.java +++ b/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; @@ -140,8 +141,11 @@ public class MdnsResponseDecoder { final ArraySet modified = new ArraySet<>(); final ArrayList responses = new ArrayList<>(existingResponses.size()); + final ArrayMap 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: @@ -167,7 +171,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(); @@ -260,7 +264,11 @@ public class MdnsResponseDecoder { findResponsesWithHostName(responses, inetRecord.getName()); for (MdnsResponse response : matchingResponses) { if (assignInetRecord(response, inetRecord)) { - modified.add(response); + final MdnsResponse originalResponse = augmentedToOriginal.get(response); + if (originalResponse == null + || !originalResponse.hasIdenticalRecord(inetRecord)) { + modified.add(response); + } } } } else { @@ -268,12 +276,26 @@ public class MdnsResponseDecoder { findResponseWithHostName(responses, inetRecord.getName()); if (response != null) { if (assignInetRecord(response, inetRecord)) { - modified.add(response); + 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); } diff --git a/tests/unit/java/com/android/server/connectivity/mdns/MdnsResponseDecoderTests.java b/tests/unit/java/com/android/server/connectivity/mdns/MdnsResponseDecoderTests.java index 6eb83da639..05eca844a0 100644 --- a/tests/unit/java/com/android/server/connectivity/mdns/MdnsResponseDecoderTests.java +++ b/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; @@ -417,6 +418,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 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( @@ -437,6 +463,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 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( @@ -504,11 +553,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())); 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 a696150cff..b9fdf184da 100644 --- a/tests/unit/java/com/android/server/connectivity/mdns/MdnsServiceTypeClientTests.java +++ b/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 */);