Merge lp:~xnox/whoopsie/fix-realloc into lp:whoopsie

[Dimitri John Ledkov]

[Evan (ev)]

Yikes. Thanks for fixing this!

review: Approve

[Evan (ev)]

I'll let John have a look before merging.

[John Lenton (chipaca)]

The bug is real (it's a bug in the tests), the fix is wrong.

What needs doing is using a g_malloc'ed string in the tests, so that it's the same flavour of memory in both places.

review: Disapprove

[Dimitri John Ledkov (xnox)]

On 10 June 2014 09:31, John Lenton <email address hidden> wrote:
> Review: Disapprove
>
> The bug is real (it's a bug in the tests), the fix is wrong.
>
> What needs doing is using a g_malloc'ed string in the tests, so that it's the same flavour of memory in both places.

Well, the type accepted is char **, that is an array of pointers of
pointers to chars.
It's not const, thus the function may mutate the pointers as it likes,
within reason.
In practice it only mutates it, if it decides to append values to it.
A pointer to string literal is "const char *" but there is no easy way
to detect "const" when dereferencing "char **", thus imho
implementation should be resilient to dealing with string literals as
we can't compile time enforce them.

Ideally one would use str_array as e.g. provide by more mature libinh
http://libnih.la/nih-String.html#nih-str-array-new library, all
strings there are are always reallocated since it's always valid to
append a string literal to the string array.

Why does imei function mutate first string in the array, instead of
prepending/appending another value to the array?

--
Regards,

Dimitri.

[Dimitri John Ledkov (xnox)]

On 10 June 2014 09:31, John Lenton <email address hidden> wrote:
> Review: Disapprove
>
> The bug is real (it's a bug in the tests), the fix is wrong.
>
> What needs doing is using a g_malloc'ed string in the tests, so that it's the same flavour of memory in both places.

Hm, but it's also scary to change external variable from non-malloced
to malloced and thus causing them to have a memory leak. (cause
external code is not freeing it already)

--
Regards,

Dimitri.

Unmerged revisions

611. By Dimitri John Ledkov on 2014-06-10

Do not try to realloc string literals.

Make a failing test-case, then fix code to use strdup (safe) instead
of doing realloc.

=== modified file 'src/identifier.c'
--- src/identifier.c	2014-05-13 11:13:30 +0000
+++ src/identifier.c	2014-06-10 02:56:32 +0000
@@ -228,11 +228,7 @@
         return;
     }
 
-    size_t old_len = *result ? strlen(*result) : 0;
-
-    *result = g_realloc (*result, old_len + imei_len + 1);
-    strcpy (*result + old_len, imei);
-
+    *result = g_strdup_printf ("%s%s", *result, imei);
     g_variant_unref(var4);
 }
 
 
=== modified file 'src/tests/test_identifier.c'
--- src/tests/test_identifier.c	2014-05-13 11:13:30 +0000
+++ src/tests/test_identifier.c	2014-06-10 02:56:32 +0000
@@ -385,7 +385,7 @@
 static void
 test_get_imei (void)
 {
-    char* res = NULL;
+    char* res = "bananas";
     GError* error = NULL;
 
     brofono_start ();
@@ -393,8 +393,9 @@
 
     whoopsie_identifier_append_imei (&res, &error);
     g_assert_no_error (error);
-    g_assert (res != NULL);
+    g_assert_cmpstr (res, ==,"bananas* not an imei *");
     brofono_stop ();
+    g_free (res);
 }
 
 static void