improve error handling in PDU packing

lib/include/srslte/common/pdu.h

@@ -79,11 +79,11 @@ public:
 
   bool new_subh()
   {
-    if (nof_subheaders < (int)max_subheaders - 1 && rem_len > 0) {
+    if (nof_subheaders < (int)max_subheaders - 1 && rem_len > 0 && buffer_tx->get_headroom() > 1) {
       nof_subheaders++;
       return next();
     } else {
-      return -1;
+      return false;
     }
   }
 
@@ -112,7 +112,7 @@ public:
     if (cur_idx >= 0) {
       return &subheaders[cur_idx];
     } else {
-      return NULL;
+      return nullptr;
     }
   }
 

lib/src/common/pdu.cc

@@ -72,26 +72,29 @@ uint8_t* sch_pdu::write_packet(srslte::log* log_h)
 {
   if (nof_subheaders <= 0 && nof_subheaders < (int)max_subheaders) {
     log_h->error("Trying to write packet with invalid number of subheaders (nof_subheaders=%d).\n", nof_subheaders);
-    return NULL;
+    return nullptr;
   }
 
+  // set padding to remaining length in PDU
+  uint32_t num_padding = rem_len;
+
   // Determine if we are transmitting CEs only
   bool ce_only = last_sdu_idx < 0 ? true : false;
 
   // Determine if we will need multi-byte padding or 1/2 bytes padding
   bool     multibyte_padding = false;
   uint32_t onetwo_padding    = 0;
-  if (rem_len > 2) {
+  if (num_padding > 2) {
     multibyte_padding = true;
     // Add 1 header for padding
-    rem_len--;
+    num_padding--;
     // Add the header for the last SDU
     if (!ce_only) {
-      rem_len -= (subheaders[last_sdu_idx].get_header_size(false) - 1); // Because we were assuming it was the one
+      num_padding -= (subheaders[last_sdu_idx].get_header_size(false) - 1); // Because we were assuming it was the one
     }
-  } else if (rem_len > 0) {
+  } else if (num_padding > 0) {
-    onetwo_padding = rem_len;
+    onetwo_padding = num_padding;
-    rem_len        = 0;
+    num_padding    = 0;
   }
 
   // Determine the header size and CE payload size
@@ -111,6 +114,12 @@ uint8_t* sch_pdu::write_packet(srslte::log* log_h)
 
   uint32_t total_header_size = header_sz + ce_payload_sz;
 
+  // make sure there is enough room for header
+  if (buffer_tx->get_headroom() < total_header_size) {
+    log_h->error("Not enough headroom for MAC header (%d < %d).\n", buffer_tx->get_headroom(), total_header_size);
+    return nullptr;
+  }
+
   // Rewind PDU pointer and leave space for entire header
   buffer_tx->msg -= total_header_size;
   buffer_tx->N_bytes += total_header_size;
@@ -143,7 +152,7 @@ uint8_t* sch_pdu::write_packet(srslte::log* log_h)
   // and finally add multi-byte padding
   if (multibyte_padding) {
     sch_subh padding_multi;
-    padding_multi.set_padding(rem_len);
+    padding_multi.set_padding(num_padding);
     padding_multi.write_subheader(&ptr, true);
   }
 
@@ -153,10 +162,16 @@ uint8_t* sch_pdu::write_packet(srslte::log* log_h)
       subheaders[i].write_payload(&ptr);
     }
   }
+
+  if (buffer_tx->get_tailroom() < num_padding) {
+    log_h->error("Not enough tailroom for MAC padding (%d < %d).\n", buffer_tx->get_tailroom(), num_padding);
+    return nullptr;
+  }
+
   // Set padding to zeros (if any)
-  if (rem_len > 0) {
+  if (num_padding > 0) {
-    bzero(&buffer_tx->msg[total_header_size + total_sdu_len], rem_len * sizeof(uint8_t));
+    bzero(&buffer_tx->msg[total_header_size + total_sdu_len], num_padding * sizeof(uint8_t));
-    buffer_tx->N_bytes += rem_len;
+    buffer_tx->N_bytes += num_padding;
   }
 
   /* Sanity check and print if error */
@@ -171,7 +186,7 @@ uint8_t* sch_pdu::write_packet(srslte::log* log_h)
         last_sdu_idx,
         total_sdu_len,
         onetwo_padding,
-        rem_len);
+        num_padding);
   } else {
     printf("Wrote PDU: pdu_len=%d, header_and_ce=%d (%d+%d), nof_subh=%d, last_sdu=%d, sdu_len=%d, onepad=%d, "
            "multi=%d\n",
@@ -183,10 +198,10 @@ uint8_t* sch_pdu::write_packet(srslte::log* log_h)
            last_sdu_idx,
            total_sdu_len,
            onetwo_padding,
-           rem_len);
+           num_padding);
   }
 
