Change in osmo-msc[master]: libmsc/sms_queue.c: fix memleak in smsq_take_next_sms()

Vadim Yanitskiy gerrit-no-reply at lists.osmocom.org
Mon Apr 1 12:02:58 UTC 2019


Vadim Yanitskiy has submitted this change and it was merged. ( https://gerrit.osmocom.org/13450 )

Change subject: libmsc/sms_queue.c: fix memleak in smsq_take_next_sms()
......................................................................

libmsc/sms_queue.c: fix memleak in smsq_take_next_sms()

A memleak has been noticed after executing some of TTCN-3 test
cases. For example, the following ones:

  - MSC_Tests.TC_lu_and_mo_sms,
  - MSC_Tests.TC_lu_and_mt_sms.

The key point is that MSC_Tests.TC_lu_and_mo_sms basically sends
a MO SMS to a non-attached subscriber with MSISDN 12345, so this
message is getting stored in the SMSC's database.

As soon as the SMSC's queue is triggered, sms_submit_pending() would
retrieve pending messages from the database by calling function
smsq_take_next_sms() in loop and attempt to deliver them.

This function in it's turn checks whether the subscriber is attached
or not. If not, the allocated 'gsm_sms' structure would not be
free()ed! Therefore, every time smsq_take_next_sms() is called,
one 'gsm_sms' structure for an unattached subscriber is leaked.

Furthermore, there is a unit test called 'sms_queue_test', that
actually does cover smsq_take_next_sms() and was designed to
catch some potential memory leaks, but...

In order to avoid emulating the low-level SQLite API, the unit
test by design overwrites some functions of libmsc, including
db_sms_get_next_unsent_rr_msisdn(), that is being called by
smsq_take_next_sms().

The problem is that the original function in libmsc does
allocate a 'gsm_sms' structure on heap (using talloc), while
the overwriting function did this statically, returning a
pointer to stack. This critical difference made it impossible
to spot the memleak in smsq_take_next_sms() during the
unit test execution.

Let's refactor 'sms_queue_test' to use dynamic memory allocation,
and finally fix the evil memleak in smsq_take_next_sms().

Change-Id: Iad5e4d84d8d410ea43d5907e9ddf6e5fdb55bc7a
Closes: OS#3860
---
M src/libmsc/sms_queue.c
M tests/sms_queue/sms_queue_test.c
2 files changed, 37 insertions(+), 9 deletions(-)

Approvals:
  Harald Welte: Looks good to me, approved
  Neels Hofmeyr: Looks good to me, but someone else must approve
  Jenkins Builder: Verified



diff --git a/src/libmsc/sms_queue.c b/src/libmsc/sms_queue.c
index c924dde..274c712 100644
--- a/src/libmsc/sms_queue.c
+++ b/src/libmsc/sms_queue.c
@@ -226,8 +226,13 @@
 		osmo_strlcpy(last_msisdn, sms->dst.addr, last_msisdn_buflen);
 
 		/* Is the subscriber attached? If not, go to next SMS */
-		if (!sms->receiver || !sms->receiver->lu_complete)
+		if (!sms->receiver || !sms->receiver->lu_complete) {
+			LOGP(DLSMS, LOGL_DEBUG,
+			     "Subscriber %s is not attached, skipping SMS %llu\n",
+			     vlr_subscr_msisdn_or_name(sms->receiver), sms->id);
+			sms_free(sms);
 			continue;
+		}
 
 		return sms;
 	}
diff --git a/tests/sms_queue/sms_queue_test.c b/tests/sms_queue/sms_queue_test.c
index e426377..f64f715 100644
--- a/tests/sms_queue/sms_queue_test.c
+++ b/tests/sms_queue/sms_queue_test.c
@@ -26,8 +26,10 @@
 #include <osmocom/msc/debug.h>
 #include <osmocom/msc/vlr.h>
 #include <osmocom/msc/gsm_data.h>
+#include <osmocom/msc/gsm_04_11.h>
 
 static void *talloc_ctx = NULL;
+extern void *tall_gsms_ctx;
 
 struct gsm_sms *smsq_take_next_sms(struct gsm_network *net,
 				   char *last_msisdn,
@@ -45,8 +47,6 @@
 	printf(" (last_msisdn='%s')\n", last_msisdn? last_msisdn : "NULL");
 }
 
