Fix a dangling SDU pointer in mac_sch_subpdu_nr when adding subpdus into a mac_sch_pdu_nr.

lib/include/srsran/mac/mac_sch_pdu_nr.h

@@ -111,10 +111,63 @@ private:
   int      header_length = 0;
   int      sdu_length    = 0;
   bool     F_bit         = false;
-  uint8_t* sdu           = nullptr;
 
-  static const uint8_t mac_ce_payload_len = 8 + 1;         // Long BSR has max. 9 octets (see sizeof_ce() too)
+  /// This helper class manages a SDU pointer that can point to either a user provided external buffer or to a small
-  std::array<uint8_t, mac_ce_payload_len> ce_write_buffer; // Buffer for CE payload
+  /// internal buffer, useful for storing very short SDUs.
+  class sdu_buffer
+  {
+    static const uint8_t mac_ce_payload_len = 8 + 1;         // Long BSR has max. 9 octets (see sizeof_ce() too)
+    std::array<uint8_t, mac_ce_payload_len> ce_write_buffer; // Buffer for CE payload
+    uint8_t*                                sdu = nullptr;
+
+  public:
+    sdu_buffer() = default;
+
+    sdu_buffer(const sdu_buffer& other) : ce_write_buffer(other.ce_write_buffer)
+    {
+      // First check if we need to use internal storage.
+      if (other.sdu == other.ce_write_buffer.data()) {
+        sdu = ce_write_buffer.data();
+        return;
+      }
+      sdu = other.sdu;
+    }
+
+    sdu_buffer& operator=(const sdu_buffer& other)
+    {
+      if (this == &other) {
+        return *this;
+      }
+      ce_write_buffer = other.ce_write_buffer;
+      if (other.sdu == other.ce_write_buffer.data()) {
+        sdu = ce_write_buffer.data();
+        return *this;
+      }
+      sdu = other.sdu;
+      return *this;
+    }
+
+    explicit operator bool() const { return sdu; }
+
+    /// Set the SDU pointer to use the internal buffer.
+    uint8_t* use_internal_storage()
+    {
+      sdu = ce_write_buffer.data();
+      return sdu;
+    }
+
+    /// Set the SDU pointer to point to the provided buffer.
+    uint8_t* set_storage_to(uint8_t* p)
+    {
+      sdu = p;
+      return sdu;
+    }
+
+    /// Returns the SDU pointer.
+    uint8_t* ptr() { return sdu; }
+  };
+
+  sdu_buffer sdu;
 
   mac_sch_pdu_nr* parent = nullptr;
 };

lib/src/mac/mac_sch_pdu_nr.cc

@@ -65,7 +65,7 @@ int32_t mac_sch_subpdu_nr::read_subheader(const uint8_t* ptr)
     } else {
       sdu_length = sizeof_ce(lcid, parent->is_ulsch());
     }
