From 0c374445dc94db91d8fa2a68ee67a13adf3a741a Mon Sep 17 00:00:00 2001 From: Kangping Dong Date: Mon, 20 Nov 2023 15:42:18 +0800 Subject: [PATCH 1/3] [mdns] fix duplicated records for multiple questions Existing MdnsRecordRepositiry#getReply will result in duplicated RRs in answer and additional records sections if multiple PTR questions for the same service are received in a single query message. This commit fixes this by using a Set<> for maintaining the RR set and moving the NSEC construction to outer of the addReplyFromService() after all answer and additional records have been added. This commit also refactors the unit tests to cover more cases of multiple queries other than PTR records Bug: 265095929 Change-Id: I379dd89703915406c5700ed19aad46ef4035b917 --- .../mdns/MdnsRecordRepository.java | 57 ++-- .../mdns/MdnsRecordRepositoryTest.kt | 298 +++++++++++++----- 2 files changed, 249 insertions(+), 106 deletions(-) diff --git a/service-t/src/com/android/server/connectivity/mdns/MdnsRecordRepository.java b/service-t/src/com/android/server/connectivity/mdns/MdnsRecordRepository.java index d46a7b5474..3a46e5f6d3 100644 --- a/service-t/src/com/android/server/connectivity/mdns/MdnsRecordRepository.java +++ b/service-t/src/com/android/server/connectivity/mdns/MdnsRecordRepository.java @@ -46,6 +46,7 @@ import java.util.Arrays; import java.util.Collections; import java.util.Enumeration; import java.util.Iterator; +import java.util.LinkedHashSet; import java.util.List; import java.util.Map; import java.util.Objects; @@ -497,13 +498,17 @@ public class MdnsRecordRepository { public MdnsReplyInfo getReply(MdnsPacket packet, InetSocketAddress src) { final long now = SystemClock.elapsedRealtime(); final boolean replyUnicast = (packet.flags & MdnsConstants.QCLASS_UNICAST) != 0; - final ArrayList additionalAnswerRecords = new ArrayList<>(); - final ArrayList> answerInfo = new ArrayList<>(); + + // Use LinkedHashSet for preserving the insert order of the RRs, so that RRs of the same + // service or host are grouped together (which is more developer-friendly). + final Set> answerInfo = new LinkedHashSet<>(); + final Set> additionalAnswerInfo = new LinkedHashSet<>(); + for (MdnsRecord question : packet.questions) { // Add answers from general records addReplyFromService(question, mGeneralRecords, null /* servicePtrRecord */, null /* serviceSrvRecord */, null /* serviceTxtRecord */, replyUnicast, now, - answerInfo, additionalAnswerRecords, Collections.emptyList()); + answerInfo, additionalAnswerInfo, Collections.emptyList()); // Add answers from each service for (int i = 0; i < mServices.size(); i++) { @@ -511,13 +516,33 @@ public class MdnsRecordRepository { if (registration.exiting || registration.isProbing) continue; if (addReplyFromService(question, registration.allRecords, registration.ptrRecords, registration.srvRecord, registration.txtRecord, replyUnicast, now, - answerInfo, additionalAnswerRecords, packet.answers)) { + answerInfo, additionalAnswerInfo, packet.answers)) { registration.repliedServiceCount++; registration.sentPacketCount++; } } } + // If any record was already in the answer section, remove it from the additional answer + // section. This can typically happen when there are both queries for + // SRV / TXT / A / AAAA and PTR (which can cause SRV / TXT / A / AAAA records being added + // to the additional answer section). + additionalAnswerInfo.removeAll(answerInfo); + + final List additionalAnswerRecords = + new ArrayList<>(additionalAnswerInfo.size()); + for (RecordInfo info : additionalAnswerInfo) { + additionalAnswerRecords.add(info.record); + } + + // RFC6762 6.1: negative responses + // "On receipt of a question for a particular name, rrtype, and rrclass, for which a + // responder does have one or more unique answers, the responder MAY also include an NSEC + // record in the Additional Record Section indicating the nonexistence of other rrtypes + // for that name and rrclass." + addNsecRecordsForUniqueNames(additionalAnswerRecords, + answerInfo.iterator(), additionalAnswerInfo.iterator()); + if (answerInfo.size() == 0 && additionalAnswerRecords.size() == 0) { return null; } @@ -581,15 +606,15 @@ public class MdnsRecordRepository { @Nullable List> servicePtrRecords, @Nullable RecordInfo serviceSrvRecord, @Nullable RecordInfo serviceTxtRecord, - boolean replyUnicast, long now, @NonNull List> answerInfo, - @NonNull List additionalAnswerRecords, + boolean replyUnicast, long now, @NonNull Set> answerInfo, + @NonNull Set> additionalAnswerInfo, @NonNull List knownAnswerRecords) { boolean hasDnsSdPtrRecordAnswer = false; boolean hasDnsSdSrvRecordAnswer = false; boolean hasFullyOwnedNameMatch = false; boolean hasKnownAnswer = false; - final int answersStartIndex = answerInfo.size(); + final int answersStartSize = answerInfo.size(); for (RecordInfo info : serviceRecords) { /* RFC6762 6.: the record name must match the question name, the record rrtype @@ -645,7 +670,7 @@ public class MdnsRecordRepository { // ownership, for a type for which that name has no records, the responder MUST [...] // respond asserting the nonexistence of that record" if (hasFullyOwnedNameMatch && !hasKnownAnswer) { - additionalAnswerRecords.add(new MdnsNsecRecord( + MdnsNsecRecord nsecRecord = new MdnsNsecRecord( question.getName(), 0L /* receiptTimeMillis */, true /* cacheFlush */, @@ -653,13 +678,14 @@ public class MdnsRecordRepository { // be the same as the TTL that the record would have had, had it existed." NAME_RECORDS_TTL_MILLIS, question.getName(), - new int[] { question.getType() })); + new int[] { question.getType() }); + additionalAnswerInfo.add( + new RecordInfo<>(null /* serviceInfo */, nsecRecord, false /* isSharedName */)); } // No more records to add if no answer - if (answerInfo.size() == answersStartIndex) return false; + if (answerInfo.size() == answersStartSize) return false; - final List> additionalAnswerInfo = new ArrayList<>(); // RFC6763 12.1: if including PTR record, include the SRV and TXT records it names if (hasDnsSdPtrRecordAnswer) { if (serviceTxtRecord != null) { @@ -678,15 +704,6 @@ public class MdnsRecordRepository { } } } - - for (RecordInfo info : additionalAnswerInfo) { - additionalAnswerRecords.add(info.record); - } - - // RFC6762 6.1: negative responses - addNsecRecordsForUniqueNames(additionalAnswerRecords, - answerInfo.listIterator(answersStartIndex), - additionalAnswerInfo.listIterator()); return true; } diff --git a/tests/unit/java/com/android/server/connectivity/mdns/MdnsRecordRepositoryTest.kt b/tests/unit/java/com/android/server/connectivity/mdns/MdnsRecordRepositoryTest.kt index 196f73ff22..dc5507c095 100644 --- a/tests/unit/java/com/android/server/connectivity/mdns/MdnsRecordRepositoryTest.kt +++ b/tests/unit/java/com/android/server/connectivity/mdns/MdnsRecordRepositoryTest.kt @@ -22,6 +22,12 @@ import android.net.nsd.NsdServiceInfo import android.os.Build import android.os.HandlerThread import com.android.server.connectivity.mdns.MdnsAnnouncer.AnnouncementInfo +import com.android.server.connectivity.mdns.MdnsRecord.TYPE_A +import com.android.server.connectivity.mdns.MdnsRecord.TYPE_AAAA +import com.android.server.connectivity.mdns.MdnsRecord.TYPE_ANY +import com.android.server.connectivity.mdns.MdnsRecord.TYPE_PTR +import com.android.server.connectivity.mdns.MdnsRecord.TYPE_SRV +import com.android.server.connectivity.mdns.MdnsRecord.TYPE_TXT import com.android.server.connectivity.mdns.MdnsRecordRepository.Dependencies import com.android.server.connectivity.mdns.MdnsRecordRepository.getReverseDnsAddress import com.android.server.connectivity.mdns.MdnsServiceInfo.TextEntry @@ -38,6 +44,7 @@ import kotlin.test.assertFalse import kotlin.test.assertNotNull import kotlin.test.assertNull import kotlin.test.assertTrue +import kotlin.test.fail import org.junit.After import org.junit.Before import org.junit.Test @@ -398,31 +405,31 @@ class MdnsRecordRepositoryTest { true /* cacheFlush */, 120000L /* ttlMillis */, v4AddrRev, - intArrayOf(MdnsRecord.TYPE_PTR)), + intArrayOf(TYPE_PTR)), MdnsNsecRecord(TEST_HOSTNAME, 0L /* receiptTimeMillis */, true /* cacheFlush */, 120000L /* ttlMillis */, TEST_HOSTNAME, - intArrayOf(MdnsRecord.TYPE_A, MdnsRecord.TYPE_AAAA)), + intArrayOf(TYPE_A, TYPE_AAAA)), MdnsNsecRecord(v6Addr1Rev, 0L /* receiptTimeMillis */, true /* cacheFlush */, 120000L /* ttlMillis */, v6Addr1Rev, - intArrayOf(MdnsRecord.TYPE_PTR)), + intArrayOf(TYPE_PTR)), MdnsNsecRecord(v6Addr2Rev, 0L /* receiptTimeMillis */, true /* cacheFlush */, 120000L /* ttlMillis */, v6Addr2Rev, - intArrayOf(MdnsRecord.TYPE_PTR)), + intArrayOf(TYPE_PTR)), MdnsNsecRecord(serviceName, 0L /* receiptTimeMillis */, true /* cacheFlush */, 4500000L /* ttlMillis */, serviceName, - intArrayOf(MdnsRecord.TYPE_TXT, MdnsRecord.TYPE_SRV)) + intArrayOf(TYPE_TXT, TYPE_SRV)) ), packet.additionalRecords) } @@ -504,94 +511,212 @@ class MdnsRecordRepositoryTest { assertEquals(7, replyCaseInsensitive.additionalAnswers.size) } - @Test - fun testGetReply() { - doGetReplyTest(queryWithSubtype = false) - } - - @Test - fun testGetReply_WithSubtype() { - doGetReplyTest(queryWithSubtype = true) - } - - private fun doGetReplyTest(queryWithSubtype: Boolean) { - val repository = MdnsRecordRepository(thread.looper, deps, TEST_HOSTNAME, flags) - repository.initWithService(TEST_SERVICE_ID_1, TEST_SERVICE_1, - setOf(TEST_SUBTYPE, TEST_SUBTYPE2)) - val queriedName = if (!queryWithSubtype) arrayOf("_testservice", "_tcp", "local") - else arrayOf(TEST_SUBTYPE, "_sub", "_testservice", "_tcp", "local") - - val questions = listOf(MdnsPointerRecord(queriedName, false /* isUnicast */)) - val query = MdnsPacket(0 /* flags */, questions, listOf() /* answers */, + /** + * Creates mDNS query packet with given query names and types. + */ + private fun makeQuery(vararg queries: Pair>): MdnsPacket { + val questions = queries.map { (type, name) -> makeQuestionRecord(name, type) } + return MdnsPacket(0 /* flags */, questions, listOf() /* answers */, listOf() /* authorityRecords */, listOf() /* additionalRecords */) + } + + private fun makeQuestionRecord(name: Array, type: Int): MdnsRecord { + when (type) { + TYPE_PTR -> return MdnsPointerRecord(name, false /* isUnicast */) + TYPE_SRV -> return MdnsServiceRecord(name, false /* isUnicast */) + TYPE_TXT -> return MdnsTextRecord(name, false /* isUnicast */) + TYPE_A, TYPE_AAAA -> return MdnsInetAddressRecord(name, type, false /* isUnicast */) + else -> fail("Unexpected question type: $type") + } + } + + @Test + fun testGetReply_singlePtrQuestion_returnsSrvTxtAddressNsecRecords() { + val repository = MdnsRecordRepository(thread.looper, deps, TEST_HOSTNAME, flags) + repository.initWithService(TEST_SERVICE_ID_1, TEST_SERVICE_1, setOf(TEST_SUBTYPE)) val src = InetSocketAddress(parseNumericAddress("192.0.2.123"), 5353) + val serviceName = arrayOf("MyTestService", "_testservice", "_tcp", "local") + + val query = makeQuery(TYPE_PTR to arrayOf("_testservice", "_tcp", "local")) val reply = repository.getReply(query, src) assertNotNull(reply) - // Source address is IPv4 - assertEquals(MdnsConstants.getMdnsIPv4Address(), reply.destination.address) - assertEquals(MdnsConstants.MDNS_PORT, reply.destination.port) - - val serviceName = arrayOf("MyTestService", "_testservice", "_tcp", "local") - assertEquals(listOf( MdnsPointerRecord( - queriedName, - 0L /* receiptTimeMillis */, - false /* cacheFlush */, - LONG_TTL, - serviceName), - ), reply.answers) - + arrayOf("_testservice", "_tcp", "local"), 0L, false, LONG_TTL, serviceName)), + reply.answers) assertEquals(listOf( - MdnsTextRecord( - serviceName, - 0L /* receiptTimeMillis */, - true /* cacheFlush */, - LONG_TTL, - listOf() /* entries */), - MdnsServiceRecord( - serviceName, - 0L /* receiptTimeMillis */, - true /* cacheFlush */, - SHORT_TTL, - 0 /* servicePriority */, - 0 /* serviceWeight */, - TEST_PORT, - TEST_HOSTNAME), - MdnsInetAddressRecord( - TEST_HOSTNAME, - 0L /* receiptTimeMillis */, - true /* cacheFlush */, - SHORT_TTL, - TEST_ADDRESSES[0].address), - MdnsInetAddressRecord( - TEST_HOSTNAME, - 0L /* receiptTimeMillis */, - true /* cacheFlush */, - SHORT_TTL, - TEST_ADDRESSES[1].address), - MdnsInetAddressRecord( - TEST_HOSTNAME, - 0L /* receiptTimeMillis */, - true /* cacheFlush */, - SHORT_TTL, - TEST_ADDRESSES[2].address), - MdnsNsecRecord( - serviceName, - 0L /* receiptTimeMillis */, - true /* cacheFlush */, - LONG_TTL, - serviceName /* nextDomain */, - intArrayOf(MdnsRecord.TYPE_TXT, MdnsRecord.TYPE_SRV)), - MdnsNsecRecord( - TEST_HOSTNAME, - 0L /* receiptTimeMillis */, - true /* cacheFlush */, - SHORT_TTL, - TEST_HOSTNAME /* nextDomain */, - intArrayOf(MdnsRecord.TYPE_A, MdnsRecord.TYPE_AAAA)), - ), reply.additionalAnswers) + MdnsTextRecord(serviceName, 0L, true, LONG_TTL, listOf()), + MdnsServiceRecord(serviceName, 0L, true, SHORT_TTL, 0, 0, TEST_PORT, TEST_HOSTNAME), + MdnsInetAddressRecord( + TEST_HOSTNAME, 0L, true, SHORT_TTL, TEST_ADDRESSES[0].address), + MdnsInetAddressRecord( + TEST_HOSTNAME, 0L, true, SHORT_TTL, TEST_ADDRESSES[1].address), + MdnsInetAddressRecord( + TEST_HOSTNAME, 0L, true, SHORT_TTL, TEST_ADDRESSES[2].address), + MdnsNsecRecord(serviceName, 0L, true, LONG_TTL, serviceName /* nextDomain */, + intArrayOf(TYPE_TXT, TYPE_SRV)), + MdnsNsecRecord(TEST_HOSTNAME, 0L, true, SHORT_TTL, TEST_HOSTNAME /* nextDomain */, + intArrayOf(TYPE_A, TYPE_AAAA)), + ), reply.additionalAnswers) + } + + @Test + fun testGetReply_singleSubtypePtrQuestion_returnsSrvTxtAddressNsecRecords() { + val repository = MdnsRecordRepository(thread.looper, deps, TEST_HOSTNAME, flags) + repository.initWithService(TEST_SERVICE_ID_1, TEST_SERVICE_1, setOf(TEST_SUBTYPE)) + val src = InetSocketAddress(parseNumericAddress("192.0.2.123"), 5353) + val serviceName = arrayOf("MyTestService", "_testservice", "_tcp", "local") + + val query = makeQuery( + TYPE_PTR to arrayOf(TEST_SUBTYPE, "_sub", "_testservice", "_tcp", "local")) + val reply = repository.getReply(query, src) + + assertNotNull(reply) + assertEquals(listOf( + MdnsPointerRecord( + arrayOf(TEST_SUBTYPE, "_sub", "_testservice", "_tcp", "local"), 0L, false, + LONG_TTL, serviceName)), + reply.answers) + assertEquals(listOf( + MdnsTextRecord(serviceName, 0L, true, LONG_TTL, listOf()), + MdnsServiceRecord(serviceName, 0L, true, SHORT_TTL, 0, 0, TEST_PORT, TEST_HOSTNAME), + MdnsInetAddressRecord( + TEST_HOSTNAME, 0L, true, SHORT_TTL, TEST_ADDRESSES[0].address), + MdnsInetAddressRecord( + TEST_HOSTNAME, 0L, true, SHORT_TTL, TEST_ADDRESSES[1].address), + MdnsInetAddressRecord( + TEST_HOSTNAME, 0L, true, SHORT_TTL, TEST_ADDRESSES[2].address), + MdnsNsecRecord(serviceName, 0L, true, LONG_TTL, serviceName /* nextDomain */, + intArrayOf(TYPE_TXT, TYPE_SRV)), + MdnsNsecRecord(TEST_HOSTNAME, 0L, true, SHORT_TTL, TEST_HOSTNAME /* nextDomain */, + intArrayOf(TYPE_A, TYPE_AAAA)), + ), reply.additionalAnswers) + } + + @Test + fun testGetReply_duplicatePtrQuestions_doesNotReturnDuplicateRecords() { + val repository = MdnsRecordRepository(thread.looper, deps, TEST_HOSTNAME, flags) + repository.initWithService(TEST_SERVICE_ID_1, TEST_SERVICE_1, setOf(TEST_SUBTYPE)) + val src = InetSocketAddress(parseNumericAddress("192.0.2.123"), 5353) + val serviceName = arrayOf("MyTestService", "_testservice", "_tcp", "local") + + val query = makeQuery( + TYPE_PTR to arrayOf("_testservice", "_tcp", "local"), + TYPE_PTR to arrayOf("_testservice", "_tcp", "local")) + val reply = repository.getReply(query, src) + + assertNotNull(reply) + assertEquals(listOf( + MdnsPointerRecord( + arrayOf("_testservice", "_tcp", "local"), 0L, false, LONG_TTL, serviceName)), + reply.answers) + assertEquals(listOf( + MdnsTextRecord(serviceName, 0L, true, LONG_TTL, listOf()), + MdnsServiceRecord(serviceName, 0L, true, SHORT_TTL, 0, 0, TEST_PORT, TEST_HOSTNAME), + MdnsInetAddressRecord( + TEST_HOSTNAME, 0L, true, SHORT_TTL, TEST_ADDRESSES[0].address), + MdnsInetAddressRecord( + TEST_HOSTNAME, 0L, true, SHORT_TTL, TEST_ADDRESSES[1].address), + MdnsInetAddressRecord( + TEST_HOSTNAME, 0L, true, SHORT_TTL, TEST_ADDRESSES[2].address), + MdnsNsecRecord(serviceName, 0L, true, LONG_TTL, serviceName /* nextDomain */, + intArrayOf(TYPE_TXT, TYPE_SRV)), + MdnsNsecRecord(TEST_HOSTNAME, 0L, true, SHORT_TTL, TEST_HOSTNAME /* nextDomain */, + intArrayOf(TYPE_A, TYPE_AAAA)), + ), reply.additionalAnswers) + } + + @Test + fun testGetReply_multiplePtrQuestionsWithSubtype_doesNotReturnDuplicateRecords() { + val repository = MdnsRecordRepository(thread.looper, deps, TEST_HOSTNAME, flags) + repository.initWithService(TEST_SERVICE_ID_1, TEST_SERVICE_1, setOf(TEST_SUBTYPE)) + val src = InetSocketAddress(parseNumericAddress("192.0.2.123"), 5353) + val serviceName = arrayOf("MyTestService", "_testservice", "_tcp", "local") + + val query = makeQuery( + TYPE_PTR to arrayOf("_testservice", "_tcp", "local"), + TYPE_PTR to arrayOf(TEST_SUBTYPE, "_sub", "_testservice", "_tcp", "local")) + val reply = repository.getReply(query, src) + + assertNotNull(reply) + assertEquals(listOf( + MdnsPointerRecord( + arrayOf("_testservice", "_tcp", "local"), 0L, false, LONG_TTL, serviceName), + MdnsPointerRecord( + arrayOf(TEST_SUBTYPE, "_sub", "_testservice", "_tcp", "local"), + 0L, false, LONG_TTL, serviceName)), + reply.answers) + assertEquals(listOf( + MdnsTextRecord(serviceName, 0L, true, LONG_TTL, listOf()), + MdnsServiceRecord(serviceName, 0L, true, SHORT_TTL, 0, 0, TEST_PORT, TEST_HOSTNAME), + MdnsInetAddressRecord( + TEST_HOSTNAME, 0L, true, SHORT_TTL, TEST_ADDRESSES[0].address), + MdnsInetAddressRecord( + TEST_HOSTNAME, 0L, true, SHORT_TTL, TEST_ADDRESSES[1].address), + MdnsInetAddressRecord( + TEST_HOSTNAME, 0L, true, SHORT_TTL, TEST_ADDRESSES[2].address), + MdnsNsecRecord(serviceName, 0L, true, LONG_TTL, serviceName /* nextDomain */, + intArrayOf(TYPE_TXT, TYPE_SRV)), + MdnsNsecRecord(TEST_HOSTNAME, 0L, true, SHORT_TTL, TEST_HOSTNAME /* nextDomain */, + intArrayOf(TYPE_A, TYPE_AAAA)), + ), reply.additionalAnswers) + } + + @Test + fun testGetReply_ptrAndSrvQuestions_doesNotReturnSrvRecordInAdditionalAnswerSection() { + val repository = MdnsRecordRepository(thread.looper, deps, TEST_HOSTNAME, flags) + repository.initWithService(TEST_SERVICE_ID_1, TEST_SERVICE_1, setOf(TEST_SUBTYPE)) + val src = InetSocketAddress(parseNumericAddress("192.0.2.123"), 5353) + val serviceName = arrayOf("MyTestService", "_testservice", "_tcp", "local") + + val query = makeQuery( + TYPE_PTR to arrayOf("_testservice", "_tcp", "local"), + TYPE_SRV to serviceName) + val reply = repository.getReply(query, src) + + assertNotNull(reply) + assertEquals(listOf( + MdnsPointerRecord( + arrayOf("_testservice", "_tcp", "local"), 0L, false, LONG_TTL, serviceName), + MdnsServiceRecord( + serviceName, 0L, true, SHORT_TTL, 0, 0, TEST_PORT, TEST_HOSTNAME)), + reply.answers) + assertFalse(reply.additionalAnswers.any { it -> it is MdnsServiceRecord }) + } + + @Test + fun testGetReply_srvTxtAddressQuestions_returnsAllRecordsInAnswerSectionExceptNsec() { + val repository = MdnsRecordRepository(thread.looper, deps, TEST_HOSTNAME, flags) + repository.initWithService(TEST_SERVICE_ID_1, TEST_SERVICE_1, setOf(TEST_SUBTYPE)) + val src = InetSocketAddress(parseNumericAddress("192.0.2.123"), 5353) + val serviceName = arrayOf("MyTestService", "_testservice", "_tcp", "local") + + val query = makeQuery( + TYPE_SRV to serviceName, + TYPE_TXT to serviceName, + TYPE_SRV to serviceName, + TYPE_A to TEST_HOSTNAME, + TYPE_AAAA to TEST_HOSTNAME) + val reply = repository.getReply(query, src) + + assertNotNull(reply) + assertEquals(listOf( + MdnsServiceRecord(serviceName, 0L, true, SHORT_TTL, 0, 0, TEST_PORT, TEST_HOSTNAME), + MdnsTextRecord(serviceName, 0L, true, LONG_TTL, listOf()), + MdnsInetAddressRecord( + TEST_HOSTNAME, 0L, true, SHORT_TTL, TEST_ADDRESSES[0].address), + MdnsInetAddressRecord( + TEST_HOSTNAME, 0L, true, SHORT_TTL, TEST_ADDRESSES[1].address), + MdnsInetAddressRecord( + TEST_HOSTNAME, 0L, true, SHORT_TTL, TEST_ADDRESSES[2].address)), + reply.answers) + assertEquals(listOf( + MdnsNsecRecord(serviceName, 0L, true, LONG_TTL, serviceName /* nextDomain */, + intArrayOf(TYPE_TXT, TYPE_SRV)), + MdnsNsecRecord(TEST_HOSTNAME, 0L, true, SHORT_TTL, TEST_HOSTNAME /* nextDomain */, + intArrayOf(TYPE_A, TYPE_AAAA))), + reply.additionalAnswers) } @Test @@ -1064,8 +1189,9 @@ private fun MdnsRecordRepository.initWithService( serviceId: Int, serviceInfo: NsdServiceInfo, subtypes: Set = setOf(), + addresses: List = TEST_ADDRESSES ): AnnouncementInfo { - updateAddresses(TEST_ADDRESSES) + updateAddresses(addresses) serviceInfo.setSubtypes(subtypes) addService(serviceId, serviceInfo) val probingInfo = setServiceProbing(serviceId) From 04b61c4a3c41427b412010c5adf88b6b79dafdf3 Mon Sep 17 00:00:00 2001 From: Kangping Dong Date: Mon, 11 Dec 2023 13:09:04 +0800 Subject: [PATCH 2/3] [mdns] fix wrong NSEC answer for unique RRs Existing MdnsRecordRepositiry#getReply will send a NSEC RR in the reply when 1) a query for a unique RR is not found or 2) any unique name which is included in the answer sections. The second one is not aligned with the spec: ``` On receipt of a question for a particular name, rrtype, and rrclass, for which a responder does have one or more unique answers, the responder MAY also include an NSEC record in the Additional Record Section indicating the nonexistence of other rrtypes for that name and rrclass. ``` This commit fixes the issue in 2) by checking that all unique records of the name are included in the answers before adding a NSEC record. Bug: 265095929 Test: atest FrameworksNetTests Change-Id: I0531fbccf98e69a50bb8a45e529caaed3ad80b59 --- .../mdns/MdnsRecordRepository.java | 33 ++++++++++++-- .../mdns/MdnsRecordRepositoryTest.kt | 43 ++++++++++++++++--- 2 files changed, 66 insertions(+), 10 deletions(-) diff --git a/service-t/src/com/android/server/connectivity/mdns/MdnsRecordRepository.java b/service-t/src/com/android/server/connectivity/mdns/MdnsRecordRepository.java index 3a46e5f6d3..c8fe651a22 100644 --- a/service-t/src/com/android/server/connectivity/mdns/MdnsRecordRepository.java +++ b/service-t/src/com/android/server/connectivity/mdns/MdnsRecordRepository.java @@ -720,7 +720,7 @@ public class MdnsRecordRepository { * answer and additionalAnswer sections) */ @SafeVarargs - private static void addNsecRecordsForUniqueNames( + private void addNsecRecordsForUniqueNames( List destinationList, Iterator>... answerRecords) { // Group unique records by name. Use a TreeMap with comparator as arrays don't implement @@ -736,6 +736,12 @@ public class MdnsRecordRepository { for (String[] nsecName : namesInAddedOrder) { final List entryRecords = nsecByName.get(nsecName); + + // Add NSEC records only when the answers include all unique records of this name + if (entryRecords.size() != countUniqueRecords(nsecName)) { + continue; + } + long minTtl = Long.MAX_VALUE; final Set types = new ArraySet<>(entryRecords.size()); for (MdnsRecord record : entryRecords) { @@ -753,6 +759,27 @@ public class MdnsRecordRepository { } } + /** Returns the number of unique records on this device for a given {@code name}. */ + private int countUniqueRecords(String[] name) { + int cnt = countUniqueRecords(mGeneralRecords, name); + + for (int i = 0; i < mServices.size(); i++) { + final ServiceRegistration registration = mServices.valueAt(i); + cnt += countUniqueRecords(registration.allRecords, name); + } + return cnt; + } + + private static int countUniqueRecords(List> records, String[] name) { + int cnt = 0; + for (RecordInfo record : records) { + if (!record.isSharedName && Arrays.equals(name, record.record.getName())) { + cnt++; + } + } + return cnt; + } + /** * Add non-shared records to a map listing them by record name, and to a list of names that * remembers the adding order. @@ -767,10 +794,10 @@ public class MdnsRecordRepository { private static void addNonSharedRecordsToMap( Iterator> records, Map> dest, - List namesInAddedOrder) { + @Nullable List namesInAddedOrder) { while (records.hasNext()) { final RecordInfo record = records.next(); - if (record.isSharedName) continue; + if (record.isSharedName || record.record instanceof MdnsNsecRecord) continue; final List recordsForName = dest.computeIfAbsent(record.record.name, key -> { namesInAddedOrder.add(key); diff --git a/tests/unit/java/com/android/server/connectivity/mdns/MdnsRecordRepositoryTest.kt b/tests/unit/java/com/android/server/connectivity/mdns/MdnsRecordRepositoryTest.kt index dc5507c095..69f039c154 100644 --- a/tests/unit/java/com/android/server/connectivity/mdns/MdnsRecordRepositoryTest.kt +++ b/tests/unit/java/com/android/server/connectivity/mdns/MdnsRecordRepositoryTest.kt @@ -663,6 +663,42 @@ class MdnsRecordRepositoryTest { ), reply.additionalAnswers) } + @Test + fun testGetReply_txtQuestion_returnsNoNsecRecord() { + val repository = MdnsRecordRepository(thread.looper, deps, TEST_HOSTNAME, flags) + repository.initWithService(TEST_SERVICE_ID_1, TEST_SERVICE_1, setOf(TEST_SUBTYPE)) + val src = InetSocketAddress(parseNumericAddress("192.0.2.123"), 5353) + val serviceName = arrayOf("MyTestService", "_testservice", "_tcp", "local") + + val query = makeQuery(TYPE_TXT to serviceName) + val reply = repository.getReply(query, src) + + assertNotNull(reply) + assertEquals(listOf(MdnsTextRecord(serviceName, 0L, true, LONG_TTL, listOf())), + reply.answers) + // No NSEC records because the reply doesn't include the SRV record + assertTrue(reply.additionalAnswers.isEmpty()) + } + + @Test + fun testGetReply_AAAAQuestionButNoIpv6Address_returnsNsecRecord() { + val repository = MdnsRecordRepository(thread.looper, deps, TEST_HOSTNAME, flags) + repository.initWithService( + TEST_SERVICE_ID_1, TEST_SERVICE_1, setOf(TEST_SUBTYPE), + listOf(LinkAddress(parseNumericAddress("192.0.2.111"), 24))) + val src = InetSocketAddress(parseNumericAddress("192.0.2.123"), 5353) + + val query = makeQuery(TYPE_AAAA to TEST_HOSTNAME) + val reply = repository.getReply(query, src) + + assertNotNull(reply) + assertTrue(reply.answers.isEmpty()) + assertEquals(listOf( + MdnsNsecRecord(TEST_HOSTNAME, 0L, true, LONG_TTL, TEST_HOSTNAME /* nextDomain */, + intArrayOf(TYPE_AAAA))), + reply.additionalAnswers) + } + @Test fun testGetReply_ptrAndSrvQuestions_doesNotReturnSrvRecordInAdditionalAnswerSection() { val repository = MdnsRecordRepository(thread.looper, deps, TEST_HOSTNAME, flags) @@ -1139,13 +1175,6 @@ class MdnsRecordRepositoryTest { true /* cacheFlush */, SHORT_TTL, TEST_ADDRESSES[2].address), - MdnsNsecRecord( - serviceName, - 0L /* receiptTimeMillis */, - true /* cacheFlush */, - LONG_TTL, - serviceName /* nextDomain */, - intArrayOf(MdnsRecord.TYPE_SRV)), MdnsNsecRecord( TEST_HOSTNAME, 0L /* receiptTimeMillis */, From b83b90e779d7ab06aa99ba191769ae2c562b4405 Mon Sep 17 00:00:00 2001 From: Kangping Dong Date: Mon, 11 Dec 2023 13:21:47 +0800 Subject: [PATCH 3/3] [mdns] add tests for verifying address of mDNS reply Bug: 265095929 Test: atest FrameworksNetTests Change-Id: I3499fb9dfc24fec9648f9c97d6028c3fca4f925a --- .../mdns/MdnsRecordRepository.java | 3 ++ .../mdns/MdnsRecordRepositoryTest.kt | 28 +++++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/service-t/src/com/android/server/connectivity/mdns/MdnsRecordRepository.java b/service-t/src/com/android/server/connectivity/mdns/MdnsRecordRepository.java index c8fe651a22..f9951a966a 100644 --- a/service-t/src/com/android/server/connectivity/mdns/MdnsRecordRepository.java +++ b/service-t/src/com/android/server/connectivity/mdns/MdnsRecordRepository.java @@ -141,6 +141,9 @@ public class MdnsRecordRepository { * Last time (as per SystemClock.elapsedRealtime) when sent via unicast or multicast, * 0 if never */ + // FIXME: the `lastSentTimeMs` and `lastAdvertisedTimeMs` should be maintained separately + // for IPv4 and IPv6, because neither IPv4 nor and IPv6 clients can receive replies in + // different address space. public long lastSentTimeMs; RecordInfo(NsdServiceInfo serviceInfo, T record, boolean sharedName) { diff --git a/tests/unit/java/com/android/server/connectivity/mdns/MdnsRecordRepositoryTest.kt b/tests/unit/java/com/android/server/connectivity/mdns/MdnsRecordRepositoryTest.kt index 69f039c154..4b1f16690f 100644 --- a/tests/unit/java/com/android/server/connectivity/mdns/MdnsRecordRepositoryTest.kt +++ b/tests/unit/java/com/android/server/connectivity/mdns/MdnsRecordRepositoryTest.kt @@ -755,6 +755,34 @@ class MdnsRecordRepositoryTest { reply.additionalAnswers) } + @Test + fun testGetReply_queryWithIpv4Address_replyWithIpv4Address() { + val repository = MdnsRecordRepository(thread.looper, deps, TEST_HOSTNAME, flags) + repository.initWithService(TEST_SERVICE_ID_1, TEST_SERVICE_1, setOf(TEST_SUBTYPE)) + val query = makeQuery(TYPE_PTR to arrayOf("_testservice", "_tcp", "local")) + + val srcIpv4 = InetSocketAddress(parseNumericAddress("192.0.2.123"), 5353) + val replyIpv4 = repository.getReply(query, srcIpv4) + + assertNotNull(replyIpv4) + assertEquals(MdnsConstants.getMdnsIPv4Address(), replyIpv4.destination.address) + assertEquals(MdnsConstants.MDNS_PORT, replyIpv4.destination.port) + } + + @Test + fun testGetReply_queryWithIpv6Address_replyWithIpv6Address() { + val repository = MdnsRecordRepository(thread.looper, deps, TEST_HOSTNAME, flags) + repository.initWithService(TEST_SERVICE_ID_1, TEST_SERVICE_1, setOf(TEST_SUBTYPE)) + val query = makeQuery(TYPE_PTR to arrayOf("_testservice", "_tcp", "local")) + + val srcIpv6 = InetSocketAddress(parseNumericAddress("2001:db8::123"), 5353) + val replyIpv6 = repository.getReply(query, srcIpv6) + + assertNotNull(replyIpv6) + assertEquals(MdnsConstants.getMdnsIPv6Address(), replyIpv6.destination.address) + assertEquals(MdnsConstants.MDNS_PORT, replyIpv6.destination.port) + } + @Test fun testGetConflictingServices() { val repository = MdnsRecordRepository(thread.looper, deps, TEST_HOSTNAME, flags)