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
Thu Mar 28 17:55:30 UTC 2019


Hello Neels Hofmeyr, Harald Welte, Jenkins Builder, 

I'd like you to reexamine a change. Please visit

    https://gerrit.osmocom.org/13450

to look at the new patch set (#2).

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 a test case basically disconnects too quickly,
leaving some MT SMS messages (e.g. delivery reports) undelivered,
so they are 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, 21 insertions(+), 8 deletions(-)


  git pull ssh://gerrit.osmocom.org:29418/osmo-msc refs/changes/50/13450/2
-- 
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: newpatchset
Gerrit-Change-Id: Iad5e4d84d8d410ea43d5907e9ddf6e5fdb55bc7a
Gerrit-Change-Number: 13450
Gerrit-PatchSet: 2
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>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20190328/d96b9452/attachment.html>


More information about the gerrit-log mailing list