-static struct gsm_sms fake_sms = { 0 };
-
 struct {
 	const char *msisdn;
 	int nr_of_sms;
@@ -91,11 +91,19 @@
 							const char *last_msisdn,
 							unsigned int max_failed)
 {
-	static struct vlr_subscr arbitrary_vsub = { .lu_complete = true };
+	static struct vlr_subscr arbitrary_vsub;
+	struct gsm_sms *sms;
 	int i;
 	printf("     hitting database: looking for MSISDN > '%s', failed_attempts <= %d\n",
 	       last_msisdn, max_failed);
 
+	/* Every time we call sms_free(), the internal logic of libmsc
+	 * may call vlr_subscr_put() on our arbitrary_vsub, what would
+	 * lead to a segfault if its use_count <= 0. To prevent this,
+	 * let's ensure a big enough initial value. */
+	arbitrary_vsub.use_count = 1000;
+	arbitrary_vsub.lu_complete = true;
+
 	for (i = 0; i < ARRAY_SIZE(fake_sms_db); i++) {
 		if (!fake_sms_db[i].nr_of_sms)
 			continue;
@@ -103,14 +111,19 @@
 			continue;
 		if (fake_sms_db[i].failed_attempts > max_failed)
 			continue;
-		osmo_strlcpy(fake_sms.dst.addr, fake_sms_db[i].msisdn,
-			     sizeof(fake_sms.dst.addr));
-		fake_sms.receiver = fake_sms_db[i].vsub_attached? &arbitrary_vsub : NULL;
-		osmo_strlcpy(fake_sms.text, fake_sms_db[i].msisdn, sizeof(fake_sms.text));
+
+		sms = sms_alloc();
+		OSMO_ASSERT(sms);
+
+		osmo_strlcpy(sms->dst.addr, fake_sms_db[i].msisdn,
+			     sizeof(sms->dst.addr));
+		sms->receiver = fake_sms_db[i].vsub_attached? &arbitrary_vsub : NULL;
+		osmo_strlcpy(sms->text, fake_sms_db[i].msisdn, sizeof(sms->text));
 		if (fake_sms_db[i].vsub_attached)
 			fake_sms_db[i].nr_of_sms--;
-		return &fake_sms;
+		return sms;
 	}
+
 	return NULL;
 }
 
@@ -127,6 +140,10 @@
 	printf("-->\n");
 }
 
+/* sms_free() is not safe against NULL */
+#define sms_free_safe(sms) \
+	if (sms != NULL) sms_free(sms)
+
 static void test_next_sms()
 {
 	int i;
@@ -141,6 +158,7 @@
 		struct gsm_sms *sms = smsq_take_next_sms(NULL, last_msisdn, sizeof(last_msisdn));
 		_test_take_next_sms_print(i, sms, last_msisdn);
 		OSMO_ASSERT(i >= 4 || sms);
+		sms_free_safe(sms);
 	}
 
 	printf("\n- SMS are pending at various nr failed attempts (cutoff at >= 10)\n");
@@ -156,6 +174,7 @@
 		struct gsm_sms *sms = smsq_take_next_sms(NULL, last_msisdn, sizeof(last_msisdn));
 		_test_take_next_sms_print(i, sms, last_msisdn);
 		OSMO_ASSERT(i >= 2 || sms);
+		sms_free_safe(sms);
 	}
 
 	printf("\n- iterate the SMS DB at most once\n");
@@ -206,6 +225,10 @@
 	logging_ctx = talloc_named_const(talloc_ctx, 0, "logging");
 	osmo_init_logging2(logging_ctx, &info);
 
+	/* Share our talloc context with libmsc's GSM 04.11 code,
+	 * so sms_alloc() would use it instead of NULL. */
+	tall_gsms_ctx = talloc_ctx;
+
 	OSMO_ASSERT(osmo_stderr_target);
 	log_set_use_color(osmo_stderr_target, 0);
 	log_set_print_timestamp(osmo_stderr_target, 0);

-- 
To view, visit https://gerrit.osmocom.org/13450
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: Iad5e4d84d8d410ea43d5907e9ddf6e5fdb55bc7a
Gerrit-Change-Number: 13450
Gerrit-PatchSet: 5
Gerrit-Owner: Vadim Yanitskiy <axilirator at gmail.com>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder (1000002)
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Pau Espin Pedrol <pespin at sysmocom.de>
Gerrit-Reviewer: Vadim Yanitskiy <axilirator at gmail.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20190401/8b68a08c/attachment.html>


More information about the gerrit-log mailing list