Merge "ACL: Allow short packets" into main

system/gd/hci/acl_manager/assembler.h

@@ -34,23 +34,35 @@ constexpr size_t kMaxQueuedPacketsPerConnection = 10;
 constexpr size_t kL2capBasicFrameHeaderSize = 4;
 
 namespace {
+// This is a helper class to keep the state of the assembler and expose PacketView<>::Append.
 class PacketViewForRecombination : public packet::PacketView<packet::kLittleEndian> {
  public:
-  PacketViewForRecombination(const PacketView& packetView) : PacketView(packetView) {}
+  PacketViewForRecombination(const PacketView& packetView)
+      : PacketView(packetView), received_first_(true) {}
+
+  PacketViewForRecombination()
+      : PacketView(PacketView<packet::kLittleEndian>(std::make_shared<std::vector<uint8_t>>())) {}
+
   void AppendPacketView(packet::PacketView<packet::kLittleEndian> to_append) {
     Append(to_append);
   }
+
+  bool ReceivedFirstPacket() {
+    return received_first_;
+  }
+
+ private:
+  bool received_first_{};
 };
 
-// Per spec 5.1 Vol 2 Part B 5.3, ACL link shall carry L2CAP data. Therefore, an ACL packet shall contain L2CAP PDU.
-// This function returns the PDU size of the L2CAP data if it's a starting packet. Returns 0 if it's invalid.
-uint16_t GetL2capPduSize(AclView packet) {
-  auto l2cap_payload = packet.GetPayload();
-  if (l2cap_payload.size() < kL2capBasicFrameHeaderSize) {
-    LOG_ERROR("Controller sent an invalid L2CAP starting packet!");
-    return 0;
+// Per spec 5.1 Vol 2 Part B 5.3, ACL link shall carry L2CAP data. Therefore, an ACL packet shall
+// contain L2CAP PDU. This function returns the PDU size of the L2CAP starting packet, or
+// kL2capBasicFrameHeaderSize if it's invalid.
+size_t GetL2capPduSize(packet::PacketView<packet::kLittleEndian> pdu) {
+  if (pdu.size() < 2) {
+    return kL2capBasicFrameHeaderSize;  // We need at least 4 bytes to send it to L2CAP
   }
-  return (l2cap_payload.at(1) << 8u) + l2cap_payload.at(0);
+  return (static_cast<size_t>(pdu[1]) << 8u) + pdu[0];
 }
 
 }  // namespace
@@ -61,9 +73,7 @@ struct assembler {
   AddressWithType address_with_type_;
   AclConnection::QueueDownEnd* down_end_;
   os::Handler* handler_;
-  PacketViewForRecombination recombination_stage_{
-      PacketView<packet::kLittleEndian>(std::make_shared<std::vector<uint8_t>>())};
-  size_t remaining_sdu_continuation_packet_size_ = 0;
+  PacketViewForRecombination recombination_stage_{};
   std::shared_ptr<std::atomic_bool> enqueue_registered_ = std::make_shared<std::atomic_bool>(false);
   std::queue<packet::PacketView<packet::kLittleEndian>> incoming_queue_;
 
@@ -74,7 +84,7 @@ struct assembler {
   }
 
   // Invoked from some external Queue Reactable context
-  std::unique_ptr<packet::PacketView<packet::kLittleEndian>> on_le_incoming_data_ready() {
+  std::unique_ptr<packet::PacketView<packet::kLittleEndian>> on_data_ready() {
     auto packet = incoming_queue_.front();
     incoming_queue_.pop();
     if (incoming_queue_.empty() && enqueue_registered_->exchange(false)) {
@@ -85,7 +95,6 @@ struct assembler {
 
   void on_incoming_packet(AclView packet) {
     PacketView<packet::kLittleEndian> payload = packet.GetPayload();
-    size_t payload_size = payload.size();
     auto broadcast_flag = packet.GetBroadcastFlag();
     if (broadcast_flag == BroadcastFlag::ACTIVE_PERIPHERAL_BROADCAST) {
       LOG_WARN("Dropping broadcast from remote");
@@ -97,58 +106,39 @@ struct assembler {
       return;
     }
     if (packet_boundary_flag == PacketBoundaryFlag::CONTINUING_FRAGMENT) {
-      if (remaining_sdu_continuation_packet_size_ < payload_size) {
-        LOG_WARN("Remote sent unexpected L2CAP PDU. Drop the entire L2CAP PDU");
-        recombination_stage_ =
-            PacketViewForRecombination(PacketView<packet::kLittleEndian>(std::make_shared<std::vector<uint8_t>>()));
-        remaining_sdu_continuation_packet_size_ = 0;
+      if (!recombination_stage_.ReceivedFirstPacket()) {
+        LOG_ERROR("Continuing fragment received without previous first, dropping it.");
         return;
       }
-      remaining_sdu_continuation_packet_size_ -= payload_size;
       recombination_stage_.AppendPacketView(payload);
-      if (remaining_sdu_continuation_packet_size_ != 0) {
-        return;
-      } else {
-        payload = recombination_stage_;
-        recombination_stage_ =
-            PacketViewForRecombination(PacketView<packet::kLittleEndian>(std::make_shared<std::vector<uint8_t>>()));
-      }
     } else if (packet_boundary_flag == PacketBoundaryFlag::FIRST_AUTOMATICALLY_FLUSHABLE) {
-      if (recombination_stage_.size() > 0) {
+      if (recombination_stage_.ReceivedFirstPacket()) {
         LOG_ERROR("Controller sent a starting packet without finishing previous packet. Drop previous one.");
       }
-      size_t l2cap_pdu_size = GetL2capPduSize(packet);
-      if (l2cap_pdu_size == 0) {
-        LOG_WARN("dropping an invalid L2CAP packet");
-        return;
-      }
-
-      remaining_sdu_continuation_packet_size_ = l2cap_pdu_size - (payload_size - kL2capBasicFrameHeaderSize);
-      if ((payload_size - kL2capBasicFrameHeaderSize) > l2cap_pdu_size) {
-        LOG_WARN(
-            "Remote presented mismatched packet sizes payload_size:%zu l2cap_pdu_size:%zu",
-            payload_size - kL2capBasicFrameHeaderSize,
-            l2cap_pdu_size);
-        remaining_sdu_continuation_packet_size_ = 0;
-      } else {
-        remaining_sdu_continuation_packet_size_ =
-            l2cap_pdu_size - (payload_size - kL2capBasicFrameHeaderSize);
-      }
-      if (remaining_sdu_continuation_packet_size_ > 0) {
-        recombination_stage_ = payload;
-        return;
-      }
+      recombination_stage_ = payload;
+    }
+    // Check the size of the packet
+    size_t expected_size = GetL2capPduSize(recombination_stage_) + kL2capBasicFrameHeaderSize;
+    if (expected_size < recombination_stage_.size()) {
+      LOG_INFO("Packet size doesn't match L2CAP header, dropping it.");
+      recombination_stage_ = PacketViewForRecombination();
+      return;
+    } else if (expected_size > recombination_stage_.size()) {
+      // Wait for the next fragment before sending
+      return;
     }
     if (incoming_queue_.size() > kMaxQueuedPacketsPerConnection) {
       LOG_ERROR("Dropping packet from %s due to congestion",
                  ADDRESS_TO_LOGGABLE_CSTR(address_with_type_));
+      recombination_stage_ = PacketViewForRecombination();
       return;
     }
 
-    incoming_queue_.push(payload);
+    incoming_queue_.push(recombination_stage_);
+    recombination_stage_ = PacketViewForRecombination();
     if (!enqueue_registered_->exchange(true)) {
-      down_end_->RegisterEnqueue(handler_,
-                                 common::Bind(&assembler::on_le_incoming_data_ready, common::Unretained(this)));
+      down_end_->RegisterEnqueue(
+          handler_, common::Bind(&assembler::on_data_ready, common::Unretained(this)));
     }
   }
 };

system/gd/hci/acl_manager_test.cc

@@ -46,6 +46,7 @@ using bluetooth::packet::kLittleEndian;
 using bluetooth::packet::PacketView;
 using bluetooth::packet::RawBuilder;
 using testing::_;
+using testing::ElementsAreArray;
 
 namespace {
 
@@ -1387,6 +1388,179 @@ TEST_F(AclManagerWithConnectionTest, remote_esco_connect_request) {
   fake_registry_.SynchronizeModuleHandler(&HciLayer::Factory, std::chrono::milliseconds(20));
 }
 
+class AclManagerWithConnectionAssemblerTest : public AclManagerWithConnectionTest {
+ protected:
+  void SetUp() override {
+    AclManagerWithConnectionTest::SetUp();
+    connection_queue_end_ = connection_->GetAclQueueEnd();
+  }
+
+  std::vector<uint8_t> MakeAclPayload(size_t length, uint16_t cid, uint8_t offset) {
+    std::vector<uint8_t> acl_payload;
+    acl_payload.push_back(length & 0xff);
+    acl_payload.push_back((length >> 8u) & 0xff);
+    acl_payload.push_back(cid & 0xff);
+    acl_payload.push_back((cid >> 8u) & 0xff);
+    for (uint8_t i = 0; i < length; i++) {
+      acl_payload.push_back(i + offset);
+    }
+    return acl_payload;
+  }
+
+  void SendSinglePacket(const std::vector<uint8_t>& acl_payload) {
+    auto payload_builder = std::make_unique<RawBuilder>(acl_payload);
+
+    test_hci_layer_->IncomingAclData(
+        handle_,
+        AclBuilder::Create(
+            handle_,
+            PacketBoundaryFlag::FIRST_AUTOMATICALLY_FLUSHABLE,
+            BroadcastFlag::POINT_TO_POINT,
+            std::move(payload_builder)));
+  }
+
+  void ReceiveAndCheckSinglePacket(const std::vector<uint8_t>& acl_payload) {
+    std::unique_ptr<PacketView<kLittleEndian>> received;
+    do {
+      received = connection_queue_end_->TryDequeue();
+    } while (received == nullptr);
+
+    std::vector<uint8_t> received_vector;
+    for (uint8_t byte : *received) {
+      received_vector.push_back(byte);
+    }
+
+    EXPECT_THAT(received_vector, ElementsAreArray(acl_payload));
+  }
+
+  void SendAndReceiveSinglePacket(const std::vector<uint8_t>& acl_payload) {
+    SendSinglePacket(acl_payload);
+    ReceiveAndCheckSinglePacket(acl_payload);
+  }
+
+  void TearDown() override {
+    // Make sure that all previous packets were received and the assembler is in a good state.
+    SendAndReceiveSinglePacket(MakeAclPayload(0x60, 0xACC, 3));
+    AclManagerWithConnectionTest::TearDown();
+  }
+  AclConnection::QueueUpEnd* connection_queue_end_{};
+};
+
+TEST_F(AclManagerWithConnectionAssemblerTest, assembler_test_single_packet) {}
+
+TEST_F(AclManagerWithConnectionAssemblerTest, assembler_test_short_packet_discarded) {
+  std::vector<uint8_t> invalid_payload{1, 2};
+  test_hci_layer_->IncomingAclData(
+      handle_,
+      AclBuilder::Create(
+          handle_,
+          PacketBoundaryFlag::FIRST_AUTOMATICALLY_FLUSHABLE,
+          BroadcastFlag::POINT_TO_POINT,
+          std::make_unique<RawBuilder>(invalid_payload)));
+}
+
+TEST_F(AclManagerWithConnectionAssemblerTest, assembler_test_two_short_packets_discarded) {
+  std::vector<uint8_t> invalid_payload{1, 2};
+  test_hci_layer_->IncomingAclData(
+      handle_,
+      AclBuilder::Create(
+          handle_,
+          PacketBoundaryFlag::FIRST_AUTOMATICALLY_FLUSHABLE,
+          BroadcastFlag::POINT_TO_POINT,
+          std::make_unique<RawBuilder>(invalid_payload)));
+  test_hci_layer_->IncomingAclData(
+      handle_,
+      AclBuilder::Create(
+          handle_,
+          PacketBoundaryFlag::FIRST_AUTOMATICALLY_FLUSHABLE,
+          BroadcastFlag::POINT_TO_POINT,
+          std::make_unique<RawBuilder>(invalid_payload)));
+}
+
+TEST_F(AclManagerWithConnectionAssemblerTest, assembler_test_single_valid_packet) {
+  SendAndReceiveSinglePacket(MakeAclPayload(20, 0x41, 2));
+}
+
+TEST_F(AclManagerWithConnectionAssemblerTest, assembler_test_one_byte_packets) {
+  size_t payload_size = 0x30;
+  std::vector<uint8_t> payload = MakeAclPayload(payload_size, 0xABB /* cid */, 4 /* offset */);
+  test_hci_layer_->IncomingAclData(
+      handle_,
+      AclBuilder::Create(
+          handle_,
+          PacketBoundaryFlag::FIRST_AUTOMATICALLY_FLUSHABLE,
+          BroadcastFlag::POINT_TO_POINT,
+          std::make_unique<RawBuilder>(
+              std::vector<uint8_t>{payload.cbegin(), payload.cbegin() + 1})));
+  for (size_t i = 1; i < payload.size(); i++) {
+    test_hci_layer_->IncomingAclData(
+        handle_,
+        AclBuilder::Create(
+            handle_,
+            PacketBoundaryFlag::CONTINUING_FRAGMENT,
+            BroadcastFlag::POINT_TO_POINT,
+            std::make_unique<RawBuilder>(
+                std::vector<uint8_t>{payload.cbegin() + i, payload.cbegin() + i + 1})));
+  }
+  ReceiveAndCheckSinglePacket(payload);
+}
+
+TEST_F(AclManagerWithConnectionAssemblerTest, assembler_test_two_byte_packets) {
+  size_t payload_size = 0x30;  // must be even
+  std::vector<uint8_t> payload = MakeAclPayload(payload_size, 0xABB /* cid */, 4 /* offset */);
+  test_hci_layer_->IncomingAclData(
+      handle_,
+      AclBuilder::Create(
+          handle_,
+          PacketBoundaryFlag::FIRST_AUTOMATICALLY_FLUSHABLE,
+          BroadcastFlag::POINT_TO_POINT,
+          std::make_unique<RawBuilder>(
+              std::vector<uint8_t>{payload.cbegin(), payload.cbegin() + 2})));
+  for (size_t i = 1; i < payload.size() / 2; i++) {
+    test_hci_layer_->IncomingAclData(
+        handle_,
+        AclBuilder::Create(
+            handle_,
+            PacketBoundaryFlag::CONTINUING_FRAGMENT,
+            BroadcastFlag::POINT_TO_POINT,
+            std::make_unique<RawBuilder>(
+                std::vector<uint8_t>{payload.cbegin() + 2 * i, payload.cbegin() + 2 * (i + 1)})));
+  }
+  ReceiveAndCheckSinglePacket(payload);
+}
+
+TEST_F(AclManagerWithConnectionAssemblerTest, assembler_test_continuation_without_begin) {
+  size_t payload_size = 0x30;
+  std::vector<uint8_t> payload = MakeAclPayload(payload_size, 0xABB /* cid */, 4 /* offset */);
+  test_hci_layer_->IncomingAclData(
+      handle_,
+      AclBuilder::Create(
+          handle_,
+          PacketBoundaryFlag::CONTINUING_FRAGMENT,
+          BroadcastFlag::POINT_TO_POINT,
+          std::make_unique<RawBuilder>(std::vector<uint8_t>{payload.cbegin(), payload.cend()})));
+}
+
+TEST_F(AclManagerWithConnectionAssemblerTest, assembler_test_drop_broadcasts) {
+  test_hci_layer_->IncomingAclData(
+      handle_,
+      AclBuilder::Create(
+          handle_,
+          PacketBoundaryFlag::FIRST_AUTOMATICALLY_FLUSHABLE,
+          BroadcastFlag::ACTIVE_PERIPHERAL_BROADCAST,
+          std::make_unique<RawBuilder>(MakeAclPayload(20, 0xBBB /* cid */, 5 /* offset */))));
+}
+
+TEST_F(AclManagerWithConnectionAssemblerTest, assembler_test_drop_non_flushable) {
+  test_hci_layer_->IncomingAclData(
+      handle_,
+      AclBuilder::Create(
+          handle_,
+          PacketBoundaryFlag::FIRST_NON_AUTOMATICALLY_FLUSHABLE,
+          BroadcastFlag::POINT_TO_POINT,
+          std::make_unique<RawBuilder>(MakeAclPayload(20, 0xAAA /* cid */, 6 /* offset */))));
+}
+
 }  // namespace acl_manager
 }  // namespace hci
 }  // namespace bluetooth

system/gd/hci/hci_layer_fake.cc

@@ -168,28 +168,36 @@ void TestHciLayer::InitEmptyCommand() {
   ASSERT_TRUE(empty_command_view_.IsValid());
 }
 
-void TestHciLayer::IncomingAclData(uint16_t handle) {
+void TestHciLayer::IncomingAclData(uint16_t handle, std::unique_ptr<AclBuilder> acl_builder) {
   os::Handler* hci_handler = GetHandler();
   auto* queue_end = acl_queue_.GetDownEnd();
   std::promise<void> promise;
   auto future = promise.get_future();
+  auto packet = GetPacketView(std::move(acl_builder));
+  auto acl_view = AclView::Create(packet);
   queue_end->RegisterEnqueue(
       hci_handler,
       common::Bind(
-          [](decltype(queue_end) queue_end, uint16_t handle, std::promise<void> promise) {
-            auto packet = GetPacketView(NextAclPacket(handle));
-            AclView acl2 = AclView::Create(packet);
+          [](decltype(queue_end) queue_end,
+             uint16_t handle,
+             AclView acl2,
+             std::promise<void> promise) {
             queue_end->UnregisterEnqueue();
             promise.set_value();
             return std::make_unique<AclView>(acl2);
           },
           queue_end,
           handle,
+          acl_view,
           common::Passed(std::move(promise))));
   auto status = future.wait_for(std::chrono::milliseconds(1000));
   ASSERT_EQ(status, std::future_status::ready);
 }
 
+void TestHciLayer::IncomingAclData(uint16_t handle) {
+  IncomingAclData(handle, NextAclPacket(handle));
+}
+
 void TestHciLayer::AssertNoOutgoingAclData() {
   auto queue_end = acl_queue_.GetDownEnd();
   EXPECT_EQ(queue_end->TryDequeue(), nullptr);

system/gd/hci/hci_layer_fake.h

@@ -61,6 +61,8 @@ class TestHciLayer : public HciLayer {
 
   void IncomingAclData(uint16_t handle);
 
+  void IncomingAclData(uint16_t handle, std::unique_ptr<AclBuilder> acl_builder);
+
   void AssertNoOutgoingAclData();
 
   packet::PacketView<packet::kLittleEndian> OutgoingAclData();