-  if (rem_len + header_sz + ce_payload_sz + total_sdu_len != pdu_len) {
+  if (total_header_size + total_sdu_len + num_padding != pdu_len) {
     if (log_h) {
       log_h->console("\n------------------------------\n");
       for (int i = 0; i < nof_subheaders; i++) {
@@ -202,7 +217,7 @@ uint8_t* sch_pdu::write_packet(srslte::log* log_h)
                      last_sdu_idx,
                      total_sdu_len,
                      onetwo_padding,
-                     rem_len);
+                     num_padding);
       ERROR("Expected PDU len %d bytes but wrote %d\n", pdu_len, rem_len + header_sz + ce_payload_sz + total_sdu_len);
       log_h->console("------------------------------\n");
 
@@ -216,10 +231,10 @@ uint8_t* sch_pdu::write_packet(srslte::log* log_h)
                    last_sdu_idx,
                    total_sdu_len,
                    onetwo_padding,
-                   rem_len);
+                   num_padding);
     }
 
-    return NULL;
+    return nullptr;
   }
 
   return buffer_tx->msg;
@@ -293,12 +308,13 @@ bool sch_pdu::update_space_sdu(uint32_t nbytes)
 
 int sch_pdu::get_sdu_space()
 {
-  int ret;
+  int ret = 0;
   if (last_sdu_idx < 0) {
     ret = rem_len - 1;
   } else {
     ret = rem_len - (size_header_sdu(subheaders[last_sdu_idx].get_payload_size()) - 1) - 1;
   }
+  ret = SRSLTE_MIN(ret >= 0 ? ret : 0, buffer_tx->get_tailroom());
   return ret;
 }
 
@@ -630,7 +646,7 @@ int sch_subh::set_sdu(uint32_t lcid_, uint32_t requested_bytes_, read_pdu_interf
     int sdu_sz = sdu_itf_->read_pdu(lcid, payload, requested_bytes_);
 
     if (sdu_sz < 0) {
-      return -1;
+      return SRSLTE_ERROR;
     }
     if (sdu_sz == 0) {
       return 0;
@@ -639,7 +655,7 @@ int sch_subh::set_sdu(uint32_t lcid_, uint32_t requested_bytes_, read_pdu_interf
       nof_bytes = sdu_sz;
 
       if (nof_bytes > (int32_t)requested_bytes_) {
-        return -1;
+        return SRSLTE_ERROR;
       }
     }
 
@@ -647,7 +663,7 @@ int sch_subh::set_sdu(uint32_t lcid_, uint32_t requested_bytes_, read_pdu_interf
     ((sch_pdu*)parent)->update_space_sdu(nof_bytes);
     return nof_bytes;
   } else {
-    return -1;
+    return SRSLTE_ERROR;
   }
 }
 

lib/test/common/pdu_test.cc

@@ -361,6 +361,66 @@ int mac_sch_pdu_pack_test3()
   return SRSLTE_SUCCESS;
 }
 
+// Test for checking error cases
+int mac_sch_pdu_pack_error_test()
+{
+  srslte::log_filter rlc_log("RLC");
+  rlc_log.set_level(srslte::LOG_LEVEL_DEBUG);
+  rlc_log.set_hex_limit(100000);
+
+  rlc_dummy rlc;
+
+  srslte::log_filter mac_log("MAC");
+  mac_log.set_level(srslte::LOG_LEVEL_DEBUG);
+  mac_log.set_hex_limit(100000);
+
+  // create RLC SDUs
+  rlc.write_sdu(1, 8);
+
+  const uint32_t  pdu_size = 150;
+  srslte::sch_pdu pdu(10);
+
+  byte_buffer_t buffer;
+  pdu.init_tx(&buffer, pdu_size, true);
+
+  TESTASSERT(pdu.rem_size() == pdu_size);
+  TESTASSERT(pdu.get_pdu_len() == pdu_size);
+  TESTASSERT(pdu.get_sdu_space() == pdu_size - 1);
+  TESTASSERT(pdu.get_current_sdu_ptr() == buffer.msg);
+
+  // set msg pointer almost to end of byte buffer
+  int buffer_space = buffer.get_tailroom();
+  buffer.msg += buffer_space - 2;
+
+  // subheader can be added
+  TESTASSERT(pdu.new_subh());
+
+  // adding SDU fails
+  TESTASSERT(pdu.get()->set_sdu(1, 8, &rlc) == SRSLTE_ERROR);
+
+  // writing PDU fails
+  TESTASSERT(pdu.write_packet(&mac_log) == nullptr);
+
+  // reset buffer
+  buffer.clear();
+
+  // write SDU again
+  TESTASSERT(pdu.get() != nullptr);
+  TESTASSERT(pdu.get()->set_sdu(1, 100, &rlc) == 8); // only 8 bytes in RLC buffer
+
+  // writing PDU fails
+  TESTASSERT(pdu.write_packet(&mac_log));
+
+  // log
+  mac_log.info_hex(buffer.msg, buffer.N_bytes, "MAC PDU (%d B):\n", buffer.N_bytes);
+
+#if HAVE_PCAP
+  pcap_handle->write_ul_crnti(buffer.msg, buffer.N_bytes, 0x1001, true, 1);
+#endif
+
+  return SRSLTE_SUCCESS;
+}
+
 int main(int argc, char** argv)
 {
 #if HAVE_PCAP
@@ -403,5 +463,10 @@ int main(int argc, char** argv)
     return SRSLTE_ERROR;
   }
 
+  if (mac_sch_pdu_pack_error_test()) {
+    fprintf(stderr, "mac_sch_pdu_pack_error_test failed.\n");
+    return SRSLTE_ERROR;
+  }
+
   return SRSLTE_SUCCESS;
 }