From b6d7fd5def8e84b70420a2445a394fab65293b75 Mon Sep 17 00:00:00 2001 From: Andre Puschmann Date: Wed, 19 Jun 2019 18:51:03 +0200 Subject: [PATCH] improve error handling in PDU packing --- lib/include/srslte/common/pdu.h | 6 +-- lib/src/common/pdu.cc | 58 ++++++++++++++++++----------- lib/test/common/pdu_test.cc | 65 +++++++++++++++++++++++++++++++++ 3 files changed, 105 insertions(+), 24 deletions(-) diff --git a/lib/include/srslte/common/pdu.h b/lib/include/srslte/common/pdu.h index 5b58c5e9c..6dec794de 100644 --- a/lib/include/srslte/common/pdu.h +++ b/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; } } diff --git a/lib/src/common/pdu.cc b/lib/src/common/pdu.cc index ff0402d96..162240113 100644 --- a/lib/src/common/pdu.cc +++ b/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) { - onetwo_padding = rem_len; - rem_len = 0; + } else if (num_padding > 0) { + onetwo_padding = num_padding; + 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) { - bzero(&buffer_tx->msg[total_header_size + total_sdu_len], rem_len * sizeof(uint8_t)); - buffer_tx->N_bytes += rem_len; + if (num_padding > 0) { + bzero(&buffer_tx->msg[total_header_size + total_sdu_len], num_padding * sizeof(uint8_t)); + 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; } } diff --git a/lib/test/common/pdu_test.cc b/lib/test/common/pdu_test.cc index 858bac123..c9c89034c 100644 --- a/lib/test/common/pdu_test.cc +++ b/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; }