From 555b2e5ac128211edffa34a586fe5f548eb3acba Mon Sep 17 00:00:00 2001 From: Jacob Erlbeck Date: Mon, 26 Jan 2015 13:52:42 +0100 Subject: sgsn: Don't allow mmctx == NULL in sgsn_update_subscriber_data Currently, sgsn_update_subscriber_data can be called with mmctx == NULL and will find and associate the right context (if present) based on the subscriber's IMSI. This will not happen in regular use any more, since sgsn_update_subscriber_data will only be called when subscribers are used (auth mode 'remote') and in this case gprs_subscr_get_or_create_by_mmctx will already be called by sgsn_auth_request. Therefore, MM context and subscriber are always associated except for some test cases and experimental VTY usage. The current implementation of sgsn_update_subscriber_data also causes additional complexity for the deletion on MM contexts to avoid a ipossible double-free MM contexts. This commit removes the MM context <-> subscriber association code from sgsn_update_subscriber_data. That function must always be called with mmctx != NULL, now. To avoid problems with VTY and test usage, the calling subscriber function now only call sgsn_update_subscriber_data when mmctx != NULL, since the purpose of that function is to update that state of an existing MM context after subscriber data has been changed. Sponsored-by: On-Waves ehf --- openbsc/include/openbsc/gprs_sgsn.h | 3 +-- openbsc/src/gprs/gprs_sgsn.c | 21 ++------------------- openbsc/src/gprs/gprs_subscriber.c | 6 ++++-- openbsc/tests/sgsn/sgsn_test.c | 21 +++++++++++---------- 4 files changed, 18 insertions(+), 33 deletions(-) diff --git a/openbsc/include/openbsc/gprs_sgsn.h b/openbsc/include/openbsc/gprs_sgsn.h index 533117a74..ce73e0189 100644 --- a/openbsc/include/openbsc/gprs_sgsn.h +++ b/openbsc/include/openbsc/gprs_sgsn.h @@ -339,8 +339,7 @@ int gprs_subscr_query_auth_info(struct gsm_subscriber *subscr); int gprs_subscr_location_update(struct gsm_subscriber *subscr); /* Called on subscriber data updates */ -void sgsn_update_subscriber_data(struct sgsn_mm_ctx *mmctx, - struct gsm_subscriber *subscr); +void sgsn_update_subscriber_data(struct sgsn_mm_ctx *mmctx); int gprs_sndcp_vty_init(void); struct sgsn_instance; diff --git a/openbsc/src/gprs/gprs_sgsn.c b/openbsc/src/gprs/gprs_sgsn.c index 781be3ffb..14b925400 100644 --- a/openbsc/src/gprs/gprs_sgsn.c +++ b/openbsc/src/gprs/gprs_sgsn.c @@ -487,28 +487,11 @@ int sgsn_force_reattach_oldmsg(struct msgb *oldmsg) return gsm0408_gprs_force_reattach_oldmsg(oldmsg); } -void sgsn_update_subscriber_data(struct sgsn_mm_ctx *mmctx, - struct gsm_subscriber *subscr) +void sgsn_update_subscriber_data(struct sgsn_mm_ctx *mmctx) { - if (!mmctx && subscr && strlen(subscr->imsi) > 0) { - mmctx = sgsn_mm_ctx_by_imsi(subscr->imsi); - OSMO_ASSERT(!mmctx || !mmctx->subscr || mmctx->subscr == subscr); - } - - if (!mmctx) { - LOGP(DMM, LOGL_INFO, - "Subscriber data update for unregistered MM context, IMSI %s\n", - subscr->imsi); - return; - } - + OSMO_ASSERT(mmctx != NULL); LOGMMCTXP(LOGL_INFO, mmctx, "Subscriber data update\n"); - if (!subscr->sgsn_data->mm && !mmctx->subscr) { - mmctx->subscr = subscr_get(subscr); - mmctx->subscr->sgsn_data->mm = mmctx; - } - sgsn_auth_update(mmctx); } diff --git a/openbsc/src/gprs/gprs_subscriber.c b/openbsc/src/gprs/gprs_subscriber.c index ee6c47786..ea5d1d8d6 100644 --- a/openbsc/src/gprs/gprs_subscriber.c +++ b/openbsc/src/gprs/gprs_subscriber.c @@ -614,7 +614,8 @@ void gprs_subscr_update(struct gsm_subscriber *subscr) subscr->flags &= ~GPRS_SUBSCRIBER_UPDATE_LOCATION_PENDING; subscr->flags &= ~GSM_SUBSCRIBER_FIRST_CONTACT; - sgsn_update_subscriber_data(subscr->sgsn_data->mm, subscr); + if (subscr->sgsn_data->mm) + sgsn_update_subscriber_data(subscr->sgsn_data->mm); } void gprs_subscr_update_auth_info(struct gsm_subscriber *subscr) @@ -625,7 +626,8 @@ void gprs_subscr_update_auth_info(struct gsm_subscriber *subscr) subscr->flags &= ~GPRS_SUBSCRIBER_UPDATE_AUTH_INFO_PENDING; subscr->flags &= ~GSM_SUBSCRIBER_FIRST_CONTACT; - sgsn_update_subscriber_data(subscr->sgsn_data->mm, subscr); + if (subscr->sgsn_data->mm) + sgsn_update_subscriber_data(subscr->sgsn_data->mm); } struct gsm_subscriber *gprs_subscr_get_or_create_by_mmctx(struct sgsn_mm_ctx *mmctx) diff --git a/openbsc/tests/sgsn/sgsn_test.c b/openbsc/tests/sgsn/sgsn_test.c index 5d142da90..da7da855f 100644 --- a/openbsc/tests/sgsn/sgsn_test.c +++ b/openbsc/tests/sgsn/sgsn_test.c @@ -61,14 +61,13 @@ int bssgp_tx_dl_ud(struct msgb *msg, uint16_t pdu_lifetime, } /* override, requires '-Wl,--wrap=sgsn_update_subscriber_data' */ -void __real_sgsn_update_subscriber_data(struct sgsn_mm_ctx *, struct gsm_subscriber *); -void (*update_subscriber_data_cb)(struct sgsn_mm_ctx *, struct gsm_subscriber *) = +void __real_sgsn_update_subscriber_data(struct sgsn_mm_ctx *); +void (*update_subscriber_data_cb)(struct sgsn_mm_ctx *) = &__real_sgsn_update_subscriber_data; -void __wrap_sgsn_update_subscriber_data(struct sgsn_mm_ctx *mmctx, - struct gsm_subscriber *subscr) +void __wrap_sgsn_update_subscriber_data(struct sgsn_mm_ctx *mmctx) { - (*update_subscriber_data_cb)(mmctx, subscr); + (*update_subscriber_data_cb)(mmctx); } /* override, requires '-Wl,--wrap=gprs_subscr_request_update_location' */ @@ -191,12 +190,12 @@ static void test_llme(void) } struct gsm_subscriber *last_updated_subscr = NULL; -void my_dummy_sgsn_update_subscriber_data(struct sgsn_mm_ctx *mmctx, - struct gsm_subscriber *subscr) +void my_dummy_sgsn_update_subscriber_data(struct sgsn_mm_ctx *mmctx) { + OSMO_ASSERT(mmctx); fprintf(stderr, "Called %s, mmctx = %p, subscr = %p\n", - __func__, mmctx, subscr); - last_updated_subscr = subscr; + __func__, mmctx, mmctx->subscr); + last_updated_subscr = mmctx->subscr; } static void assert_subscr(const struct gsm_subscriber *subscr, const char *imsi) @@ -266,7 +265,9 @@ static void test_subscriber(void) /* Update entry 1 */ last_updated_subscr = NULL; gprs_subscr_update(s1); - OSMO_ASSERT(last_updated_subscr == s1); + OSMO_ASSERT(last_updated_subscr == NULL); + OSMO_ASSERT(s1->sgsn_data->mm == NULL); + OSMO_ASSERT((s1->flags & GSM_SUBSCRIBER_FIRST_CONTACT) == 0); /* There is no subscriber cache. Verify it */ gprs_subscr_cleanup(s1); -- cgit v1.2.3