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

2 files changed, 19 insertions, 6 deletions

diff --git a/tests/tbf/TbfTest.cpp b/tests/tbf/TbfTest.cpp
index 5c41b53b..45dbfc46 100644
--- a/tests/tbf/TbfTest.cpp
+++ b/tests/tbf/TbfTest.cpp
@@ -34,6 +34,13 @@ extern "C" {
 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 @@ static void test_tbf_final_ack(enum test_tbf_final_ack_mode test_mode)
 	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 @@ static void test_tbf_final_ack(enum test_tbf_final_ack_mode test_mode)
 	/* 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 9d9e5f76..ca522ecb 100644
--- a/tests/tbf/TbfTest.err
+++ b/tests/tbf/TbfTest.err
@@ -72,10 +72,12 @@ Slot Allocation (Algorithm A) for class 45
 - 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 @@ Slot Allocation (Algorithm A) for class 45
 - 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 **********