From 96262a7ca67115482cfb89f9954413638b1d5dcc Mon Sep 17 00:00:00 2001 From: Vadim Yanitskiy Date: Thu, 28 Mar 2019 21:25:14 +0700 Subject: [PATCH] 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 --- src/libmsc/sms_queue.c | 7 +++++- tests/sms_queue/sms_queue_test.c | 39 +++++++++++++++++++++++++------- 2 files changed, 37 insertions(+), 9 deletions(-) diff --git a/src/libmsc/sms_queue.c b/src/libmsc/sms_queue.c index c924ddecf..274c71295 100644 --- a/src/libmsc/sms_queue.c +++ b/src/libmsc/sms_queue.c @@ -226,8 +226,13 @@ struct gsm_sms *smsq_take_next_sms(struct gsm_network *net, 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 e4263778a..f64f715db 100644 --- a/tests/sms_queue/sms_queue_test.c +++ b/tests/sms_queue/sms_queue_test.c @@ -26,8 +26,10 @@ #include #include #include +#include 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 @@ static void _test_take_next_sms_print(int i, 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 @@ struct gsm_sms *__wrap_db_sms_get_next_unsent_rr_msisdn(struct gsm_network *net, 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 @@ struct gsm_sms *__wrap_db_sms_get_next_unsent_rr_msisdn(struct gsm_network *net, 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 @@ void show_fake_sms_db() 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 @@ static void test_next_sms() 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 @@ static void test_next_sms() 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 @@ int main(int argc, char **argv) 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);