Hello Our goal was to send status sms via the vty interface. But all of our sms were cropped. In contrast sms from one MS to another MS are displayed correctly (despite the fact, that the text at the database contains several '@' at the end / the user_data contains several zero-octets). Therefore i have inspect the code and found several bugs. The main problem is that the "user_data_len" is not correctly used. As per GSM 03.40, 9.2.3.16 TP‑User‑Data‑Length (TP‑UDL): "If the TP‑User‑Data is coded using the GSM 7 bit default alphabet, the TP‑User‑Data‑Length field gives an integer representation of the number of septets within the TP‑User‑Data field to follow." Currently the "user_data_len" contains the number of octets (returned from gsm_7bit_encode(...) at gsm_utils.c (libosmocore)). The big problem here is that this information is not unique, e.g.: 1.) 46 non-extension characters + 1 extension character => (46 * 7 bit + (1 * (2 * 7 bit))) / 8 bit = 42 octets 2.) 47 non-extension characters => (47 * 7 bit) / 8 bit = 41,125 = 42 octets 3.) 48 non-extension characters => (48 * 7 bit) / 8 bit = 42 octects But the MS has to know the correct "user_data_len" to decode the correct number of characters. For this reason i updated the gsm_7bit_encode() function to return the correct number of septets. However sometimes it is needed to know the correct number of octets (e.g. at gsm_04_11.c: gsm340_gen_tpdu(...)) => i added a function to gsm_utils.c named: uint8_t get_octet_len(const uint8_t sept_len) I have also fixed the problem, that the sms are wrongly stored / displayed on the database. But the solution on the function *sms_from_result(...) (at db.c) is not really "beautiful". This is because there exists no "user_data_len" field at the database. To store the right value for "user_data_len" (which is further needed) i have to get the length from the "text" field. Unfortunately this is not enough. If the text contains extension characters like {[]} etc. then the "user_data_len" has to be bigger because these characters needs two septets. Therefore i use a switch statement so search for these characters. A better solution for that is to store the right "user_data_len" to the database (on the encoding / decoding procedure). But i don't know if this is a suitable solution for all of you (because you have to change your database structure etc.). Best Regards Dennis Wehrle
On 03/18/2011 11:01 PM, Dennis Wehrle wrote:
Hello
Hi Dennis, thank you very much for your patch. There are some formal issues with the patch and it would be nice if you could resolve them. * In general we don't keep commented out code around, please remove it. * We avoid the // comments. * When introducing a new function that has known static linkage please use a namespace, e.g. not get_octlet_length but at least gsm_get_octet_length or put in 7bit as well. Besides that your patch looks nice, but I do have two concerns. Your 160 char limit assumes the default GSM alphabet? In the future we might be able to encode and decode from different (e.g. chinese/unicode) alphabet. The same concern is for the new code in db.c that handles things specially. It would be nice if all of this could be hidden in libosmocore or such. cheers holger
Hi Holger On 19.03.2011 01:54, Holger Hans Peter Freyther wrote:
On 03/18/2011 11:01 PM, Dennis Wehrle wrote:
Hello
Hi Dennis,
thank you very much for your patch. There are some formal issues with the patch and it would be nice if you could resolve them.
sure ;-)
* In general we don't keep commented out code around, please remove it. * We avoid the // comments.
Can you explain why? (just for my interest)
* When introducing a new function that has known static linkage please use a namespace, e.g. not get_octlet_length but at least gsm_get_octet_length or put in 7bit as well.
Besides that your patch looks nice, but I do have two concerns. Your 160 char limit assumes the default GSM alphabet? In the future we might be able to encode and decode from different (e.g. chinese/unicode) alphabet. The same concern is for the new code in db.c that handles things specially. It would be nice if all of this could be hidden in libosmocore or such.
I have moved the septet length lookup to the gsm_utils.c. This function looks for characters defined at the "GSM 7bit default alphabet extension table" (GSM 03.38 6.2.1.1) which needs two septets. These characters are: ^ | € { } [ ] ~ \
cheers holger
Best Regards Dennis
On 21/03/11 13:03, Dennis Wehrle wrote:
Hi Holger
On 19.03.2011 01:54, Holger Hans Peter Freyther wrote:
On 03/18/2011 11:01 PM, Dennis Wehrle wrote:
Hello
Hi Dennis,
thank you very much for your patch. There are some formal issues with the patch and it would be nice if you could resolve them.
sure ;-)
* In general we don't keep commented out code around, please remove it. * We avoid the // comments.
Can you explain why? (just for my interest)
This is a bad practise. Leaving code that has been commented is not of any help, it distracts the attention of the reader (who would wonder why it's still there). On the other hand, if the intention is to track changes, the SCM already does this for you, with it it's easy to see what code was removed and added along time. If the intention to leave that code commented to remark any special (or tricky) case, better add some comment on that (my criteria usually is to document special cases that I would even forget myself). BTW, adding a short explanation to the patch also helps a lot to apply them with git-am.
Hi Pablo
* In general we don't keep commented out code around, please remove it. * We avoid the // comments. Can you explain why? (just for my interest) This is a bad practise. Leaving code that has been commented is not of any help, it distracts the attention of the reader (who would wonder why it's still there). On the other hand, if the intention is to track changes, the SCM already does this for you, with it it's easy to see what code was removed and added along time. If the intention to leave that code commented to remark any special (or tricky) case, better add some comment on that (my criteria usually is to document special cases that I would even forget myself).
Sry for the misunderstanding question, but my question was why the "//" comments are not used.
BTW, adding a short explanation to the patch also helps a lot to apply them with git-am.
Thx for this hint. The patches were created with the command "git diff HEAD > sms_openbsc.patch". But now i know that was not the correct way. Now i have created a branch, commited the changes (with a comment) and created the patch with "git format-patch master --stdout > sms_openbsc.patch". Enclosed are the new patches. Best Regards Dennis
Hi,
Sry for the misunderstanding question, but my question was why the "//" comments are not used.
Consistency I guess. We try to stick to the kernel coding style ( see linux/Documentation/CodingStyle ), which states: <quote> Linux style for comments is the C89 "/* ... */" style. Don't use C99-style "// ..." comments. </quote> Cheers, Sylvain
On 31.03.2011 13:04, Holger Hans Peter Freyther wrote:
Hi Dennis,
sorry for the delay and thanks a lot for your patch. Do you feel like adding a test case to the libosmocore/tests/sms/sms_test.c?
thanks a lot holger
Hi Holger No Problem. See attach for the new sms_lib.patch Best regards Dennis
On 01/04/11 14:35, Dennis Wehrle wrote:
On 31.03.2011 13:04, Holger Hans Peter Freyther wrote:
Hi Dennis,
sorry for the delay and thanks a lot for your patch. Do you feel like adding a test case to the libosmocore/tests/sms/sms_test.c?
thanks a lot holger
Hi Holger
No Problem. See attach for the new sms_lib.patch
diff --git a/include/osmocore/gsm_utils.h b/include/osmocore/gsm_utils.h index 0aadd2e..a47ce7b 100644 --- a/include/osmocore/gsm_utils.h +++ b/include/osmocore/gsm_utils.h
I'm sorry, but this won't apply to current libosmocore git tree (we have applied several patches to rework the header files directories). would you rework it upon current git libosmocore tree? Thanks!
Hi Pablo Thx for your answer. I have updated to the newest version, created the patch and tested it. But i have found another bug. Therefore i want to wait until i have fix this bug (next week). It is really strange....: working: Sent a SMS with 167 chars from Nokia 3310 to iPhone. not working: Sent the same message from iPhone to Nokia 3310. The differences are: Nokia: message identifier = 199, user-data-length part1 = 160, part2 = 31 iPhone: message identifier = 11, user-data-length part1 = 161, part2 = 33; part1 is retransmitted 5 times.... Best Regards and a nice weekend Dennis Wehrle On 01.04.2011 14:53, Pablo Neira Ayuso wrote:
On 01/04/11 14:35, Dennis Wehrle wrote:
On 31.03.2011 13:04, Holger Hans Peter Freyther wrote:
Hi Dennis,
sorry for the delay and thanks a lot for your patch. Do you feel like adding a test case to the libosmocore/tests/sms/sms_test.c?
thanks a lot holger
Hi Holger
No Problem. See attach for the new sms_lib.patch
diff --git a/include/osmocore/gsm_utils.h b/include/osmocore/gsm_utils.h index 0aadd2e..a47ce7b 100644 --- a/include/osmocore/gsm_utils.h +++ b/include/osmocore/gsm_utils.h I'm sorry, but this won't apply to current libosmocore git tree (we have applied several patches to rework the header files directories).
would you rework it upon current git libosmocore tree?
Thanks!
Hello As said last Friday i have inspected the following bug: SMS with more than 160 chars from Nokia 3310 to iPhone works, but not the other way around. It turns out, that once more the user_data_len is the problem. The problem is, that if we sent more than 160 chars a header with some information (user data header length, information element identifier, length, message identifier, message parts, message part number, fill bits) will be transmitted within the user_data. This is a problem because in my gsm_get_septet_len function i take the text-field from the database and count every extension character twice. In this special case it is possible that parts of the header data will be interpretet (decoded) as a extension character (in my case a pipe |). Therefore my function returns the wrong number of septets. The best solution for that is to store the number of septets at the database (on the decoding process). Therefore i want to discuss if this is a suitable solution. If we don't want to change the structure of the database i could possible make a dirty hack (look if the "ud_hdr_ind" is set, get the "user data header length" (udhl) from the user_data, count the number of septets from "text - 1 - udhl - fill bits"). But i strongly recommend to change the database (i could do it). Best Regards Dennis Wehrle
On 04/04/2011 05:27 PM, Dennis Wehrle wrote:
Hello
Hi Dennis, sorry for the delay. I had hoped someone with more SMS knowledge pops up and takes a look. I have mostly fixed issues with the code itself (timesouts, leaks, etc) but not really used it.
The best solution for that is to store the number of septets at the database (on the decoding process). Therefore i want to discuss if this is a suitable solution. If we don't want to change the structure of the database i could possible make a dirty hack (look if the "ud_hdr_ind" is set, get the "user data header length" (udhl) from the user_data, count the number of septets from "text - 1 - udhl - fill bits"). But i strongly recommend to change the database (i could do it).
Yes, please change the database. I think you want to increase the version of the database schema but I would not worry about having a migration inside the db.c code itself (we can have a script to update the schema and drop the SMS table..).
Hi, * Holger Hans Peter Freyther <holger@freyther.de> [2011-04-17 19:00]:
On 04/04/2011 05:27 PM, Dennis Wehrle wrote: [...] FWIW, your original patch and motivation also looks very reasonable to me!
The best solution for that is to store the number of septets at the database (on the decoding process). Therefore i want to discuss if this is a suitable solution. If we don't want to change the structure of the database i could possible make a dirty hack (look if the "ud_hdr_ind" is set, get the "user data header length" (udhl) from the user_data, count the number of septets from "text - 1 - udhl - fill bits"). But i strongly recommend to change the database (i could do it).
Yes, please change the database. I think you want to increase the version of the database schema but I would not worry about having a migration inside the db.c code itself (we can have a script to update the schema and drop the SMS table..).
Imho the current database scheme for SMS is rather suboptimal in general because it attempts to mix incomplete sms encoding values with decoded user data. As we have to properly encode the message anyway when sending from the vty it seems only reasonable to me to store UDL in the db as well. This way it is very easy to do the decoding/encoding step with the already present data_coding_scheme value. But thinking more about this... does it really make sense to store anything but control data and the actual PDU in the database? When sending a message from the vty the message is encoded anyway, so just inserting the PDU + control data should be sufficient. I'm not sure if OpenBSC has to be aware of things like UDHI, UDH, PID and thelike. The only place where this is useful so far is for the debugging output. But what about moving this decoding for mere visual purposes to libosmocore completely and not store this information in the database at all? Cheers Nico P.S. attached is a submit decoder I used in the past one could use as a basis for the proposed libosmocore function.
Hi Nico, On Sun, Apr 17, 2011 at 08:06:54PM +0200, Nico Golde wrote:
But thinking more about this... does it really make sense to store anything but control data and the actual PDU in the database? When sending a message from the vty the message is encoded anyway, so just inserting the PDU + control data should be sufficient. I'm not sure if OpenBSC has to be aware of things like UDHI, UDH, PID and thelike.
well, what you definitely want aside from the PDU is 'valid_until', which is a result of PDU parsing... But yes, generally, * there should be no decoded SMS in the database (and if it is, only for debugging, i.e. no code should ever use the human-readable decode * there should be no direct references to the subscriber table (sender_id/receiver_id). Rather, the actual phone number (MSISDN) should be used. -- - Harald Welte <laforge@gnumonks.org> http://laforge.gnumonks.org/ ============================================================================ "Privacy in residential applications is a desirable marketing option." (ETSI EN 300 175-7 Ch. A6)
Hi Holger wrote:
Yes, please change the database. I think you want to increase the version of the database schema but I would not worry about having a migration inside the db.c code itself (we can have a script to update the schema and drop the SMS table..).
Ok, sounds good. But unfortunally i have no time for the next 2 weeks. Nico wrote:
Imho the current database scheme for SMS is rather suboptimal in general because it attempts to mix incomplete sms encoding values with decoded user data. As we have to properly encode the message anyway when sending from the vty it seems only reasonable to me to store UDL in the db as well. This way it is very easy to do the decoding/encoding step with the already present data_coding_scheme value.
But thinking more about this... does it really make sense to store anything but control data and the actual PDU in the database? When sending a message from the vty the message is encoded anyway, so just inserting the PDU + control data should be sufficient. I'm not sure if OpenBSC has to be aware of things like UDHI, UDH, PID and thelike.
The only place where this is useful so far is for the debugging output. But what about moving this decoding for mere visual purposes to libosmocore completely and not store this information in the database at all?
Cheers Nico P.S. attached is a submit decoder I used in the past one could use as a basis for the proposed libosmocore function.
Thx for your explanatory notes. And thx for your code. I will see how i can use it. Harald wrote:
well, what you definitely want aside from the PDU is 'valid_until', which is a result of PDU parsing...
But yes, generally, * there should be no decoded SMS in the database (and if it is, only for debugging, i.e. no code should ever use the human-readable decode * there should be no direct references to the subscriber table (sender_id/receiver_id). Rather, the actual phone number (MSISDN) should be used.
I will keep this in mind. Best Regards Dennis
Hello Here it is. After some delay (sorry!) i've finished the sms-patch. As already mentioned, i have changed the database-format for the SMS-table. If you already have a database, you have to _drop_ the SMS-table! If you have questions, please feel free to ask me. Best Regards Dennis Wehrle On 18.03.2011 23:01, Dennis Wehrle wrote:
Hello
Our goal was to send status sms via the vty interface. But all of our sms were cropped. In contrast sms from one MS to another MS are displayed correctly (despite the fact, that the text at the database contains several '@' at the end / the user_data contains several zero-octets). Therefore i have inspect the code and found several bugs.
The main problem is that the "user_data_len" is not correctly used. As per GSM 03.40, 9.2.3.16 TP‑User‑Data‑Length (TP‑UDL): "If the TP‑User‑Data is coded using the GSM 7 bit default alphabet, the TP‑User‑Data‑Length field gives an integer representation of the number of septets within the TP‑User‑Data field to follow."
Currently the "user_data_len" contains the number of octets (returned from gsm_7bit_encode(...) at gsm_utils.c (libosmocore)). The big problem here is that this information is not unique, e.g.: 1.) 46 non-extension characters + 1 extension character => (46 * 7 bit + (1 * (2 * 7 bit))) / 8 bit = 42 octets 2.) 47 non-extension characters => (47 * 7 bit) / 8 bit = 41,125 = 42 octets 3.) 48 non-extension characters => (48 * 7 bit) / 8 bit = 42 octects
But the MS has to know the correct "user_data_len" to decode the correct number of characters. For this reason i updated the gsm_7bit_encode() function to return the correct number of septets. However sometimes it is needed to know the correct number of octets (e.g. at gsm_04_11.c: gsm340_gen_tpdu(...)) => i added a function to gsm_utils.c named: uint8_t get_octet_len(const uint8_t sept_len)
I have also fixed the problem, that the sms are wrongly stored / displayed on the database. But the solution on the function *sms_from_result(...) (at db.c) is not really "beautiful". This is because there exists no "user_data_len" field at the database. To store the right value for "user_data_len" (which is further needed) i have to get the length from the "text" field. Unfortunately this is not enough. If the text contains extension characters like {[]} etc. then the "user_data_len" has to be bigger because these characters needs two septets. Therefore i use a switch statement so search for these characters. A better solution for that is to store the right "user_data_len" to the database (on the encoding / decoding procedure). But i don't know if this is a suitable solution for all of you (because you have to change your database structure etc.).
Best Regards Dennis Wehrle
On 07/07/2011 07:16 PM, Dennis Wehrle wrote:
Hello
Here it is. After some delay (sorry!) i've finished the sms-patch. As already mentioned, i have changed the database-format for the SMS-table. If you already have a database, you have to _drop_ the SMS-table!
If you have questions, please feel free to ask me.
Hi Dennis, the biggest issue I see is the change of signatures in libosmocore (e.g. adding a parameter). We will need to introduce a method with another name. The next issue is with coding style (yeah, we are picky...). Would you have time to work on it? Do you need a hand with it? holger
On 07/24/2011 03:01 PM, Holger Hans Peter Freyther wrote:
Would you have time to work on it? Do you need a hand with it?
Hi Dennis, I pushed zecke/sms-fixes-by-dennis-wehrle to libosmocore, do you agree with the change? I have moved the VTY change to a new patch (if you want to I can use you as author), I left the 'normal' _decode message like it was and added a new method to it, I also had some cosmetic changes (formation). I would also like to see a rewording of the commit message. I am now moving to the OpenBSC part of your patch. thanks holger
On 07/24/2011 08:20 PM, Holger Hans Peter Freyther wrote:
On 07/24/2011 03:01 PM, Holger Hans Peter Freyther wrote:
Would you have time to work on it? Do you need a hand with it?
I am now moving to the OpenBSC part of your patch.
Hi Dennis, do you think you could split the work into multiple patches? E.g. 1.) Fix the truncation of the message when sent from the VTY 2.) Move from id to number in the SMS Table (also bump the db version) 3.) Implement multipart SMS? I will try to start with 1st, it would be very kind of you could work on the other parts. holger
Hello Holger Here are the patches. It was necessary to rewrite your patch for task 1 because some code was missing. Best Regards Dennis On 24.07.2011 20:30, Holger Hans Peter Freyther wrote:
On 07/24/2011 08:20 PM, Holger Hans Peter Freyther wrote:
On 07/24/2011 03:01 PM, Holger Hans Peter Freyther wrote:
Would you have time to work on it? Do you need a hand with it?
I am now moving to the OpenBSC part of your patch.
Hi Dennis,
do you think you could split the work into multiple patches? E.g.
1.) Fix the truncation of the message when sent from the VTY 2.) Move from id to number in the SMS Table (also bump the db version) 3.) Implement multipart SMS?
I will try to start with 1st, it would be very kind of you could work on the other parts.
holger
Hi Holger On 24.07.2011 20:20, Holger Hans Peter Freyther wrote:
Hi Dennis,
I pushed zecke/sms-fixes-by-dennis-wehrle to libosmocore, do you
agree with
the change?
Sure, i agree with your changes.
I have moved the VTY change to a new patch (if you want to I can use you as author),
I think, it's not necessary :)
I left the 'normal' _decode message like it was and added a new method to it, I also had some cosmetic changes (formation). I would also like to see a rewording of the commit message.
I think there are some typos in my commit message. For instance, in the first sentence, i think it should be 'were' instead of 'where'. On 24.07.2011 20:30, Holger Hans Peter Freyther wrote:
Hi Dennis,
do you think you could split the work into multiple patches? E.g.
1.) Fix the truncation of the message when sent from the VTY 2.) Move from id to number in the SMS Table (also bump the db version) 3.) Implement multipart SMS?
I will try to start with 1st, it would be very kind of you could work on the other parts.
Sure, i can split it up. My understanding is that the patch for task 2.) depends on task 1.) and the patch for task 3.) depends on task 2.). (create a patch between the commits) Right? On 24.07.2011 22:18, Holger Hans Peter Freyther wrote:
On 07/24/2011 08:30 PM, Holger Hans Peter Freyther wrote:
I will try to start with 1st, it would be very kind of you could
work on the
other parts.
E.g. I think this will fix the truncation issue from the VTY.
Ok, i can work on the missing two parts. I will also have a look at your patch. Best Regards Dennis
On 07/25/2011 05:46 PM, Dennis Wehrle wrote:
Hi Holger
I have moved the VTY change to a new patch (if you want to I can use you as author),
I think, it's not necessary :)
hehe, we are picky, one change, one (atomic) reason.
I think there are some typos in my commit message. For instance, in the first sentence, i think it should be 'were' instead of 'where'.
Yes, and in general 72/80 chars per line limit. (also try to follow it in the code)
Sure, i can split it up. My understanding is that the patch for task 2.) depends on task 1.) and the patch for task 3.) depends on task 2.). (create a patch between the commits) Right?
exactly, it should be commits depending on each other.
Ok, i can work on the missing two parts. I will also have a look at your patch.
thanks.
Hi Holger On 25.07.2011 18:13, Holger Hans Peter Freyther wrote:
On 07/25/2011 05:46 PM, Dennis Wehrle wrote:
Hi Holger
I have moved the VTY change to a new patch (if you want to I can use you as author),
I think, it's not necessary :)
hehe, we are picky, one change, one (atomic) reason.
I meant: it's not necessary to write me as the author :)
I think there are some typos in my commit message. For instance, in the first sentence, i think it should be 'were' instead of 'where'.
Yes, and in general 72/80 chars per line limit. (also try to follow it in the code)
Ok, i will pay attention to only use 72/80 chars per line. I will also pay attention to make a space before and after brackets. Best Regards Dennis
participants (6)
-
Dennis Wehrle -
Harald Welte -
Holger Hans Peter Freyther -
Nico Golde -
Pablo Neira Ayuso -
Sylvain Munaut