From ec272061a0f6f632103f7021f741abea49abb638 Mon Sep 17 00:00:00 2001 From: faluco Date: Wed, 8 Sep 2021 11:05:00 +0200 Subject: [PATCH] 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 | 59 ++++++++++++++++++++-- lib/src/mac/mac_sch_pdu_nr.cc | 65 +++++++++++++------------ 2 files changed, 91 insertions(+), 33 deletions(-) diff --git a/lib/include/srsran/mac/mac_sch_pdu_nr.h b/lib/include/srsran/mac/mac_sch_pdu_nr.h index 8a3af9962..1ff74110d 100644 --- a/lib/include/srsran/mac/mac_sch_pdu_nr.h +++ b/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) - std::array ce_write_buffer; // Buffer for CE payload + /// This helper class manages a SDU pointer that can point to either a user provided external buffer or to a small + /// 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 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; }; diff --git a/lib/src/mac/mac_sch_pdu_nr.cc b/lib/src/mac/mac_sch_pdu_nr.cc index d42d8a437..432f9e1ea 100644 --- a/lib/src/mac/mac_sch_pdu_nr.cc +++ b/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_; - sdu = const_cast(payload_); + lcid = lcid_; + sdu.set_storage_to(const_cast(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; - header_length = 1; - sdu_length = sizeof_ce(lcid, parent->is_ulsch()); - sdu = ce_write_buffer.data(); - uint16_t crnti = htole16(crnti_); - ce_write_buffer.at(0) = (uint8_t)((crnti & 0xff00) >> 8); - ce_write_buffer.at(1) = (uint8_t)((crnti & 0x00ff)); + lcid = CRNTI; + header_length = 1; + sdu_length = sizeof_ce(lcid, parent->is_ulsch()); + uint16_t crnti = htole16(crnti_); + uint8_t* ptr = sdu.use_internal_storage(); + ptr[0] = (uint8_t)((crnti & 0xff00) >> 8); + 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; - header_length = 1; - sdu_length = sizeof_ce(lcid, parent->is_ulsch()); - sdu = ce_write_buffer.data(); - ce_write_buffer.at(0) = (uint8_t)(phr_ & 0x3f); - ce_write_buffer.at(1) = (uint8_t)(pcmax_ & 0x3f); + lcid = SE_PHR; + header_length = 1; + sdu_length = sizeof_ce(lcid, parent->is_ulsch()); + uint8_t* ptr = sdu.use_internal_storage(); + ptr[0] = (uint8_t)(phr_ & 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; - header_length = 1; - sdu_length = sizeof_ce(lcid, parent->is_ulsch()); - sdu = ce_write_buffer.data(); - ce_write_buffer.at(0) = ((bsr_.lcg_id & 0x07) << 5) | (bsr_.buffer_size & 0x1f); + lcid = SHORT_BSR; + header_length = 1; + sdu_length = sizeof_ce(lcid, parent->is_ulsch()); + uint8_t* ptr = sdu.use_internal_storage(); + 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; - ta.ta_command = sdu[0] & 0x3f; + uint8_t* ptr = sdu.ptr(); + 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; - sbsr.buffer_size = sdu[0] & 0x1f; + uint8_t* ptr = sdu.ptr(); + sbsr.lcg_id = (ptr[0] & 0xe0) >> 5; + sbsr.buffer_size = ptr[0] & 0x1f; } return sbsr; }