Hello all!
I just quickly looked at the gsm48_mi_to_string() method in gsm48.c. I found that it is used in several projects like OsmocomBB and OpenBSC, but it seems a bit strange to me that the TMSI represented in decimal...
... uint32_t tmsi; ... return snprintf(string, str_len, "%u", tmsi); ...
Can we move to use a hex representation instead of decimal? And why if not?
... uint32_t tmsi; ... return snprintf(string, str_len, "%x", tmsi); ...
С наилучшими пожеланиями, Яницкий Вадим.
Hi Vadim,
On Tue, Mar 15, 2016 at 01:46:31AM +0600, Вадим Яницкий wrote:
I found that it is used in several projects like OsmocomBB and OpenBSC, but it seems a bit strange to me that the TMSI represented in decimal...
yes, indeed this seems highly unusual. It reflects on the fact how little we knew about the usual representation of TMSIs back when we started to write that code.
Can we move to use a hex representation instead of decimal? And why if not?
yes, I would merge an associated patch. Preferrably together with patches for other locations where we print an IMSI and use the decimal representation at this point.
From: Vadim Yanitskiy axilirator@gmail.com
Signed-off-by: Vadim Yanitskiy axilirator@gmail.com --- src/gsm/gsm48.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/gsm/gsm48.c b/src/gsm/gsm48.c index d0a2286..e4a98d6 100644 --- a/src/gsm/gsm48.c +++ b/src/gsm/gsm48.c @@ -394,7 +394,7 @@ int gsm48_mi_to_string(char *string, const int str_len, const uint8_t *mi, if (mi_len == GSM48_TMSI_LEN && mi[0] == (0xf0 | GSM_MI_TYPE_TMSI)) { memcpy(&tmsi, &mi[1], 4); tmsi = ntohl(tmsi); - return snprintf(string, str_len, "%u", tmsi); + return snprintf(string, str_len, "0x%08x", tmsi); } break; case GSM_MI_TYPE_IMSI:
From: Vadim Yanitskiy axilirator@gmail.com
Signed-off-by: Vadim Yanitskiy axilirator@gmail.com --- openbsc/include/openbsc/gsm_subscriber.h | 2 +- openbsc/src/libmsc/db.c | 15 ++++++++------- openbsc/tests/db/db_test.c | 4 ++-- openbsc/tests/gsm0408/gsm0408_test.c | 2 +- 4 files changed, 12 insertions(+), 11 deletions(-)
diff --git a/openbsc/include/openbsc/gsm_subscriber.h b/openbsc/include/openbsc/gsm_subscriber.h index 7d6c776..785dc36 100644 --- a/openbsc/include/openbsc/gsm_subscriber.h +++ b/openbsc/include/openbsc/gsm_subscriber.h @@ -14,7 +14,7 @@
#define GSM_SUBSCRIBER_FIRST_CONTACT 0x00000001 /* gprs_sgsn.h defines additional flags including and above bit 16 (0x10000) */ -#define tmsi_from_string(str) strtoul(str, NULL, 10) +#define tmsi_from_string(str) strtoul(str + 2, NULL, 16)
#define GSM_SUBSCRIBER_NO_EXPIRATION 0x0
diff --git a/openbsc/src/libmsc/db.c b/openbsc/src/libmsc/db.c index 0935fc5..952151e 100644 --- a/openbsc/src/libmsc/db.c +++ b/openbsc/src/libmsc/db.c @@ -893,9 +893,10 @@ struct gsm_subscriber *db_get_subscriber(enum gsm_subscriber_field field, subscr->id = dbi_result_get_ulonglong(result, "id");
db_set_from_query(subscr, result); - DEBUGP(DDB, "Found Subscriber: ID %llu, IMSI %s, NAME '%s', TMSI %u, EXTEN '%s', LAC %hu, AUTH %u\n", - subscr->id, subscr->imsi, subscr->name, subscr->tmsi, subscr->extension, - subscr->lac, subscr->authorized); + DEBUGP(DDB, "Found Subscriber: ID %llu, IMSI %s, NAME '%s', " + "TMSI 0x%08x, EXTEN '%s', LAC %hu, AUTH %u\n", + subscr->id, subscr->imsi, subscr->name, subscr->tmsi, + subscr->extension, subscr->lac, subscr->authorized); dbi_result_free(result);
get_equipment_by_subscr(subscr); @@ -935,7 +936,7 @@ int db_subscriber_update(struct gsm_subscriber *subscr) int db_sync_subscriber(struct gsm_subscriber *subscriber) { dbi_result result; - char tmsi[14]; + char tmsi[11]; char *q_tmsi, *q_name, *q_extension;
dbi_conn_quote_string_copy(conn, @@ -944,7 +945,7 @@ int db_sync_subscriber(struct gsm_subscriber *subscriber) subscriber->extension, &q_extension); if (subscriber->tmsi != GSM_RESERVED_TMSI) { - sprintf(tmsi, "%u", subscriber->tmsi); + sprintf(tmsi, "0x%08x", subscriber->tmsi); dbi_conn_quote_string_copy(conn, tmsi, &q_tmsi); @@ -1194,7 +1195,7 @@ int db_subscriber_expire(void *priv, void (*callback)(void *priv, long long unsi int db_subscriber_alloc_tmsi(struct gsm_subscriber *subscriber) { dbi_result result = NULL; - char tmsi[14]; + char tmsi[11]; char *tmsi_quoted;
for (;;) { @@ -1205,7 +1206,7 @@ int db_subscriber_alloc_tmsi(struct gsm_subscriber *subscriber) if (subscriber->tmsi == GSM_RESERVED_TMSI) continue;
- sprintf(tmsi, "%u", subscriber->tmsi); + sprintf(tmsi, "0x%08x", subscriber->tmsi); dbi_conn_quote_string_copy(conn, tmsi, &tmsi_quoted); result = dbi_conn_queryf(conn, "SELECT * FROM Subscriber " diff --git a/openbsc/tests/db/db_test.c b/openbsc/tests/db/db_test.c index a02d1f8..faea820 100644 --- a/openbsc/tests/db/db_test.c +++ b/openbsc/tests/db/db_test.c @@ -200,7 +200,7 @@ int main() alice->lac=42; db_sync_subscriber(alice); /* Get by TMSI */ - snprintf(scratch_str, sizeof(scratch_str), "%"PRIu32, alice->tmsi); + snprintf(scratch_str, sizeof(scratch_str), "0x%08x", alice->tmsi); alice_db = db_get_subscriber(GSM_SUBSCRIBER_TMSI, scratch_str); COMPARE(alice, alice_db); SUBSCR_PUT(alice_db); @@ -227,7 +227,7 @@ int main() db_subscriber_assoc_imei(alice, "1234567890"); db_subscriber_assoc_imei(alice, "6543560920"); /* Get by TMSI */ - snprintf(scratch_str, sizeof(scratch_str), "%"PRIu32, alice->tmsi); + snprintf(scratch_str, sizeof(scratch_str), "0x%08x", alice->tmsi); alice_db = db_get_subscriber(GSM_SUBSCRIBER_TMSI, scratch_str); COMPARE(alice, alice_db); SUBSCR_PUT(alice_db); diff --git a/openbsc/tests/gsm0408/gsm0408_test.c b/openbsc/tests/gsm0408/gsm0408_test.c index 781ef61..8ed57ca 100644 --- a/openbsc/tests/gsm0408/gsm0408_test.c +++ b/openbsc/tests/gsm0408/gsm0408_test.c @@ -93,7 +93,7 @@ static void test_mi_functionality(void) /* tmsi code */ mi_len = gsm48_generate_mid_from_tmsi(mi, tmsi); gsm48_mi_to_string(mi_parsed, sizeof(mi_parsed), mi + 2, mi_len - 2); - COMPARE((uint32_t)strtoul(mi_parsed, NULL, 10), ==, tmsi); + COMPARE((uint32_t)tmsi_from_string(mi_parsed), ==, tmsi);
/* imsi code */ mi_len = gsm48_generate_mid_from_imsi(mi, imsi_odd);
Hi Vadim,
I've merged both of your patches, thanks!
If you have time, please check if there are other occurrences in OpenBSC or OsmocomBB where the TMSI is printed as integer. Thanks!
Regards, Harald
On 17 Mar 2016, at 14:17, Harald Welte laforge@gnumonks.org wrote:
Hi Vadim,
Hi Guys,
I've merged both of your patches, thanks!
I think it was a bit too quick. I foresee one problem. Let's assume someone is using TMSIs and now upgrade the sourcecode. All CM Service Requests will fail because the TMSI is not known. What is the migration/mitigation plan?
-#define tmsi_from_string(str) strtoul(str, NULL, 10) +#define tmsi_from_string(str) strtoul(str + 2, NULL, 16)
is problematic too:
1.) E.g.
gsm48_mi_to_string(mi_string, sizeof(mi_string), idi->mi, idi->mi_len); ... subscr = subscr_get_by_tmsi(bts->network->subscr_group, tmsi_from_string(mi_string));
tmsi_from_string will not work for this anymore.
2.) there is no length check but that doesn't seem to be a big issue right now.
* I think the above hunk should be reverted * a DB schema upgrade and store the TMSI as uint32_t * Use hex presentation in VTY
What do you think?
Hi Holger,
On Thu, Mar 17, 2016 at 02:29:33PM +0100, Holger Freyther wrote:
I think it was a bit too quick. I foresee one problem. Let's assume someone is using TMSIs and now upgrade the sourcecode. All CM Service Requests will fail because the TMSI is not known. What is the migration/mitigation plan?
ugh. I failed to see that for some strange reason we store the TMSI as "TEXT" in the database. That's really odd, especailly as we use %u formatting when searching for TMSIs in the database.
- I think the above hunk should be reverted
done.
- a DB schema upgrade and store the TMSI as uint32_t
correct.
Hi Guys!
If you have time, please check if there are other occurrences in OpenBSC or OsmocomBB where the TMSI is printed as integer. Thanks!
No problem! :)
I think it was a bit too quick. I foresee one problem. Let's assume
someone
is using TMSIs and now upgrade the sourcecode. All CM Service Requests will fail because the TMSI is not known. What is the migration/mitigation
plan?
I absolutely agree with Holger. Maybe we can add some code that will check if database still stores TMSIs in old representation style and convert them to uint32_t? We can change the libmsc/db.c:db_prepare() for this purpose.
tmsi_from_string will not work for this anymore.
Yes, I forgot to change the gsm_subscriber.c ... Sorry.
there is no length check but that doesn't seem to be a big issue right
now.
We can just write a function that will do this check instead of using #define.
- a DB schema upgrade and store the TMSI as uint32_t
- Use hex presentation in VTY
+1
С наилучшими пожеланиями, Яницкий Вадим.
2016-03-17 19:43 GMT+06:00 Harald Welte laforge@gnumonks.org:
Hi Holger,
On Thu, Mar 17, 2016 at 02:29:33PM +0100, Holger Freyther wrote:
I think it was a bit too quick. I foresee one problem. Let's assume someone is using TMSIs and now upgrade the sourcecode. All CM Service Requests will fail because the TMSI is not known. What is the migration/mitigation plan?
ugh. I failed to see that for some strange reason we store the TMSI as "TEXT" in the database. That's really odd, especailly as we use %u formatting when searching for TMSIs in the database.
- I think the above hunk should be reverted
done.
- a DB schema upgrade and store the TMSI as uint32_t
correct.
--
- Harald Welte laforge@gnumonks.org
============================================================================ "Privacy in residential applications is a desirable marketing option." (ETSI EN 300 175-7 Ch. A6)
On 17 Mar 2016, at 17:21, Вадим Яницкий axilirator@gmail.com wrote:
Hi Guys!
If you have time, please check if there are other occurrences in OpenBSC or OsmocomBB where the TMSI is printed as integer. Thanks!
No problem! :)
I think it was a bit too quick. I foresee one problem. Let's assume someone is using TMSIs and now upgrade the sourcecode. All CM Service Requests will fail because the TMSI is not known. What is the migration/mitigation plan?
I absolutely agree with Holger. Maybe we can add some code that will check if database still stores TMSIs in old representation style and convert them to uint32_t? We can change the libmsc/db.c:db_prepare() for this purpose.
yes, we have a schema version and can just increase it. E.g. have a look at how we migrate SMS.
tmsi_from_string will not work for this anymore.
Yes, I forgot to change the gsm_subscriber.c ... Sorry.
it happens. thanks for contributing
there is no length check but that doesn't seem to be a big issue right now.
We can just write a function that will do this check instead of using #define.
I think you will not need to touch this define at all. We might want to change the name to _from_mi_string.
- a DB schema upgrade and store the TMSI as uint32_t
- Use hex presentation in VTY
+1
looking forward for the follow up.
holger
Hello, Harald!
There are two patches. One for libosmocore, another one for OpenBSC (sorry for a little bit of mess). Right now I have no opportunity to test it, but the output of 'make check' seems ok.
С наилучшими пожеланиями, Яницкий Вадим.
2016-03-16 16:22 GMT+06:00 Harald Welte laforge@gnumonks.org:
Hi Vadim,
On Tue, Mar 15, 2016 at 01:46:31AM +0600, Вадим Яницкий wrote:
I found that it is used in several projects like OsmocomBB and OpenBSC, but it seems a bit strange to me that the TMSI represented in decimal...
yes, indeed this seems highly unusual. It reflects on the fact how little we knew about the usual representation of TMSIs back when we started to write that code.
Can we move to use a hex representation instead of decimal? And why if
not?
yes, I would merge an associated patch. Preferrably together with patches for other locations where we print an IMSI and use the decimal representation at this point.
--
- Harald Welte laforge@gnumonks.org
============================================================================ "Privacy in residential applications is a desirable marketing option." (ETSI EN 300 175-7 Ch. A6)
-
axilirator＠gmail.com -
Harald Welte -
Holger Freyther -
Вадим Яницкий