[PATCH 1/2] sms: SMS-DELIVER TP-OA Alphanum decoding and encoding fix

[PATCH 1/2] sms: SMS-DELIVER TP-OA Alphanum decoding and encoding fix

[Tommi Kenakkala]

[-- Attachment #1: Type: text/plain, Size: 2450 bytes --]

Ofono ignores receved SMS if they have a long alphanum address with
extended ascii characters:
1) sms_encode_address_field: TP-OA max length comparison before encoding
is incorrect because TP-OA's 11 octets are 7-bit coded and thus transport
twelve 8-bit chars. Change ofono to compare character count to character
limit instead of octet limit.
2) sms_decode_address_field: Decoding from 7-bit alphabet will output max.
twelve 8-bit chars which converted to UTF-8 take 24 bytes.
Increase oFono sms_address struct's array accordingly.
---
 src/smsutil.c |   12 +++++++++---
 src/smsutil.h |    6 +++++-
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/src/smsutil.c b/src/smsutil.c
index be60ee9..b2ff349 100644
--- a/src/smsutil.c
+++ b/src/smsutil.c
@@ -524,7 +524,8 @@ static gboolean encode_validity_period(const struct sms_validity_period *vp,
 gboolean sms_encode_address_field(const struct sms_address *in, gboolean sc,
 					unsigned char *pdu, int *offset)
 {
-	size_t len = strlen(in->address);
+	const char *addr = (const char *)&in->address;
+	size_t len = strlen(addr);
 	unsigned char addr_len = 0;
 	unsigned char p[10];
 
@@ -546,7 +547,8 @@ gboolean sms_encode_address_field(const struct sms_address *in, gboolean sc,
 		unsigned char *gsm;
 		unsigned char *r;
 
-		if (len > 11)
+		/* TP-OA's 11 octets may fit 12 7-bit packed characters */
+		if (g_utf8_strlen(addr, strlen(addr)) > 12)
 			return FALSE;
 
 		gsm = convert_utf8_to_gsm(in->address, len, NULL, &written, 0);
@@ -675,7 +677,11 @@ gboolean sms_decode_address_field(const unsigned char *pdu, int len,
 		if (utf8 == NULL)
 			return FALSE;
 
-		if (strlen(utf8) > 20) {
+		/*
+		 * TP-OA's 11 7-bit octets may contain 12 8-bit chars,
+		 * which converted to unicode would take 24+1 bytes.
+		 */
+		if (strlen(utf8) > 24) {
 			g_free(utf8);
 			return FALSE;
 		}
diff --git a/src/smsutil.h b/src/smsutil.h
index b1001f8..e8266c8 100644
--- a/src/smsutil.h
+++ b/src/smsutil.h
@@ -220,7 +220,11 @@ enum cbs_geo_scope {
 struct sms_address {
 	enum sms_number_type number_type;
 	enum sms_numbering_plan numbering_plan;
-	char address[21]; /* Max 20 in semi-octet, 11 in alnum */
+	/*
+	 * An alphanum TP-OA is 11 7-bit octets. Those can contain max 12 8-bit
+	 * characters taking 24 bytes when converted to UTF-8.
+	 */
+	char address[25];
 };
 
 struct sms_scts {
-- 
1.7.9.5

Re: [PATCH 1/2] sms: SMS-DELIVER TP-OA Alphanum decoding and encoding fix

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 3252 bytes --]

Hi Tommi,

On 02/12/2015 09:09 AM, Tommi Kenakkala wrote:
> Ofono ignores receved SMS if they have a long alphanum address with
> extended ascii characters:
> 1) sms_encode_address_field: TP-OA max length comparison before encoding
> is incorrect because TP-OA's 11 octets are 7-bit coded and thus transport
> twelve 8-bit chars. Change ofono to compare character count to character
> limit instead of octet limit.

How did you arrive at 11 octet figure? TP-OA is limited to 12 bytes.  So 
12 - TON - LEN = 10 bytes payload.

10 bytes with 7-bit packed characters -> 11 unpacked characters

All characters in the GSM default alphabet can be described by a maximum 
of 2 bytes in utf8.  Exception is the euro sign, which is an extended 
character, but requires 3 utf8 bytes to encode.  So a buffer of 11 * 2 + 
1 bytes should be sufficient to hold an alphanumeric address.

> 2) sms_decode_address_field: Decoding from 7-bit alphabet will output max.
> twelve 8-bit chars which converted to UTF-8 take 24 bytes.
> Increase oFono sms_address struct's array accordingly.
> ---
>   src/smsutil.c |   12 +++++++++---
>   src/smsutil.h |    6 +++++-
>   2 files changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/src/smsutil.c b/src/smsutil.c
> index be60ee9..b2ff349 100644
> --- a/src/smsutil.c
> +++ b/src/smsutil.c
> @@ -524,7 +524,8 @@ static gboolean encode_validity_period(const struct sms_validity_period *vp,
>   gboolean sms_encode_address_field(const struct sms_address *in, gboolean sc,
>   					unsigned char *pdu, int *offset)
>   {
> -	size_t len = strlen(in->address);
> +	const char *addr = (const char *)&in->address;
> +	size_t len = strlen(addr);
>   	unsigned char addr_len = 0;
>   	unsigned char p[10];
>
> @@ -546,7 +547,8 @@ gboolean sms_encode_address_field(const struct sms_address *in, gboolean sc,
>   		unsigned char *gsm;
>   		unsigned char *r;
>
> -		if (len > 11)
> +		/* TP-OA's 11 octets may fit 12 7-bit packed characters */
> +		if (g_utf8_strlen(addr, strlen(addr)) > 12)

g_utf8_strlen() > 11 here

>   			return FALSE;
>
>   		gsm = convert_utf8_to_gsm(in->address, len, NULL, &written, 0);
> @@ -675,7 +677,11 @@ gboolean sms_decode_address_field(const unsigned char *pdu, int len,
>   		if (utf8 == NULL)
>   			return FALSE;
>
> -		if (strlen(utf8) > 20) {
> +		/*
> +		 * TP-OA's 11 7-bit octets may contain 12 8-bit chars,
> +		 * which converted to unicode would take 24+1 bytes.
> +		 */
> +		if (strlen(utf8) > 24) {

and strlen(utf8) > 22 here...

>   			g_free(utf8);
>   			return FALSE;
>   		}
> diff --git a/src/smsutil.h b/src/smsutil.h
> index b1001f8..e8266c8 100644
> --- a/src/smsutil.h
> +++ b/src/smsutil.h
> @@ -220,7 +220,11 @@ enum cbs_geo_scope {
>   struct sms_address {
>   	enum sms_number_type number_type;
>   	enum sms_numbering_plan numbering_plan;
> -	char address[21]; /* Max 20 in semi-octet, 11 in alnum */
> +	/*
> +	 * An alphanum TP-OA is 11 7-bit octets. Those can contain max 12 8-bit
> +	 * characters taking 24 bytes when converted to UTF-8.
> +	 */
> +	char address[25];

And address should be 23, the comment is wrong

>   };
>
>   struct sms_scts {
>

Regards,
-Denis

Re: [PATCH 1/2] sms: SMS-DELIVER TP-OA Alphanum decoding and encoding fix

[Tommi Kenakkala]

[-- Attachment #1: Type: text/plain, Size: 1091 bytes --]

>
> Date: Thu, 12 Feb 2015 12:59:18 -0600
> From: Denis Kenzior <denkenz@gmail.com>
> To: ofono(a)ofono.org
> Subject: Re: [PATCH 1/2] sms: SMS-DELIVER TP-OA Alphanum decoding and
>         encoding fix
> Message-ID: <54DCF806.5060609@gmail.com>
> Content-Type: text/plain; charset=ISO-8859-1; format=flowed
>
> Hi Tommi,
>
> On 02/12/2015 09:09 AM, Tommi Kenakkala wrote:
> > Ofono ignores receved SMS if they have a long alphanum address with
> > extended ascii characters:
> > 1) sms_encode_address_field: TP-OA max length comparison before encoding
> > is incorrect because TP-OA's 11 octets are 7-bit coded and thus transport
> > twelve 8-bit chars. Change ofono to compare character count to character
> > limit instead of octet limit.
>
> How did you arrive at 11 octet figure? TP-OA is limited to 12 bytes.  So
> 12 - TON - LEN = 10 bytes payload.
>

Apologies, I did develop this using those values but apparently had a
glitch in my thought-generator during patch creation. I'll re-submit a
better patch.
Thanks for the quick comments.

Br.
-- 
Tommi

[-- Attachment #2: attachment.html --]
[-- Type: text/html, Size: 1572 bytes --]

[PATCH 0/2] SMS-DELIVER TP-OA Alphanum decoding and encoding fix

[Tommi Kenakkala]

[-- Attachment #1: Type: text/plain, Size: 596 bytes --]

There are problems with some real-life scenarios where SMS-DELIVER originator
address is an alphanumeric string with 11 chars (some extended ASCII like äöü).
Currently oFono rejects such messages. Here's a patch to improve that.

Tommi Kenakkala (2):
  sms: SMS-DELIVER TP-OA Alphanum decoding and encoding fix
  unit: SMS-DELIVER decoding/encoding test for long alphanum address

 src/smsutil.c   |   12 +++++++++---
 src/smsutil.h   |    6 +++++-
 unit/test-sms.c |   38 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 52 insertions(+), 4 deletions(-)

-- 
1.7.9.5

[PATCH 1/2] sms: SMS-DELIVER TP-OA Alphanum decoding and encoding fix

[Tommi Kenakkala]

[-- Attachment #1: Type: text/plain, Size: 2121 bytes --]

TP-OA max length comparisons were incorrect because TP-OA's 7-bit
coded octets transport eleven 8-bit chars which take 23 bytes in UTF-8.
Increase address array accordingly and don't compare byte length to
character limit, but to a proper limit.
---
 src/smsutil.c |   12 +++++++++---
 src/smsutil.h |    6 +++++-
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/src/smsutil.c b/src/smsutil.c
index be60ee9..213e50e 100644
--- a/src/smsutil.c
+++ b/src/smsutil.c
@@ -524,7 +524,8 @@ static gboolean encode_validity_period(const struct sms_validity_period *vp,
 gboolean sms_encode_address_field(const struct sms_address *in, gboolean sc,
 					unsigned char *pdu, int *offset)
 {
-	size_t len = strlen(in->address);
+	const char *addr = (const char *)&in->address;
+	size_t len = strlen(addr);
 	unsigned char addr_len = 0;
 	unsigned char p[10];
 
@@ -546,7 +547,8 @@ gboolean sms_encode_address_field(const struct sms_address *in, gboolean sc,
 		unsigned char *gsm;
 		unsigned char *r;
 
-		if (len > 11)
+		/* TP-OA's 10 octets transport 11 8-bit chars */
+		if (g_utf8_strlen(addr, strlen(addr)) > 11)
 			return FALSE;
 
 		gsm = convert_utf8_to_gsm(in->address, len, NULL, &written, 0);
@@ -675,7 +677,11 @@ gboolean sms_decode_address_field(const unsigned char *pdu, int len,
 		if (utf8 == NULL)
 			return FALSE;
 
-		if (strlen(utf8) > 20) {
+		/*
+		 * TP-OA's 10 octets transport 11 8-bit chars,
+		 * which take 23 bytes in unicode.
+		 */
+		if (strlen(utf8) > 23) {
 			g_free(utf8);
 			return FALSE;
 		}
diff --git a/src/smsutil.h b/src/smsutil.h
index b1001f8..d252810 100644
--- a/src/smsutil.h
+++ b/src/smsutil.h
@@ -220,7 +220,11 @@ enum cbs_geo_scope {
 struct sms_address {
 	enum sms_number_type number_type;
 	enum sms_numbering_plan numbering_plan;
-	char address[21]; /* Max 20 in semi-octet, 11 in alnum */
+	/*
+	 * An alphanum TP-OA is 10 7-bit coded octets, which can carry
+	 * 11 8-bit characters. Those converted to UTF-8 take 23 bytes.
+	 */
+	char address[23];
 };
 
 struct sms_scts {
-- 
1.7.9.5

[PATCH 2/2] unit: SMS-DELIVER decoding/encoding test for long alphanum address

[Tommi Kenakkala]

[-- Attachment #1: Type: text/plain, Size: 1968 bytes --]

---
 unit/test-sms.c |   38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/unit/test-sms.c b/unit/test-sms.c
index 7b644df..259594e 100644
--- a/unit/test-sms.c
+++ b/unit/test-sms.c
@@ -38,6 +38,12 @@ static const char *simple_deliver = "07911326040000F0"
 		"040B911346610089F60000208062917314480CC8F71D14969741F977FD07";
 static const char *alnum_sender = "0791447758100650"
 		"040DD0F334FC1CA6970100008080312170224008D4F29CDE0EA7D9";
+static const char *unicode_deliver = "04819999990414D0FBFD7EBFDFEFF77BFE1E001"
+		"9512090801361807E00DC00FC00C400E400D600F600C500E500D800F800C"
+		"600E600C700E700C900E900CA00EA00DF003100320033003400350036003"
+		"7003800390030002000540068006900730020006D0065007300730061006"
+		"7006500200069007300200036003300200075006E00690063006F0064006"
+		"5002000630068006100720073002E";
 static const char *simple_submit = "0011000B916407281553F80000AA"
 		"0AE8329BFD4697D9EC37";
 
@@ -362,6 +368,38 @@ static void test_deliver_encode(void)
 	g_assert(strcmp(alnum_sender, encoded_pdu) == 0);
 
 	g_free(encoded_pdu);
+
+	/* test unicode_deliver*/
+	decoded_pdu = decode_hex(unicode_deliver, -1, &pdu_len, 0);
+	g_assert(decoded_pdu);
+	g_assert(pdu_len == (long)strlen(unicode_deliver) / 2);
+
+	ret = sms_decode(decoded_pdu, pdu_len, FALSE, 149, &sms);
+
+	g_free(decoded_pdu);
+
+	g_assert(ret);
+	g_assert(sms.type == SMS_TYPE_DELIVER);
+
+	ret = sms_encode(&sms, &encoded_pdu_len, &encoded_tpdu_len, pdu);
+
+	if (g_test_verbose()) {
+		int i;
+
+		for (i = 0; i < encoded_pdu_len; i++)
+			g_print("%02X", pdu[i]);
+		g_print("\n");
+	}
+
+	g_assert(ret);
+	g_assert(encoded_tpdu_len == 149);
+	g_assert(encoded_pdu_len == pdu_len);
+
+	encoded_pdu = encode_hex(pdu, encoded_pdu_len, 0);
+
+	g_assert(strcmp(unicode_deliver, encoded_pdu) == 0);
+
+	g_free(encoded_pdu);
 }
 
 static void test_simple_submit(void)
-- 
1.7.9.5

Re: [PATCH 1/2] sms: SMS-DELIVER TP-OA Alphanum decoding and encoding fix

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 2376 bytes --]

Hi Tommi,

On 02/13/2015 06:23 AM, Tommi Kenakkala wrote:
> TP-OA max length comparisons were incorrect because TP-OA's 7-bit
> coded octets transport eleven 8-bit chars which take 23 bytes in UTF-8.
> Increase address array accordingly and don't compare byte length to
> character limit, but to a proper limit.
> ---
>   src/smsutil.c |   12 +++++++++---
>   src/smsutil.h |    6 +++++-
>   2 files changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/src/smsutil.c b/src/smsutil.c
> index be60ee9..213e50e 100644
> --- a/src/smsutil.c
> +++ b/src/smsutil.c
> @@ -524,7 +524,8 @@ static gboolean encode_validity_period(const struct sms_validity_period *vp,
>   gboolean sms_encode_address_field(const struct sms_address *in, gboolean sc,
>   					unsigned char *pdu, int *offset)
>   {
> -	size_t len = strlen(in->address);
> +	const char *addr = (const char *)&in->address;
> +	size_t len = strlen(addr);
>   	unsigned char addr_len = 0;
>   	unsigned char p[10];
>
> @@ -546,7 +547,8 @@ gboolean sms_encode_address_field(const struct sms_address *in, gboolean sc,
>   		unsigned char *gsm;
>   		unsigned char *r;
>
> -		if (len > 11)
> +		/* TP-OA's 10 octets transport 11 8-bit chars */
> +		if (g_utf8_strlen(addr, strlen(addr)) > 11)
>   			return FALSE;
>
>   		gsm = convert_utf8_to_gsm(in->address, len, NULL, &written, 0);
> @@ -675,7 +677,11 @@ gboolean sms_decode_address_field(const unsigned char *pdu, int len,
>   		if (utf8 == NULL)
>   			return FALSE;
>
> -		if (strlen(utf8) > 20) {
> +		/*
> +		 * TP-OA's 10 octets transport 11 8-bit chars,
> +		 * which take 23 bytes in unicode.
> +		 */
> +		if (strlen(utf8) > 23) {

22, not 23.

>   			g_free(utf8);
>   			return FALSE;
>   		}
> diff --git a/src/smsutil.h b/src/smsutil.h
> index b1001f8..d252810 100644
> --- a/src/smsutil.h
> +++ b/src/smsutil.h
> @@ -220,7 +220,11 @@ enum cbs_geo_scope {
>   struct sms_address {
>   	enum sms_number_type number_type;
>   	enum sms_numbering_plan numbering_plan;
> -	char address[21]; /* Max 20 in semi-octet, 11 in alnum */
> +	/*
> +	 * An alphanum TP-OA is 10 7-bit coded octets, which can carry
> +	 * 11 8-bit characters. Those converted to UTF-8 take 23 bytes.

22 bytes + null terminator.

> +	 */
> +	char address[23];
>   };
>
>   struct sms_scts {
>

Regards,
-Denis