From 7078ac59b9108979a2f598b77e8cfe8058ea7817 Mon Sep 17 00:00:00 2001 From: Remi NGUYEN VAN Date: Thu, 28 Dec 2023 18:01:52 +0900 Subject: [PATCH] Only use source addresses in handlePacket MulticastPacketReader uses an InetSocketAddress as a buffer that gets filled for each incoming packet for the source address. Similarly to the receive buffer, it should not be used outside of the handlePacket method, as it will be modified on the handler thread when further packets are received. Fix the current usage in MdnsInterfaceAdvertiser/MdnsRecordRepository. Fortunately due to a bug packets are never replied unicast, so today the MdnsRecordRepository code using the source address never triggers, even though it may be circulating an incorrect source address instance that changes over time. Test: atest Change-Id: I4cadfa6e54d1b37d9a6b0f8ca0269193a229bc51 --- .../server/connectivity/mdns/MdnsInterfaceAdvertiser.java | 6 ++++-- .../server/connectivity/mdns/MulticastPacketReader.java | 6 ++++++ .../connectivity/mdns/MdnsInterfaceAdvertiserTest.kt | 8 +++++++- 3 files changed, 17 insertions(+), 3 deletions(-) diff --git a/service-t/src/com/android/server/connectivity/mdns/MdnsInterfaceAdvertiser.java b/service-t/src/com/android/server/connectivity/mdns/MdnsInterfaceAdvertiser.java index aa40c9227d..3a04dcdb49 100644 --- a/service-t/src/com/android/server/connectivity/mdns/MdnsInterfaceAdvertiser.java +++ b/service-t/src/com/android/server/connectivity/mdns/MdnsInterfaceAdvertiser.java @@ -373,12 +373,14 @@ public class MdnsInterfaceAdvertiser implements MulticastPacketReader.PacketHand } return; } + // recvbuf and src are reused after this returns; ensure references to src are not kept. + final InetSocketAddress srcCopy = new InetSocketAddress(src.getAddress(), src.getPort()); if (DBG) { mSharedLog.v("Parsed packet with " + packet.questions.size() + " questions, " + packet.answers.size() + " answers, " + packet.authorityRecords.size() + " authority, " - + packet.additionalRecords.size() + " additional from " + src); + + packet.additionalRecords.size() + " additional from " + srcCopy); } for (int conflictServiceId : mRecordRepository.getConflictingServices(packet)) { @@ -389,7 +391,7 @@ public class MdnsInterfaceAdvertiser implements MulticastPacketReader.PacketHand // happen when the incoming packet has answer records (not a question), so there will be no // answer. One exception is simultaneous probe tiebreaking (rfc6762 8.2), in which case the // conflicting service is still probing and won't reply either. - final MdnsReplyInfo answers = mRecordRepository.getReply(packet, src); + final MdnsReplyInfo answers = mRecordRepository.getReply(packet, srcCopy); if (answers == null) return; mReplySender.queueReply(answers); diff --git a/service-t/src/com/android/server/connectivity/mdns/MulticastPacketReader.java b/service-t/src/com/android/server/connectivity/mdns/MulticastPacketReader.java index 3cd77a4610..70451f3d55 100644 --- a/service-t/src/com/android/server/connectivity/mdns/MulticastPacketReader.java +++ b/service-t/src/com/android/server/connectivity/mdns/MulticastPacketReader.java @@ -42,6 +42,12 @@ public class MulticastPacketReader extends FdEventsReader mPacketHandlers = MdnsUtils.newSet(); interface PacketHandler { + /** + * Handle an incoming packet. + * + * The recvbuf and src will be reused and modified after this method returns, so + * implementers must ensure that they are not accessed after handlePacket returns. + */ void handlePacket(byte[] recvbuf, int length, InetSocketAddress src); } diff --git a/tests/unit/java/com/android/server/connectivity/mdns/MdnsInterfaceAdvertiserTest.kt b/tests/unit/java/com/android/server/connectivity/mdns/MdnsInterfaceAdvertiserTest.kt index 0c04bff0d5..ee0bd1a5db 100644 --- a/tests/unit/java/com/android/server/connectivity/mdns/MdnsInterfaceAdvertiserTest.kt +++ b/tests/unit/java/com/android/server/connectivity/mdns/MdnsInterfaceAdvertiserTest.kt @@ -35,6 +35,7 @@ import com.android.testutils.waitForIdle import java.net.InetSocketAddress import kotlin.test.assertContentEquals import kotlin.test.assertEquals +import kotlin.test.assertNotSame import kotlin.test.assertTrue import org.junit.After import org.junit.Before @@ -213,7 +214,12 @@ class MdnsInterfaceAdvertiserTest { packetHandler.handlePacket(query, query.size, src) val packetCaptor = ArgumentCaptor.forClass(MdnsPacket::class.java) - verify(repository).getReply(packetCaptor.capture(), eq(src)) + val srcCaptor = ArgumentCaptor.forClass(InetSocketAddress::class.java) + verify(repository).getReply(packetCaptor.capture(), srcCaptor.capture()) + + assertEquals(src, srcCaptor.value) + assertNotSame(src, srcCaptor.value, "src will be reused by the packetHandler, references " + + "to it should not be used outside of handlePacket.") packetCaptor.value.let { assertEquals(1, it.questions.size)