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

Proposed by Dimitri John Ledkov on 2014-06-10
Status: Needs review
Proposed branch: lp:~xnox/whoopsie/fix-realloc
Merge into: lp:whoopsie
Diff against target: 40 lines (+4/-7)
2 files modified
src/identifier.c (+1/-5)
src/tests/test_identifier.c (+3/-2)
To merge this branch: bzr merge lp:~xnox/whoopsie/fix-realloc
Reviewer Review Type Date Requested Status
John Lenton 2014-06-10 Disapprove on 2014-06-10
Evan (community) Approve on 2014-06-10
Review via email: mp+222584@code.launchpad.net
To post a comment you must log in.
Evan (ev) wrote :

Yikes. Thanks for fixing this!

review: Approve
Evan (ev) wrote :

I'll let John have a look before merging.

John Lenton (chipaca) wrote :

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) wrote :

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) wrote :

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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/identifier.c'
2--- src/identifier.c 2014-05-13 11:13:30 +0000
3+++ src/identifier.c 2014-06-10 02:56:32 +0000
4@@ -228,11 +228,7 @@
5 return;
6 }
7
8- size_t old_len = *result ? strlen(*result) : 0;
9-
10- *result = g_realloc (*result, old_len + imei_len + 1);
11- strcpy (*result + old_len, imei);
12-
13+ *result = g_strdup_printf ("%s%s", *result, imei);
14 g_variant_unref(var4);
15 }
16
17
18=== modified file 'src/tests/test_identifier.c'
19--- src/tests/test_identifier.c 2014-05-13 11:13:30 +0000
20+++ src/tests/test_identifier.c 2014-06-10 02:56:32 +0000
21@@ -385,7 +385,7 @@
22 static void
23 test_get_imei (void)
24 {
25- char* res = NULL;
26+ char* res = "bananas";
27 GError* error = NULL;
28
29 brofono_start ();
30@@ -393,8 +393,9 @@
31
32 whoopsie_identifier_append_imei (&res, &error);
33 g_assert_no_error (error);
34- g_assert (res != NULL);
35+ g_assert_cmpstr (res, ==,"bananas* not an imei *");
36 brofono_stop ();
37+ g_free (res);
38 }
39
40 static void

Subscribers

People subscribed via source and target branches

to status/vote changes: