tbf: Fix dangling m_new_tbf pointer
Currently if a 'new' TBF is freed before the 'old' one (where
old_tbf->m_new_tbf == new_tbf), the old_tbf->m_new_tbf is not cleared
and can be accessed later on. This can lead to inconsistencies or
segmentation faults.
This commit adds m_old_tbf which points back from new_tbf to old_pdf.
m_new_tbf and m_old_tbf are either both set to NULL or one is the
reverse pointer of the other (tbf->m_new_tbf->m_old_tbf == tbf and
tbf->m_old_tbf->m_new_tbf == tbf). It extends set_new_tbf and
tbf_free to update the pointee accordingly.
The TBF test is extended to check this invariant at several places.
Sponsored-by: On-Waves ehf
diff --git a/src/tbf.cpp b/src/tbf.cpp
index 680a096..05b6140 100644
--- a/src/tbf.cpp
+++ b/src/tbf.cpp
@@ -53,7 +53,22 @@
void gprs_rlcmac_tbf::set_new_tbf(gprs_rlcmac_tbf *tbf)
{
+ if (m_new_tbf) {
+ if (m_new_tbf == tbf) {
+ LOGP(DRLCMAC, LOGL_NOTICE,
+ "%s reassigning %s to m_new_tbf\n",
+ tbf_name(this), tbf_name(tbf));
+ return;
+ }
+ LOGP(DRLCMAC, LOGL_NOTICE,
+ "%s m_new_tbf is already assigned to %s, "
+ "overwriting the old value with %s\n",
+ tbf_name(this), tbf_name(m_new_tbf), tbf_name(tbf));
+ /* Detach from other TBF */
+ m_new_tbf->m_old_tbf = NULL;
+ }
m_new_tbf = tbf;
+ tbf->m_old_tbf = this;
}
gprs_rlcmac_ul_tbf *tbf_alloc_ul(struct gprs_rlcmac_bts *bts,
@@ -138,6 +153,28 @@
else
tbf->bts->tbf_dl_freed();
+ if (tbf->m_old_tbf) {
+ LOGP(DRLCMAC, LOGL_INFO, "%s Old TBF %s still exists, detaching\n",
+ tbf_name(tbf), tbf_name(tbf->m_old_tbf));
+ if (tbf->m_old_tbf->m_new_tbf == tbf)
+ tbf->m_old_tbf->m_new_tbf = NULL;
+ else
+ LOGP(DRLCMAC, LOGL_ERROR, "%s Software error: "
+ "tbf->m_old_tbf->m_new_tbf != tbf\n",
+ tbf_name(tbf));
+ }
+
+ if (tbf->m_new_tbf) {
+ LOGP(DRLCMAC, LOGL_INFO, "%s New TBF %s still exists, detaching\n",
+ tbf_name(tbf), tbf_name(tbf->m_new_tbf));
+ if (tbf->m_new_tbf->m_old_tbf == tbf)
+ tbf->m_new_tbf->m_old_tbf = NULL;
+ else
+ LOGP(DRLCMAC, LOGL_ERROR, "%s Software error: "
+ "tbf->m_new_tbf->m_old_tbf != tbf\n",
+ tbf_name(tbf));
+ }
+
LOGP(DRLCMAC, LOGL_DEBUG, "********** TBF ends here **********\n");
talloc_free(tbf);
}
diff --git a/src/tbf.h b/src/tbf.h
index 69f1f05..1bea31d 100644
--- a/src/tbf.h
+++ b/src/tbf.h
@@ -169,6 +169,7 @@
enum gprs_rlcmac_tbf_ul_ack_state ul_ack_state;
gprs_rlcmac_tbf *m_new_tbf;
+ gprs_rlcmac_tbf *m_old_tbf; /* reverse pointer for m_new_tbf */
enum gprs_rlcmac_tbf_poll_state poll_state;
uint32_t poll_fn; /* frame number to poll */
diff --git a/tests/tbf/TbfTest.cpp b/tests/tbf/TbfTest.cpp
index 5c41b53..45dbfc4 100644
--- a/tests/tbf/TbfTest.cpp
+++ b/tests/tbf/TbfTest.cpp
@@ -34,6 +34,13 @@
void *tall_pcu_ctx;
int16_t spoof_mnc = 0, spoof_mcc = 0;
+static void check_tbf(gprs_rlcmac_tbf *tbf)
+{
+ OSMO_ASSERT(tbf);
+ OSMO_ASSERT(tbf->m_new_tbf == NULL || tbf->m_new_tbf->m_old_tbf == tbf);
+ OSMO_ASSERT(tbf->m_old_tbf == NULL || tbf->m_old_tbf->m_new_tbf == tbf);
+}
+
static void test_tbf_tlli_update()
{
BTS the_bts;
@@ -118,13 +125,15 @@
tfi = the_bts.tfi_find_free(GPRS_RLCMAC_DL_TBF, &trx_no, -1);
OSMO_ASSERT(tfi >= 0);
dl_tbf = tbf_alloc_dl_tbf(bts, NULL, tfi, trx_no, ms_class, 1);
- OSMO_ASSERT(dl_tbf);
+ check_tbf(dl_tbf);
+
/* "Establish" the DL TBF */
dl_tbf->dl_ass_state = GPRS_RLCMAC_DL_ASS_SEND_ASS;
dl_tbf->set_state(GPRS_RLCMAC_FLOW);
dl_tbf->m_wait_confirm = 0;
dl_tbf->set_new_tbf(dl_tbf);
+ check_tbf(dl_tbf);
for (i = 0; i < sizeof(llc_data); i++)
llc_data[i] = i%256;
@@ -149,19 +158,20 @@
/* Clean up and ensure tbfs are in the correct state */
OSMO_ASSERT(dl_tbf->state_is(GPRS_RLCMAC_WAIT_RELEASE));
new_tbf = dl_tbf->new_tbf();
+ check_tbf(new_tbf);
OSMO_ASSERT(new_tbf != dl_tbf);
OSMO_ASSERT(new_tbf->tfi() == 1);
+ check_tbf(dl_tbf);
dl_tbf->dl_ass_state = GPRS_RLCMAC_DL_ASS_NONE;
if (test_mode == TEST_MODE_REVERSE_FREE) {
tbf_free(new_tbf);
- if (dl_tbf->m_new_tbf == new_tbf)
- fprintf(stderr, "dangling m_new_tbf pointer in dl_tbf "
- "(known bug)\n");
- /* OSMO_ASSERT(dl_tbf->m_new_tbf != new_tbf); */
+ OSMO_ASSERT(dl_tbf->m_new_tbf != new_tbf);
+ check_tbf(dl_tbf);
tbf_free(dl_tbf);
} else {
tbf_free(dl_tbf);
OSMO_ASSERT(new_tbf->m_new_tbf != dl_tbf);
+ check_tbf(new_tbf);
tbf_free(new_tbf);
}
}
diff --git a/tests/tbf/TbfTest.err b/tests/tbf/TbfTest.err
index 9d9e5f7..ca522ec 100644
--- a/tests/tbf/TbfTest.err
+++ b/tests/tbf/TbfTest.err
@@ -72,10 +72,12 @@
- Assign downlink TS=4
TBF(TFI=0 TLLI=0x00000000 DIR=DL STATE=WAIT RELEASE) Trigger dowlink assignment on PACCH, because another LLC PDU has arrived in between
Send dowlink assignment on PACCH, because TBF(TFI=0 TLLI=0x00000000 DIR=DL STATE=WAIT RELEASE) exists
+TBF(TFI=0 TLLI=0x00000000 DIR=DL STATE=WAIT RELEASE) m_new_tbf is already assigned to TBF(TFI=0 TLLI=0x00000000 DIR=DL STATE=WAIT RELEASE), overwriting the old value with TBF(TFI=0 TLLI=0x00000000 DIR=DL STATE=WAIT RELEASE)
TBF(TFI=1 TLLI=0x00000000 DIR=DL STATE=NULL) changes state from NULL to ASSIGN
TBF(TFI=1 TLLI=0x00000000 DIR=DL STATE=ASSIGN) starting timer 0.
DL packet loss of IMSI= / TLLI=0x00000000: 0%
TBF(TFI=0 TLLI=0x00000000 DIR=DL STATE=WAIT RELEASE) free
+TBF(TFI=0 TLLI=0x00000000 DIR=DL STATE=WAIT RELEASE) New TBF TBF(TFI=0 TLLI=0x00000000 DIR=DL STATE=WAIT RELEASE) still exists, detaching
********** TBF ends here **********
TBF(TFI=1 TLLI=0x00000000 DIR=DL STATE=ASSIGN) free
TBF(TFI=1 TLLI=0x00000000 DIR=DL STATE=ASSIGN) stopping timer 0.
@@ -137,12 +139,13 @@
- Assign downlink TS=4
TBF(TFI=0 TLLI=0x00000000 DIR=DL STATE=WAIT RELEASE) Trigger dowlink assignment on PACCH, because another LLC PDU has arrived in between
Send dowlink assignment on PACCH, because TBF(TFI=0 TLLI=0x00000000 DIR=DL STATE=WAIT RELEASE) exists
+TBF(TFI=0 TLLI=0x00000000 DIR=DL STATE=WAIT RELEASE) m_new_tbf is already assigned to TBF(TFI=0 TLLI=0x00000000 DIR=DL STATE=WAIT RELEASE), overwriting the old value with TBF(TFI=0 TLLI=0x00000000 DIR=DL STATE=WAIT RELEASE)
TBF(TFI=1 TLLI=0x00000000 DIR=DL STATE=NULL) changes state from NULL to ASSIGN
TBF(TFI=1 TLLI=0x00000000 DIR=DL STATE=ASSIGN) starting timer 0.
TBF(TFI=1 TLLI=0x00000000 DIR=DL STATE=ASSIGN) free
TBF(TFI=1 TLLI=0x00000000 DIR=DL STATE=ASSIGN) stopping timer 0.
+TBF(TFI=1 TLLI=0x00000000 DIR=DL STATE=ASSIGN) Old TBF TBF(TFI=1 TLLI=0x00000000 DIR=DL STATE=ASSIGN) still exists, detaching
********** TBF ends here **********
-dangling m_new_tbf pointer in dl_tbf (known bug)
DL packet loss of IMSI= / TLLI=0x00000000: 0%
TBF(TFI=0 TLLI=0x00000000 DIR=DL STATE=WAIT RELEASE) free
********** TBF ends here **********