-    sdu = (uint8_t*)ptr;
+    sdu.set_storage_to((uint8_t*)ptr);
   } else {
     srslog::fetch_basic_logger("MAC-NR").warning("Invalid LCID (%d) in MAC PDU", lcid);
     return SRSRAN_ERROR;
@@ -75,8 +75,8 @@ int32_t mac_sch_subpdu_nr::read_subheader(const uint8_t* ptr)
 
 void mac_sch_subpdu_nr::set_sdu(const uint32_t lcid_, const uint8_t* payload_, const uint32_t len_)
 {
-  lcid          = lcid_;
+  lcid = lcid_;
-  sdu           = const_cast<uint8_t*>(payload_);
+  sdu.set_storage_to(const_cast<uint8_t*>(payload_));
   header_length = is_ul_ccch() ? 1 : 2;
   sdu_length    = len_;
   if (is_ul_ccch()) {
@@ -104,34 +104,34 @@ void mac_sch_subpdu_nr::set_padding(const uint32_t len_)
 // Turn a subPDU into a C-RNTI CE, error checking takes place in the caller
 void mac_sch_subpdu_nr::set_c_rnti(const uint16_t crnti_)
 {
-  lcid                  = CRNTI;
+  lcid           = CRNTI;
-  header_length         = 1;
+  header_length  = 1;
-  sdu_length            = sizeof_ce(lcid, parent->is_ulsch());
+  sdu_length     = sizeof_ce(lcid, parent->is_ulsch());
-  sdu                   = ce_write_buffer.data();
+  uint16_t crnti = htole16(crnti_);
-  uint16_t crnti        = htole16(crnti_);
+  uint8_t* ptr   = sdu.use_internal_storage();
-  ce_write_buffer.at(0) = (uint8_t)((crnti & 0xff00) >> 8);
+  ptr[0]         = (uint8_t)((crnti & 0xff00) >> 8);
-  ce_write_buffer.at(1) = (uint8_t)((crnti & 0x00ff));
+  ptr[1]         = (uint8_t)((crnti & 0x00ff));
 }
 
 // Turn a subPDU into a single entry PHR CE, error checking takes place in the caller
 void mac_sch_subpdu_nr::set_se_phr(const uint8_t phr_, const uint8_t pcmax_)
 {
-  lcid                  = SE_PHR;
+  lcid          = SE_PHR;
-  header_length         = 1;
+  header_length = 1;
-  sdu_length            = sizeof_ce(lcid, parent->is_ulsch());
+  sdu_length    = sizeof_ce(lcid, parent->is_ulsch());
-  sdu                   = ce_write_buffer.data();
+  uint8_t* ptr  = sdu.use_internal_storage();
-  ce_write_buffer.at(0) = (uint8_t)(phr_ & 0x3f);
+  ptr[0]        = (uint8_t)(phr_ & 0x3f);
-  ce_write_buffer.at(1) = (uint8_t)(pcmax_ & 0x3f);
+  ptr[1]        = (uint8_t)(pcmax_ & 0x3f);
 }
 
 // Turn a subPDU into a single short BSR
 void mac_sch_subpdu_nr::set_sbsr(const lcg_bsr_t bsr_)
 {
-  lcid                  = SHORT_BSR;
+  lcid          = SHORT_BSR;
-  header_length         = 1;
+  header_length = 1;
-  sdu_length            = sizeof_ce(lcid, parent->is_ulsch());
+  sdu_length    = sizeof_ce(lcid, parent->is_ulsch());
-  sdu                   = ce_write_buffer.data();
+  uint8_t* ptr  = sdu.use_internal_storage();
-  ce_write_buffer.at(0) = ((bsr_.lcg_id & 0x07) << 5) | (bsr_.buffer_size & 0x1f);
+  ptr[0]        = ((bsr_.lcg_id & 0x07) << 5) | (bsr_.buffer_size & 0x1f);
 }
 
 // Turn a subPDU into a long BSR with variable size
@@ -163,7 +163,7 @@ uint32_t mac_sch_subpdu_nr::write_subpdu(const uint8_t* start_)
 
   // copy SDU payload
   if (sdu) {
-    memcpy(ptr, sdu, sdu_length);
+    memcpy(ptr, sdu.ptr(), sdu_length);
   } else {
     // clear memory
     memset(ptr, 0, sdu_length);
@@ -192,13 +192,14 @@ uint32_t mac_sch_subpdu_nr::get_lcid()
 
 uint8_t* mac_sch_subpdu_nr::get_sdu()
 {
-  return sdu;
+  return sdu.ptr();
 }
 
 uint16_t mac_sch_subpdu_nr::get_c_rnti()
 {
   if (parent->is_ulsch() && lcid == CRNTI) {
-    return le16toh((uint16_t)sdu[0] << 8 | sdu[1]);
+    uint8_t* ptr = sdu.ptr();
+    return le16toh((uint16_t)ptr[0] << 8 | ptr[1]);
   }
   return 0;
 }
@@ -206,7 +207,8 @@ uint16_t mac_sch_subpdu_nr::get_c_rnti()
 uint8_t mac_sch_subpdu_nr::get_phr()
 {
   if (parent->is_ulsch() && lcid == SE_PHR) {
-    return sdu[0] & 0x3f;
+    uint8_t* ptr = sdu.ptr();
+    return ptr[0] & 0x3f;
   }
   return 0;
 }
@@ -214,7 +216,8 @@ uint8_t mac_sch_subpdu_nr::get_phr()
 uint8_t mac_sch_subpdu_nr::get_pcmax()
 {
   if (parent->is_ulsch() && lcid == SE_PHR) {
-    return sdu[1] & 0x3f;
+    uint8_t* ptr = sdu.ptr();
+    return ptr[1] & 0x3f;
   }
   return 0;
 }
@@ -223,8 +226,9 @@ mac_sch_subpdu_nr::ta_t mac_sch_subpdu_nr::get_ta()
 {
   ta_t ta = {};
   if (lcid == TA_CMD) {
-    ta.tag_id     = (sdu[0] & 0xc0) >> 6;
+    uint8_t* ptr  = sdu.ptr();
-    ta.ta_command = sdu[0] & 0x3f;
+    ta.tag_id     = (ptr[0] & 0xc0) >> 6;
+    ta.ta_command = ptr[0] & 0x3f;
   }
   return ta;
 }
@@ -233,8 +237,9 @@ mac_sch_subpdu_nr::lcg_bsr_t mac_sch_subpdu_nr::get_sbsr()
 {
   lcg_bsr_t sbsr = {};
   if (parent->is_ulsch() && lcid == SHORT_BSR) {
-    sbsr.lcg_id      = (sdu[0] & 0xe0) >> 5;
+    uint8_t* ptr     = sdu.ptr();
-    sbsr.buffer_size = sdu[0] & 0x1f;
+    sbsr.lcg_id      = (ptr[0] & 0xe0) >> 5;
+    sbsr.buffer_size = ptr[0] & 0x1f;
   }
   return sbsr;
 }