[MERGED] libosmocore[master]: gprs_bssgp: bssgp_fc_in(): fix mem leak on queue overflow

[Neels Hofmeyr]

Neels Hofmeyr has submitted this change and it was merged.

Change subject: gprs_bssgp: bssgp_fc_in(): fix mem leak on queue overflow
......................................................................


gprs_bssgp: bssgp_fc_in(): fix mem leak on queue overflow

All successful and all error code paths of bssgp_fc_in() free the msgb, except
the code path calling fc_enqueue() when the msg is dropped (due to queue being
full, or failure to allocate).

Callers could theoretically catch the -ENOSPC return value and discard the
msgb. However, in other code paths, a callback's return value is returned,
which is expected to free the msgb, so such callback would have to never return
-ENOSPC when it freed the msgb. Much simpler semantics would be to free the
msgb in every code path, no matter which kind of error occurred.

Who is currently calling bssgp_fc_in and how do they handle the return value?
- bssgp_fc_test.c ignores the return value (and hits a mem leak aka sanitizer
  build failure if the queue is full).
- fc_timer_cb() ignores the return value.
- bssgp_tx_dl_ud() returns the bssgp_fc_in() rc.
  - which is returned by a cascade of functions leading up to being returned,
    for example, by gprs_llgmm_reset(), which is usually called with ignored
    return code.
At this point it is already fairly clear that bssgp_fc_in() should always free
the msgb, since the callers don't seem to distinguish even between error or
success, let alone between -ENOSPC or other errors.

bssgp_fc_test: assert that no msgbs remain unfreed after the tests.
Adjust expected results.

Helps fix sanitizer build on debian 9.

Change-Id: I00c62a104baeaad6a85883c380259c469aebf0df
---
M src/gb/gprs_bssgp.c
M tests/gb/bssgp_fc_test.c
M tests/gb/bssgp_fc_tests.ok
3 files changed, 9 insertions(+), 4 deletions(-)

Approvals:
  Harald Welte: Looks good to me, approved



diff --git a/src/gb/gprs_bssgp.c b/src/gb/gprs_bssgp.c
index 520868e..d27a94f 100644
--- a/src/gb/gprs_bssgp.c
+++ b/src/gb/gprs_bssgp.c
@@ -764,7 +764,11 @@
 	}
 
 	if (bssgp_fc_needs_queueing(fc, llc_pdu_len)) {
-		return fc_enqueue(fc, msg, llc_pdu_len, priv);
+		int rc;
+		rc = fc_enqueue(fc, msg, llc_pdu_len, priv);
+		if (rc)
+			msgb_free(msg);
+		return rc;
 	} else {
 		/* record the time we transmitted this PDU */
 		osmo_gettimeofday(&time_now, NULL);
diff --git a/tests/gb/bssgp_fc_test.c b/tests/gb/bssgp_fc_test.c
index 7a75a4d..98e17cc 100644
--- a/tests/gb/bssgp_fc_test.c
+++ b/tests/gb/bssgp_fc_test.c
@@ -190,7 +190,8 @@
 	printf("msgb ctx: %zu b in %zu blocks (0 b in 1 block == just the context)\n",
 	       talloc_total_size(tall_msgb_ctx),
 	       talloc_total_blocks(tall_msgb_ctx));
-	/* KNOWN BUG: expecting 0b in 1 block, but a full queue is still a mem leak */
+	OSMO_ASSERT(talloc_total_size(tall_msgb_ctx) == 0);
+	OSMO_ASSERT(talloc_total_blocks(tall_msgb_ctx) == 1);
 	talloc_free(tall_msgb_ctx);
 	printf("===== BSSGP flow-control test END\n\n");
 
diff --git a/tests/gb/bssgp_fc_tests.ok b/tests/gb/bssgp_fc_tests.ok
index d146843..30d9776 100644
--- a/tests/gb/bssgp_fc_tests.ok
+++ b/tests/gb/bssgp_fc_tests.ok
@@ -56,7 +56,7 @@
 30: FC OUT Nr 13
 40: FC OUT Nr 14
 50: FC OUT Nr 15
-msgb ctx: 685 b in 6 blocks (0 b in 1 block == just the context)
+msgb ctx: 0 b in 1 blocks (0 b in 1 block == just the context)
 ===== BSSGP flow-control test END
 
 ===== BSSGP flow-control test START
@@ -229,6 +229,6 @@
 30: FC OUT Nr 13
 40: FC OUT Nr 14
 50: FC OUT Nr 15
-msgb ctx: 685 b in 6 blocks (0 b in 1 block == just the context)
+msgb ctx: 0 b in 1 blocks (0 b in 1 block == just the context)
 ===== BSSGP flow-control test END
 

-- 
To view, visit https://gerrit.osmocom.org/4872
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: merged
Gerrit-Change-Id: I00c62a104baeaad6a85883c380259c469aebf0df
Gerrit-PatchSet: 3
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Owner: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder