s1ap - add unit test to check correct handling of invalid E-RAB ids in modification request. Fix s1ap in order for the test to pass

lib/include/srsran/interfaces/enb_rrc_interfaces.h

@@ -30,8 +30,8 @@ public:
   virtual bool setup_ue_erabs(uint16_t rnti, const asn1::s1ap::erab_setup_request_s& msg)        = 0;
   virtual void
                modify_erabs(uint16_t                                                                         rnti,
-                            srsran::const_span<const asn1::s1ap::erab_to_be_modified_item_bearer_mod_req_s*> erabs_to_modify,
+                            srsran::const_span<const asn1::s1ap::erab_to_be_modified_item_bearer_mod_req_s*> erabs_to_modify) = 0;
-                            std::vector<uint16_t>* erabs_failed_to_modify)                  = 0;
+  virtual bool has_erab(uint16_t rnti, uint32_t erab_id) const                              = 0;
   virtual bool release_erabs(uint32_t rnti)                                                 = 0;
   virtual void release_erabs(uint32_t                              rnti,
                              const asn1::s1ap::erab_release_cmd_s& msg,

srsenb/hdr/stack/rrc/rrc.h

@@ -82,9 +82,10 @@ public:
   bool setup_ue_ctxt(uint16_t rnti, const asn1::s1ap::init_context_setup_request_s& msg) override;
   bool modify_ue_ctxt(uint16_t rnti, const asn1::s1ap::ue_context_mod_request_s& msg) override;
   bool setup_ue_erabs(uint16_t rnti, const asn1::s1ap::erab_setup_request_s& msg) override;
-  void modify_erabs(uint16_t                                                                         rnti,
+  bool has_erab(uint16_t rnti, uint32_t erab_id) const override;
-                    srsran::const_span<const asn1::s1ap::erab_to_be_modified_item_bearer_mod_req_s*> erabs_to_modify,
+  void modify_erabs(
-                    std::vector<uint16_t>* erabs_failed_to_modify) override;
+      uint16_t                                                                         rnti,
+      srsran::const_span<const asn1::s1ap::erab_to_be_modified_item_bearer_mod_req_s*> erabs_to_modify) override;
   bool modify_ue_erab(uint16_t                                   rnti,
                       uint8_t                                    erab_id,
                       const asn1::s1ap::erab_level_qos_params_s& qos_params,

srsenb/hdr/stack/rrc/rrc_ue.h

@@ -53,6 +53,9 @@ public:
   ///< Helper to access a cell cfg based on ue_cc_idx
   enb_cell_common* get_ue_cc_cfg(uint32_t ue_cc_idx);
 
+  /// Helper to check UE ERABs
+  bool has_erab(uint32_t erab_id) const { return bearer_list.get_erabs().count(erab_id) > 0; }
+
   /// List of results a RRC procedure may produce.
   enum class procedure_result_code {
     none,

srsenb/src/stack/rrc/rrc.cc

@@ -386,9 +386,18 @@ void rrc::release_erabs(uint32_t                              rnti,
   user_it->second->send_connection_reconf(nullptr, false, nas_pdu);
 }
 
+bool rrc::has_erab(uint16_t rnti, uint32_t erab_id) const
+{
+  auto user_it = users.find(rnti);
+  if (user_it == users.end()) {
+    logger.warning("Unrecognised rnti: 0x%x", rnti);
+    return false;
+  }
+  return user_it->second->has_erab(erab_id);
+}
+
 void rrc::modify_erabs(uint16_t                                                                         rnti,
-                       srsran::const_span<const asn1::s1ap::erab_to_be_modified_item_bearer_mod_req_s*> erabs_to_modify,
+                       srsran::const_span<const asn1::s1ap::erab_to_be_modified_item_bearer_mod_req_s*> erabs_to_modify)
-                       std::vector<uint16_t>* erabs_failed_to_modify)
 {
   logger.info("Modifying E-RABs for 0x%x", rnti);
   auto user_it = users.find(rnti);
@@ -401,9 +410,7 @@ void rrc::modify_erabs(uint16_t
   for (const auto* erab_ptr : erabs_to_modify) {
     // Attempt to modify E-RAB
     bool ret = modify_ue_erab(rnti, erab_ptr->erab_id, erab_ptr->erab_level_qos_params, &erab_ptr->nas_pdu);
-    if (not ret) {
+    srsran_expect(ret, "modify_erabs should not called for valid E-RAB Ids");
-      erabs_failed_to_modify->push_back(erab_ptr->erab_id);
-    }
   }
 }
 

srsenb/src/stack/upper/s1ap.cc

@@ -737,38 +737,52 @@ bool s1ap::handle_erabmodifyrequest(const erab_modify_request_s& msg)
 
   // make a copy, sort by ERAB ID
   using erab_t        = erab_to_be_modified_item_bearer_mod_req_s;
+  using failed_erab_t = std::pair<uint16_t, cause_c>;
   srsran::bounded_vector<const erab_t*, 256> erab_mod_list;
-  for (const auto& erab : msg.protocol_ies.erab_to_be_modified_list_bearer_mod_req.value) {
+  std::vector<failed_erab_t>                 erabs_failed_to_modify;
-    erab_mod_list.push_back(&erab.value.erab_to_be_modified_item_bearer_mod_req());
+  auto&                                      msg_erabs = msg.protocol_ies.erab_to_be_modified_list_bearer_mod_req.value;
+
+  for (const auto& msg_erab : msg_erabs) {
+    const erab_t& e = msg_erab.value.erab_to_be_modified_item_bearer_mod_req();
+
+    // Check if E-RAB exists. If not, add to "erabs_failed_to_modify" with "unknown_erab_id" cause
+    if (not rrc->has_erab(u->ctxt.rnti, e.erab_id)) {
+      cause_c cause;
+      cause.set_radio_network().value = cause_radio_network_opts::unknown_erab_id;
+      erabs_failed_to_modify.emplace_back(e.erab_id, cause);
+      continue;
     }
-  auto lower_erab = [](const erab_t* lhs, const erab_t* rhs) { return lhs->erab_id < rhs->erab_id; };
-  std::sort(erab_mod_list.begin(), erab_mod_list.end(), lower_erab);
 
-  // Find repeated ERAB-IDs and add them to list of ERABs failed to modify
+    // Check Repeated E-RABs in the modification list. If repeated, add to the list of "erabs_failed_to_modify"
-  std::vector<std::pair<uint16_t, cause_c> > erabs_failed_to_modify;
+    // with cause "multiple_erab_id_instances"
-  auto                                       new_end = std::unique(erab_mod_list.begin(), erab_mod_list.end());
+    for (const auto& msg_erab2 : msg_erabs) {
-  for (auto it = new_end; it != erab_mod_list.end(); ++it) {
+      const erab_t& e2 = msg_erab2.value.erab_to_be_modified_item_bearer_mod_req();
+      if (&msg_erab2 != &msg_erab and e2.erab_id == e.erab_id) {
         cause_c cause;
         cause.set_radio_network().value = cause_radio_network_opts::multiple_erab_id_instances;
-    erabs_failed_to_modify.emplace_back((*it)->erab_id, cause);
+        erabs_failed_to_modify.emplace_back(e.erab_id, cause);
+        break;
+      }
+    }
+    if (not erabs_failed_to_modify.empty() and erabs_failed_to_modify.back().first == e.erab_id) {
+      continue;
+    }
+
+    // Add to the list to modify
+    erab_mod_list.push_back(&e);
   }
-  erab_mod_list.erase(new_end, erab_mod_list.end());
 
   // Modify E-RABs from RRC
-  std::vector<uint16_t> unknown_erabids;
+  rrc->modify_erabs(u->ctxt.rnti, erab_mod_list);
-  rrc->modify_erabs(u->ctxt.rnti, erab_mod_list, &unknown_erabids);
 
-  // Add Unknown E-RAB to the list of failed to modify
+  // Sort by E-RAB id, and remove duplicates
-  for (uint16_t erab : unknown_erabids) {
+  auto lower_erab = [](const erab_t* lhs, const erab_t* rhs) { return lhs->erab_id < rhs->erab_id; };
-    cause_c cause;
+  std::sort(erab_mod_list.begin(), erab_mod_list.end(), lower_erab);
-    cause.set_radio_network().value = cause_radio_network_opts::unknown_erab_id;
+  auto lower_erab2 = [](const failed_erab_t& lhs, const failed_erab_t& rhs) { return lhs.first < rhs.first; };
-    erabs_failed_to_modify.emplace_back(erab, cause);
+  auto equal_erab2 = [](const failed_erab_t& lhs, const failed_erab_t& rhs) { return lhs.first == rhs.first; };
-    auto lower_erab2 = [](const erab_t* lhs, uint16_t erab) { return lhs->erab_id < erab; };
+  std::sort(erabs_failed_to_modify.begin(), erabs_failed_to_modify.end(), lower_erab2);
-    auto it          = std::lower_bound(erab_mod_list.begin(), erab_mod_list.end(), erab, lower_erab2);
+  erabs_failed_to_modify.erase(std::unique(erabs_failed_to_modify.begin(), erabs_failed_to_modify.end(), equal_erab2),
-    if (it != erab_mod_list.end() and (*it)->erab_id == erab) {
+                               erabs_failed_to_modify.end());
-      erab_mod_list.erase(it);
-    }
-  }
 
   // Send E-RAB modify response back to the MME
   if (not u->send_erab_modify_response(erab_mod_list, erabs_failed_to_modify)) {

srsenb/test/common/dummy_classes.h

@@ -168,10 +168,11 @@ public:
   bool setup_ue_ctxt(uint16_t rnti, const asn1::s1ap::init_context_setup_request_s& msg) override { return true; }
   bool modify_ue_ctxt(uint16_t rnti, const asn1::s1ap::ue_context_mod_request_s& msg) override { return true; }
   bool setup_ue_erabs(uint16_t rnti, const asn1::s1ap::erab_setup_request_s& msg) override { return true; }
-  void modify_erabs(uint16_t                                                                         rnti,
+  void modify_erabs(
-                    srsran::const_span<const asn1::s1ap::erab_to_be_modified_item_bearer_mod_req_s*> erabs_to_modify,
+      uint16_t                                                                         rnti,
-                    std::vector<uint16_t>* erabs_failed_to_modify) override
+      srsran::const_span<const asn1::s1ap::erab_to_be_modified_item_bearer_mod_req_s*> erabs_to_modify) override
   {}
+  bool has_erab(uint16_t rnti, uint32_t erab_id) const override { return true; }
   bool release_erabs(uint32_t rnti) override { return true; }
   void release_erabs(uint32_t                              rnti,
                      const asn1::s1ap::erab_release_cmd_s& msg,

srsenb/test/upper/s1ap_test.cc

@@ -91,16 +91,18 @@ struct dummy_socket_manager : public srsran::socket_manager_itf {
 };
 
 struct rrc_tester : public rrc_dummy {
-  void modify_erabs(uint16_t                                                                         rnti,
+  void modify_erabs(
-                    srsran::const_span<const asn1::s1ap::erab_to_be_modified_item_bearer_mod_req_s*> erabs_to_modify,
+      uint16_t                                                                         rnti,
-                    std::vector<uint16_t>* erabs_failed_to_modify) override
+      srsran::const_span<const asn1::s1ap::erab_to_be_modified_item_bearer_mod_req_s*> erabs_to_modify) override
   {
+    last_erabs_modified.clear();
     for (auto& erab : erabs_to_modify) {
-      if (std::count(next_erabs_failed_to_modify.begin(), next_erabs_failed_to_modify.end(), erab->erab_id) == 0) {
       last_erabs_modified.push_back(erab->erab_id);
     }
   }
-    *erabs_failed_to_modify = next_erabs_failed_to_modify;
+  bool has_erab(uint16_t rnti, uint32_t erab_id) const override
+  {
+    return std::count(next_erabs_failed_to_modify.begin(), next_erabs_failed_to_modify.end(), erab_id) == 0;
   }
   void release_ue(uint16_t rnti) override { last_released_rnti = rnti; }
 
@@ -187,7 +189,7 @@ void add_rnti(s1ap& s1ap_obj, mme_dummy& mme)
   TESTASSERT(s1ap_pdu.successful_outcome().proc_code == ASN1_S1AP_ID_INIT_CONTEXT_SETUP);
 }
 
-enum class test_event { success, wrong_erabid_mod, wrong_mme_s1ap_id };
+enum class test_event { success, wrong_erabid_mod, wrong_mme_s1ap_id, repeated_erabid_mod };
 
 void test_s1ap_erab_setup(test_event event)
 {
@@ -223,19 +225,34 @@ void test_s1ap_erab_setup(test_event event)
   sockaddr_in            mme_addr = {};
   sctp_sndrcvinfo        rcvinfo  = {};
   int                    flags    = 0;
-  uint8_t         mod_req_msg[] = {0x00, 0x06, 0x00, 0x1E, 0x00, 0x00, 0x03, 0x00, 0x00, 0x00, 0x02, 0x00,
+  asn1::s1ap::s1ap_pdu_c mod_req_pdu;
-                           0x01, 0x00, 0x08, 0x00, 0x02, 0x00, 0x01, 0x00, 0x1E, 0x00, 0x0B, 0x00,
+  mod_req_pdu.set_init_msg().load_info_obj(ASN1_S1AP_ID_ERAB_MODIFY);
-                           0x00, 0x24, 0x00, 0x06, 0x0A, 0x00, 0x09, 0x3C, 0x01, 0x00};
+  auto& protocols                = mod_req_pdu.init_msg().value.erab_modify_request().protocol_ies;
-  // 00 06 00 1E 00 00 03 00 00 00 02 00 01 00 08 00 02 00 01 00 1E 00 0B 00 00 24 00 06 0A 00 09 3C 01 00
+  protocols.enb_ue_s1ap_id.value = 1;
+  protocols.mme_ue_s1ap_id.value = event == test_event::wrong_mme_s1ap_id ? 2 : 1;
+  auto& erab_list                = protocols.erab_to_be_modified_list_bearer_mod_req.value;
+  erab_list.resize(2);
+  erab_list[0].load_info_obj(ASN1_S1AP_ID_ERAB_TO_BE_MODIFIED_ITEM_BEARER_MOD_REQ);
+  auto* erab_ptr                      = &erab_list[0].value.erab_to_be_modified_item_bearer_mod_req();
+  erab_ptr->erab_id                   = 5;
+  erab_ptr->erab_level_qos_params.qci = 9;
+  erab_ptr->erab_level_qos_params.alloc_retention_prio.prio_level = 15;
+  erab_ptr->erab_level_qos_params.alloc_retention_prio.pre_emption_cap.value =
+      asn1::s1ap::pre_emption_cap_opts::shall_not_trigger_pre_emption;
+  erab_ptr->erab_level_qos_params.alloc_retention_prio.pre_emption_vulnerability.value =
+      asn1::s1ap::pre_emption_vulnerability_opts::not_pre_emptable;
+  erab_ptr->nas_pdu.resize(1);
+  erab_list[1]      = erab_list[0];
+  erab_ptr          = &erab_list[1].value.erab_to_be_modified_item_bearer_mod_req();
+  erab_ptr->erab_id = event == test_event::repeated_erabid_mod ? 5 : 6;
   if (event == test_event::wrong_erabid_mod) {
-    mod_req_msg[sizeof(mod_req_msg) - 6] = 0x0C; // E-RAB id = 6
     rrc.next_erabs_failed_to_modify.push_back(6);
-  } else if (event == test_event::wrong_mme_s1ap_id) {
-    mod_req_msg[12] = 0x02; // MME-UE-S1AP-ID = 2
   }
   sdu = srsran::make_byte_buffer();
-  memcpy(sdu->msg, mod_req_msg, sizeof(mod_req_msg));
+  asn1::bit_ref bref(sdu->msg, sdu->get_tailroom());
-  sdu->N_bytes = sizeof(mod_req_msg);
+  TESTASSERT(mod_req_pdu.pack(bref) == SRSRAN_SUCCESS);
+  sdu->N_bytes = bref.distance_bytes();
+
   TESTASSERT(rrc.last_released_rnti == SRSRAN_INVALID_RNTI);
   TESTASSERT(s1ap_obj.handle_mme_rx_msg(std::move(sdu), mme_addr, rcvinfo, flags));
   sdu = mme.read_msg();
@@ -258,7 +275,10 @@ void test_s1ap_erab_setup(test_event event)
   TESTASSERT(s1ap_pdu.successful_outcome().proc_code == ASN1_S1AP_ID_ERAB_MODIFY);
   auto& protocol_ies = s1ap_pdu.successful_outcome().value.erab_modify_resp().protocol_ies;
   if (event == test_event::wrong_erabid_mod) {
-    TESTASSERT(not protocol_ies.erab_modify_list_bearer_mod_res_present);
+    TESTASSERT(protocol_ies.erab_modify_list_bearer_mod_res_present);
+    TESTASSERT(protocol_ies.erab_modify_list_bearer_mod_res.value.size() == 1);
+    TESTASSERT(protocol_ies.erab_modify_list_bearer_mod_res.value[0].value.erab_modify_item_bearer_mod_res().erab_id ==
+               5);
     TESTASSERT(protocol_ies.erab_failed_to_modify_list_present);
     TESTASSERT(protocol_ies.erab_failed_to_modify_list.value.size() == 1);
     auto& erab_item = protocol_ies.erab_failed_to_modify_list.value[0].value.erab_item();
@@ -267,10 +287,21 @@ void test_s1ap_erab_setup(test_event event)
     TESTASSERT(erab_item.cause.radio_network().value == asn1::s1ap::cause_radio_network_opts::unknown_erab_id);
     return;
   }
+  if (event == test_event::repeated_erabid_mod) {
+    TESTASSERT(not protocol_ies.erab_modify_list_bearer_mod_res_present);
+    TESTASSERT(protocol_ies.erab_failed_to_modify_list_present);
+    TESTASSERT(protocol_ies.erab_failed_to_modify_list.value.size() == 1);
+    auto& erab_item = protocol_ies.erab_failed_to_modify_list.value[0].value.erab_item();
+    TESTASSERT(erab_item.erab_id == 5);
+    TESTASSERT(erab_item.cause.type().value == asn1::s1ap::cause_c::types_opts::radio_network);
+    TESTASSERT(erab_item.cause.radio_network().value ==
+               asn1::s1ap::cause_radio_network_opts::multiple_erab_id_instances);
+    return;
+  }
 
   TESTASSERT(protocol_ies.erab_modify_list_bearer_mod_res_present);
   TESTASSERT(not protocol_ies.erab_failed_to_modify_list_present);
-  TESTASSERT(protocol_ies.erab_modify_list_bearer_mod_res.value.size() == 1);
+  TESTASSERT(protocol_ies.erab_modify_list_bearer_mod_res.value.size() == 2);
   auto& erab_item = protocol_ies.erab_modify_list_bearer_mod_res.value[0].value.erab_modify_item_bearer_mod_res();
   TESTASSERT(erab_item.erab_id == 5);
 }
@@ -288,4 +319,5 @@ int main(int argc, char** argv)
   test_s1ap_erab_setup(test_event::success);
   test_s1ap_erab_setup(test_event::wrong_erabid_mod);
   test_s1ap_erab_setup(test_event::wrong_mme_s1ap_id);
+  test_s1ap_erab_setup(test_event::repeated_erabid_mod);
 }