From 0ccc849d2027de75122e5b187dc99bdf1cc24edf Mon Sep 17 00:00:00 2001
From: Harald Alvestrand <[email protected]>
Date: Wed, 02 Jul 2025 15:15:22 -0700
Subject: [PATCH] Harden P2PSocketTcpBase::SendBatch against errors

This change ensures that even if errors occur that cause the socket
to be deleted during packet processing, SendBatch will terminate
correctly.

Bug: 427681143
Change-Id: I41da6789c8aab44b31c00c6a7844cd86b918561c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6694889
Reviewed-by: Sergey Ulanov <[email protected]>
Commit-Queue: Harald Alvestrand <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1481925}
---

diff --git a/services/network/p2p/socket_tcp.cc b/services/network/p2p/socket_tcp.cc
index ba68bdcf..827ea38 100644
--- a/services/network/p2p/socket_tcp.cc
+++ b/services/network/p2p/socket_tcp.cc
@@ -73,13 +73,13 @@
 
 P2PSocketTcpBase::~P2PSocketTcpBase() = default;
 
-void P2PSocketTcpBase::InitAccepted(const net::IPEndPoint& remote_address,
+bool P2PSocketTcpBase::InitAccepted(const net::IPEndPoint& remote_address,
                                     std::unique_ptr<net::StreamSocket> socket) {
   DCHECK(socket);
   remote_address_.ip_address = remote_address;
   // TODO(ronghuawu): Add FakeSSLServerSocket.
   socket_ = std::move(socket);
-  DoRead();
+  return DoRead();
 }
 
 void P2PSocketTcpBase::Init(
@@ -128,15 +128,18 @@
   DCHECK_NE(result, net::ERR_IO_PENDING);
 
   if (result != net::OK) {
-    LOG(WARNING) << "Error from connecting socket, result=" << result;
+    LOG(WARNING) << "Error from connecting socket, result=" << result
+                 << ", destroying socket";
     OnError();
     return;
   }
 
-  OnOpen();
+  if (!OnOpen()) {
+    LOG(ERROR) << "Socket destroyed in OnConnected/OnOpen";
+  }
 }
 
-void P2PSocketTcpBase::OnOpen() {
+bool P2PSocketTcpBase::OnOpen() {
   // Setting socket send and receive buffer size.
   if (net::OK != socket_->SetReceiveBufferSize(kTcpRecvSocketBufferSize)) {
     LOG(WARNING) << "Failed to set socket receive buffer size to "
@@ -149,9 +152,9 @@
   }
 
   if (!DoSendSocketCreateMsg())
-    return;
+    return false;
 
-  DoRead();
+  return DoRead();
 }
 
 bool P2PSocketTcpBase::DoSendSocketCreateMsg() {
@@ -198,7 +201,7 @@
   return true;
 }
 
-void P2PSocketTcpBase::DoRead() {
+bool P2PSocketTcpBase::DoRead() {
   while (true) {
     if (!read_buffer_.get()) {
       read_buffer_ = base::MakeRefCounted<net::GrowableIOBuffer>();
@@ -214,14 +217,23 @@
     const int result = socket_->Read(
         read_buffer_.get(), read_buffer_->RemainingCapacity(),
         base::BindOnce(&P2PSocketTcp::OnRead, base::Unretained(this)));
-    if (result == net::ERR_IO_PENDING || !HandleReadResult(result))
-      return;
+    if (result == net::ERR_IO_PENDING) {
+      return true;  // not finished, but blocked
+    }
+    if (!HandleReadResult(result)) {
+      return false;  // error, socket deleted
+    }
   }
 }
 
 void P2PSocketTcpBase::OnRead(int result) {
-  if (HandleReadResult(result))
-    DoRead();
+  if (!HandleReadResult(result)) {
+    LOG(ERROR) << "OnRead/HandleReadResult reports socket destroyed";
+    return;
+  }
+  if (!DoRead()) {
+    LOG(ERROR) << "OnRead/DoRead reports socket destroyed";
+  }
 }
 
 bool P2PSocketTcpBase::OnPacket(base::span<const uint8_t> data) {
@@ -262,17 +274,17 @@
   return true;
 }
 
-void P2PSocketTcpBase::WriteOrQueue(SendBuffer& send_buffer) {
+bool P2PSocketTcpBase::WriteOrQueue(SendBuffer& send_buffer) {
   if (write_buffer_.buffer.get()) {
     write_queue_.push(send_buffer);
-    return;
+    return true;
   }
 
   write_buffer_ = send_buffer;
-  DoWrite();
+  return DoWrite();
 }
 
-void P2PSocketTcpBase::DoWrite() {
+bool P2PSocketTcpBase::DoWrite() {
   while (!write_pending_ && write_buffer_.buffer.get()) {
     int result = socket_->Write(
         write_buffer_.buffer.get(), write_buffer_.buffer->BytesRemaining(),
@@ -282,9 +294,10 @@
     if (result == net::ERR_IO_PENDING) {
       write_pending_ = true;
     } else if (!HandleWriteResult(result)) {
-      break;
+      return false;  // Error, socket is destroyed.
     }
   }
+  return true;
 }
 
 void P2PSocketTcpBase::OnWritten(int result) {
@@ -293,8 +306,13 @@
 
   write_pending_ = false;
 
-  if (HandleWriteResult(result))
-    DoWrite();
+  if (!HandleWriteResult(result)) {
+    LOG(ERROR) << "Socket destroyed in OnWritten/HandleWriteResult";
+    return;
+  }
+  if (!DoWrite()) {
+    LOG(ERROR) << "Socket destroyed in OnWritten/DoWrite";
+  }
 }
 
 bool P2PSocketTcpBase::HandleWriteResult(int result) {
@@ -373,13 +391,14 @@
     }
   }
 
-  DoSend(packet_info.destination, data, packet_info.packet_options);
-  return true;
+  return DoSend(packet_info.destination, data, packet_info.packet_options);
 }
 
 void P2PSocketTcpBase::Send(base::span<const uint8_t> data,
                             const P2PPacketInfo& packet_info) {
-  SendPacket(data, packet_info);
+  if (!SendPacket(data, packet_info)) {
+    LOG(ERROR) << "Socket destroyed while sending";
+  }
 }
 
 void P2PSocketTcpBase::SendBatch(
@@ -444,7 +463,7 @@
   return OnPacket(input.subspan(kPacketHeaderSize, packet_size));
 }
 
-void P2PSocketTcp::DoSend(const net::IPEndPoint& to,
+bool P2PSocketTcp::DoSend(const net::IPEndPoint& to,
                           base::span<const uint8_t> data,
                           const webrtc::AsyncSocketPacketOptions& options) {
   const size_t buffer_size = kPacketHeaderSize + data.size();
@@ -468,7 +487,7 @@
                              send_buffer_without_header.size(),
                              options.packet_time_params, webrtc::TimeMicros());
 
-  WriteOrQueue(send_buffer);
+  return WriteOrQueue(send_buffer);
 }
 
 // P2PSocketStunTcp
@@ -509,7 +528,7 @@
   return OnPacket(input.first(packet_size));
 }
 
-void P2PSocketStunTcp::DoSend(const net::IPEndPoint& to,
+bool P2PSocketStunTcp::DoSend(const net::IPEndPoint& to,

diff --git a/services/network/p2p/socket_tcp_unittest.cc b/services/network/p2p/socket_tcp_unittest.cc
index fcdd217..45761c14 100644
--- a/services/network/p2p/socket_tcp_unittest.cc
+++ b/services/network/p2p/socket_tcp_unittest.cc
@@ -685,4 +685,81 @@
   EXPECT_TRUE(ssl_socket_provider.ConnectDataConsumed());
 }
 
+// Verify that we can send packets using SendBatch
+TEST_F(P2PSocketTcpTest, SendAfterStunRequestWithSendBatch) {
+  // Receive packet from |dest_|.
+  std::vector<uint8_t> request_packet;
+  CreateStunRequest(&request_packet);
+
+  std::string received_data;
+  received_data.append(IntToSize(request_packet.size()));
+  received_data.append(request_packet.begin(), request_packet.end());
+
+  EXPECT_CALL(*fake_client_.get(), SendComplete(_)).Times(2);
+
+  EXPECT_CALL(*fake_client_.get(), DataReceived(_)).Times(1);
+  EXPECT_CALL(*this, SinglePacketReceptionHelper(_, SpanEq(request_packet), _));
+  socket_->AppendInputData(received_data);
+
+  webrtc::AsyncSocketPacketOptions options;
+  // Now we should be able to send any data to |dest_|.
+  std::vector<mojom::P2PSendPacketPtr> packet_batch;
+  std::vector<uint8_t> packet;
+  // CreateRandomPacket(&packet);
+  packet = {0x01, 0x02, 0x03};
+  std::string expected_data;
+  expected_data.append(IntToSize(packet.size()));
+  expected_data.append(packet.begin(), packet.end());
+  packet_batch.emplace_back(mojom::P2PSendPacket::New(
+      packet, P2PPacketInfo(dest_.ip_address, options, 0)));
+  std::vector<uint8_t> packet2;
+  // CreateRandomPacket(&packet2);
+  packet2 = {0x04, 0x05, 0x06};
+  expected_data.append(IntToSize(packet2.size()));
+  expected_data.append(packet2.begin(), packet2.end());
+  packet_batch.emplace_back(mojom::P2PSendPacket::New(
+      packet2, P2PPacketInfo(dest_.ip_address, options, 0)));
+  socket_impl_->SendBatch(std::move(packet_batch));
+
+  EXPECT_EQ(expected_data, sent_data_);
+
+  base::RunLoop().RunUntilIdle();
+}
+
+TEST_F(P2PSocketTcpTest, SendBatchWithBrokenFirstPacket) {
+  // Receive packet from |dest_|.
+  std::vector<uint8_t> request_packet;
+  CreateStunRequest(&request_packet);
+
+  std::string received_data;
+  received_data.append(IntToSize(request_packet.size()));
+  received_data.append(request_packet.begin(), request_packet.end());
+  EXPECT_CALL(*fake_client_.get(), DataReceived(_)).Times(1);
+  EXPECT_CALL(*this, SinglePacketReceptionHelper(_, SpanEq(request_packet), _));
+
+  socket_->AppendInputData(received_data);
+
+  webrtc::AsyncSocketPacketOptions options;
+  std::vector<mojom::P2PSendPacketPtr> packet_batch;
+  std::vector<uint8_t> packet;
+  CreateRandomPacket(&packet);
+  packet_batch.emplace_back(mojom::P2PSendPacket::New(
+      packet, P2PPacketInfo(dest_.ip_address, options, 0)));
+  std::vector<uint8_t> packet2;
+  CreateRandomPacket(&packet2);
+  packet_batch.emplace_back(mojom::P2PSendPacket::New(
+      packet2, P2PPacketInfo(dest_.ip_address, options, 0)));
+  socket_->set_error_on_next_write(net::ERR_FAILED);
+  // We now expect the first packet to cause the connection to be destroyed,
+  // and the second packet to never be processed.
+  auto socket_impl_ptr = socket_impl_.get();
+  socket_delegate_.ExpectDestruction(std::move(socket_impl_));
+  // The socket will be destroyed, so clear our raw_ptr to it to avoid
+  // a dangling pointer warning.
+  socket_ = nullptr;
+  socket_impl_ptr->SendBatch(std::move(packet_batch));
+  // process any queued callbacks.
+  base::RunLoop().RunUntilIdle();
+}
+
 }  // namespace network