core: fix some bugs in GSM7 packing code

The existing gsm_pack() had a bug where if the last
septet should be in an octet by itself, that last
octet wouldn't be added.  Plus, kinda pointless to
use a GByteArray here when we already know what the
length will be through simple arithmetic.

We can also simplify the function too.

Furthermore, there weren't any testcases for starting
packing at an offset other than zero, so add one.

2 files changed, 66 insertions, 25 deletions

diff --git a/src/mm-charsets.c b/src/mm-charsets.c
index 0956e4e1..13976ba2 100644
--- a/src/mm-charsets.c
+++ b/src/mm-charsets.c
@@ -672,37 +672,33 @@ gsm_pack (const guint8 *src,
           guint8 start_offset,
           guint32 *out_packed_len)
 {
-    GByteArray *packed;
-    guint8 c, add_last = 0;
-    int i;
+    guint8 *packed;
+    guint octet = 0, lshift, plen;
+    int i = 0;
 
-    packed = g_byte_array_sized_new (src_len);
+    g_return_val_if_fail (start_offset < 8, NULL);
 
-    for (i = 0, c = 0; i < src_len; i++) {
-        guint8 bits_here, offset;
-        guint32 start_bit;
+    plen = (src_len * 7) + start_offset; /* total length in bits */
+    if (plen % 8)
+        plen += 8;
+    plen /= 8;  /* now in bytes */
 
-        start_bit = start_offset + (i * 7); /* Overall bit offset of char in buffer */
-        offset = start_bit % 8; /* Offset to start of char in this byte */
-        bits_here = offset ? (8 - offset) : 7;
-
-        c |= (src[i] & 0x7F) << offset;
-        if (offset) {
-            /* Add this packed byte */
-            g_byte_array_append (packed, &c, 1);
-            c = add_last = 0;
-        }
+    packed = g_malloc0 (plen);
 
-        /* Pack the rest of this char into the next byte */
-        if (bits_here != 7) {
-            c = (src[i] & 0x7F) >> bits_here;
-            add_last = 1;
+    for (i = 0, lshift = start_offset; i < src_len; i++) {
+        packed[octet] |= (src[i] & 0x7F) << lshift;
+        if (lshift > 1) {
+            /* Grab the lost bits and add to next octet */
+            g_assert (octet + 1 < plen);
+            packed[octet + 1] = (src[i] & 0x7F) >> (8 - lshift);
         }
+        if (lshift)
+            octet++;
+        lshift = lshift ? lshift - 1 : 7;
     }
-    if (add_last)
-        g_byte_array_append (packed, &c, 1);
 
-    *out_packed_len = packed->len;
-    return g_byte_array_free (packed, FALSE);
+    if (out_packed_len)
+        *out_packed_len = plen;
+    return packed;
 }
 
diff --git a/src/tests/test-charsets.c b/src/tests/test-charsets.c
index 70c796ab..f954d93e 100644
--- a/src/tests/test-charsets.c
+++ b/src/tests/test-charsets.c
@@ -284,6 +284,48 @@ test_pack_gsm7_24_chars (void *f, gpointer d)
     g_free (packed);
 }
 
+static void
+test_pack_gsm7_last_septet_alone (void *f, gpointer d)
+{
+    static const guint8 unpacked[] = {
+        0x54, 0x68, 0x69, 0x73, 0x20, 0x69, 0x73, 0x20, 0x72, 0x65, 0x61, 0x6C,
+        0x6C, 0x79, 0x20, 0x63, 0x6F, 0x6F, 0x6C, 0x20, 0x10, 0x10, 0x10, 0x10,
+        0x10
+    };
+    static const guint8 expected[] = {
+        0x54, 0x74, 0x7A, 0x0E, 0x4A, 0xCF, 0x41, 0xF2, 0x72, 0x98, 0xCD, 0xCE,
+        0x83, 0xC6, 0xEF, 0x37, 0x1B, 0x04, 0x81, 0x40, 0x20, 0x10
+    };
+    guint8 *packed;
+    guint32 packed_len = 0;
+
+    /* Tests that a 25-character unpacked string (where, when packed, the last
+     * septet will be in an octet by itself) packs correctly.
+     */
+
+    packed = gsm_pack (unpacked, sizeof (unpacked), 0, &packed_len);
+    g_assert (packed);
+    g_assert_cmpint (packed_len, ==, sizeof (expected));
+
+    g_free (packed);
+}
+
+static void
+test_pack_gsm7_7_chars_offset (void *f, gpointer d)
+{
+    static const guint8 unpacked[] = { 0x68, 0x65, 0x6C, 0x6C, 0x6F, 0x10, 0x2F };
+    static const guint8 expected[] = { 0x00, 0x5D, 0x66, 0xB3, 0xDF, 0x90, 0x17 };
+    guint8 *packed;
+    guint32 packed_len = 0;
+
+    packed = gsm_pack (unpacked, sizeof (unpacked), 5, &packed_len);
+    g_assert (packed);
+    g_assert_cmpint (packed_len, ==, sizeof (expected));
+    g_assert_cmpint (memcmp (packed, expected, packed_len), ==, 0);
+
+    g_free (packed);
+}
+
 
 #if GLIB_CHECK_VERSION(2,25,12)
 typedef GTestFixtureFunc TCFunc;
@@ -314,6 +356,9 @@ int main (int argc, char **argv)
     g_test_suite_add (suite, TESTCASE (test_pack_gsm7_7_chars, NULL));
     g_test_suite_add (suite, TESTCASE (test_pack_gsm7_all_chars, NULL));
     g_test_suite_add (suite, TESTCASE (test_pack_gsm7_24_chars, NULL));
+    g_test_suite_add (suite, TESTCASE (test_pack_gsm7_last_septet_alone, NULL));
+
+    g_test_suite_add (suite, TESTCASE (test_pack_gsm7_7_chars_offset, NULL));
 
     result = g_test_run ();