From cb0f12a10ee309521c74a672406501aa95e360f1 Mon Sep 17 00:00:00 2001
From: Aleksander Morgado <aleksander@aleksander.es>
Date: Tue, 6 Sep 2016 13:41:41 +0200
Subject: [PATCH] qmicli: refactor qmicli_read_binary_array_from_string()
 helper

The original implementation actually had some bugs when freeing the output
array in error conditions. Also, use g_ascii_xdigit_value() instead of
custom conversions.
---
 src/qmicli/qmicli-helpers.c    | 49 ++++++++++++++----------------
 src/qmicli/test/test-helpers.c | 55 ++++++++++++++++++++++++++++++++++
 2 files changed, 77 insertions(+), 27 deletions(-)

diff --git a/src/qmicli/qmicli-helpers.c b/src/qmicli/qmicli-helpers.c
index 801e7fd..ec72909 100644
--- a/src/qmicli/qmicli-helpers.c
+++ b/src/qmicli/qmicli-helpers.c
@@ -16,14 +16,13 @@
  * along with this program.  If not, see <http://www.gnu.org/licenses/>.
  *
  * Copyright (C) 2015 Velocloud Inc.
- * Copyright (C) 2012-2015 Aleksander Morgado <aleksander@aleksander.es>
+ * Copyright (C) 2012-2016 Aleksander Morgado <aleksander@aleksander.es>
  */
 
 #include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
 #include <errno.h>
-#include <ctype.h>
 
 #include "qmicli-helpers.h"
 
@@ -317,41 +316,37 @@ qmicli_read_firmware_id_from_string (const gchar *str,
 
 gboolean
 qmicli_read_binary_array_from_string (const gchar *str,
-                                      GArray **out) {
-    char a;
-    gsize i,len;
-    if (!str) return FALSE;
+                                      GArray **out)
+{
+    gsize i, j, len;
 
-    if((len = strlen(str)) & 1) return FALSE;
+    g_return_val_if_fail (out != NULL, FALSE);
+    g_return_val_if_fail (str, FALSE);
 
-    *out = g_array_sized_new(FALSE, TRUE, 1, len >> 1);
-    g_array_set_size(*out, len >> 1);
+    /* Length must be a multiple of 2 */
+    len = strlen (str);
+    if (len & 1)
+        return FALSE;
 
-    for (i = 0; i < len; i++) {
-        a = toupper(str[i]);
-        if (!isxdigit(a))
-            break;
-        if (isdigit(a)) {
-            a -= '0';
-        } else {
-            a = a - 'A' + 10;
+    *out = g_array_sized_new (FALSE, TRUE, sizeof (guint8), len >> 1);
+    g_array_set_size (*out, len >> 1);
+
+    for (i = 0, j = 0; i < len; i += 2, j++) {
+        gint a, b;
+
+        a = g_ascii_xdigit_value (str[i]);
+        b = g_ascii_xdigit_value (str[i + 1]);
+        if (a < 0 || b < 0) {
+            g_array_unref (*out);
+            return FALSE;
         }
 
-        if (i & 1) {
-            g_array_index(*out, gchar, i >> 1) |= a;
-        } else {
-            g_array_index(*out, gchar, i >> 1) = a<<4;
-        }
-    }
-    if (i < len) {
-        g_free(out);
-        out = NULL;
+        g_array_index (*out, guint8, j) = (a << 4) | b;
     }
 
     return TRUE;
 }
 
-
 gboolean
 qmicli_read_pdc_configuration_type_from_string (const gchar *str,
                                                 QmiPdcConfigurationType *out)
diff --git a/src/qmicli/test/test-helpers.c b/src/qmicli/test/test-helpers.c
index 9c2e597..7523c0a 100644
--- a/src/qmicli/test/test-helpers.c
+++ b/src/qmicli/test/test-helpers.c
@@ -13,6 +13,7 @@
  * Copyright (C) 2012-2016 Aleksander Morgado <aleksander@aleksander.es>
  */
 
+#include <string.h>
 #include <glib.h>
 #include "qmicli-helpers.h"
 
@@ -116,6 +117,55 @@ test_helpers_raw_printable_4 (void)
 
 /******************************************************************************/
 
+static void
+test_helpers_binary_array_from_string_1 (void)
+{
+    const guint8 expected[] = {
+        0x12, 0x34, 0x56, 0x78, 0x9A, 0xBC, 0xDE, 0xF0, 0xAB, 0xCD, 0xEF
+    };
+    const gchar *str = "123456789ABCDEF0abcdef";
+    GArray *out = NULL;
+
+    g_assert (qmicli_read_binary_array_from_string (str, &out));
+    g_assert (out != NULL);
+    g_assert_cmpuint (out->len, ==, strlen (str) / 2);
+    g_assert (memcmp ((guint8 *)(out->data), expected, out->len) == 0);
+
+    g_array_unref (out);
+}
+
+static void
+test_helpers_binary_array_from_string_2 (void)
+{
+    const gchar *str = "";
+    GArray *out = NULL;
+
+    g_assert (qmicli_read_binary_array_from_string (str, &out));
+    g_assert (out != NULL);
+    g_assert_cmpuint (out->len, ==, 0);
+    g_array_unref (out);
+}
+
+static void
+test_helpers_binary_array_from_string_3 (void)
+{
+    const gchar *str = "hello";
+    GArray *out = NULL;
+
+    g_assert (qmicli_read_binary_array_from_string (str, &out) == FALSE);
+}
+
+static void
+test_helpers_binary_array_from_string_4 (void)
+{
+    const gchar *str = "a";
+    GArray *out = NULL;
+
+    g_assert (qmicli_read_binary_array_from_string (str, &out) == FALSE);
+}
+
+/******************************************************************************/
+
 static void
 test_helpers_supported_messages_list (void)
 {
@@ -242,6 +292,11 @@ int main (int argc, char **argv)
     g_test_add_func ("/qmicli/helpers/raw-printable/3",  test_helpers_raw_printable_3);
     g_test_add_func ("/qmicli/helpers/raw-printable/4",  test_helpers_raw_printable_4);
 
+    g_test_add_func ("/qmicli/helpers/binary-array-from-string/1", test_helpers_binary_array_from_string_1);
+    g_test_add_func ("/qmicli/helpers/binary-array-from-string/2", test_helpers_binary_array_from_string_2);
+    g_test_add_func ("/qmicli/helpers/binary-array-from-string/3", test_helpers_binary_array_from_string_3);
+    g_test_add_func ("/qmicli/helpers/binary-array-from-string/4", test_helpers_binary_array_from_string_4);
+
     g_test_add_func ("/qmicli/helpers/supported-message-list",      test_helpers_supported_messages_list);
     g_test_add_func ("/qmicli/helpers/supported-message-list/none", test_helpers_supported_messages_list_none);