[PATCH] huawei plugin: fix memory leaks in tests.

[Aleksander Morgado]

On Wed, Mar 25, 2015 at 6:43 PM, Yunlian Jiang <yunlian at chromium.org> wrote:
>
> Please see the attachment for the updated patch.
> Indeed we have seen other memory leaks in unit-test,
> https://code.google.com/p/chromium/issues/detail?id=470252
> could you please take a look at it if possible?

In plugins/icera/test/test-modem-helpers-icera.c, test_ipdpaddr()
method, when mm_icera_parse_ipdpaddr_response() is called and it
returns either 'ipv4' or 'ipv6' variables, those don't get properly
released. Change would be:

        /* IPv4 */
        if (ipdpaddr_tests[i].ipv4_addr) {
            ...
            g_object_unref (ipv4);
        }

And same for ipv6.

Could you update that and re-check the memleak tests?

>
> ---
>  plugins/huawei/tests/test-modem-helpers-huawei.c | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/plugins/huawei/tests/test-modem-helpers-huawei.c b/plugins/huawei/tests/test-modem-helpers-huawei.c
> index 9e92eee..4b73bb3 100644
> --- a/plugins/huawei/tests/test-modem-helpers-huawei.c
> +++ b/plugins/huawei/tests/test-modem-helpers-huawei.c
> @@ -420,6 +420,7 @@ test_prefmode_response (void)
>          found = mm_huawei_parse_prefmode_response (prefmode_response_tests[i].str,
>                                                     combinations,
>                                                     &error);
> +        g_assert_no_error (error);
>          g_assert (found != NULL);
>          g_assert_cmpuint (found->allowed, ==, prefmode_response_tests[i].allowed);
>          g_assert_cmpuint (found->preferred, ==, prefmode_response_tests[i].preferred);
> @@ -672,6 +673,7 @@ test_syscfg_response (void)
>                                                   combinations,
>                                                   &error);
>
> +        g_assert_no_error (error);
>          g_assert (found != NULL);
>          g_assert_cmpuint (found->allowed, ==, syscfg_response_tests[i].allowed);
>          g_assert_cmpuint (found->preferred, ==, syscfg_response_tests[i].preferred);
> @@ -993,6 +995,7 @@ test_syscfgex_response (void)
>                                                     combinations,
>                                                     &error);
>
> +        g_assert_no_error (error);
>          g_assert (found != NULL);
>          g_assert_cmpuint (found->allowed, ==, syscfgex_response_tests[i].allowed);
>          g_assert_cmpuint (found->preferred, ==, syscfgex_response_tests[i].preferred);
> @@ -1085,8 +1088,10 @@ test_nwtime (void)
>              g_assert (nwtime_tests[i].leap_seconds == mm_network_timezone_get_leap_seconds (tz));
>          }
>
> -        if (iso8601)
> -            g_free (iso8601);
> +        g_free (iso8601);
> +
> +       if (tz)
> +           g_object_unref (tz);
>      }
>  }
>
> @@ -1122,14 +1127,16 @@ test_time (void)
>                                               &iso8601,
>                                               NULL,
>                                               &error);
> -
> -        g_assert (ret == time_tests[i].ret);
> -        g_assert (ret == (error ? FALSE : TRUE));
> +       if (ret)
> +            g_assert_no_error (error);
> +       else {
> +            g_assert (ret == time_tests[i].ret);
> +            g_assert (ret == (error ? FALSE : TRUE));
> +            g_error_free (error);
> +        }
>

I was wrong; error vs ret was being checked in the following line:
   g_assert (ret == (error ? FALSE : TRUE));

So there's no need for the extra checks, just the memleak should get fixed:
   if (error)
      g_error_free (error);

Could you update the patch?


>          g_assert_cmpstr (time_tests[i].iso8601, ==, iso8601);
> -
> -        if (iso8601)
> -            g_free (iso8601);
> +        g_free (iso8601);
>      }
>  }
>
> --
> 2.2.0.rc0.207.ga3a616c
>
> On Wed, Mar 25, 2015 at 10:10 AM, Aleksander Morgado <aleksander at aleksander.es> wrote:
>>
>> On Wed, Mar 25, 2015 at 5:30 PM, Yunlian Jiang <yunlian at chromium.org> wrote:
>> > ---
>> >  plugins/huawei/tests/test-modem-helpers-huawei.c | 5 +++++
>> >  1 file changed, 5 insertions(+)
>> >
>> > diff --git a/plugins/huawei/tests/test-modem-helpers-huawei.c
>> > b/plugins/huawei/tests/test-modem-helpers-huawei.c
>> > index 9e92eee..ef1118a 100644
>> > --- a/plugins/huawei/tests/test-modem-helpers-huawei.c
>> > +++ b/plugins/huawei/tests/test-modem-helpers-huawei.c
>> > @@ -1087,6 +1087,9 @@ test_nwtime (void)
>> >
>> >          if (iso8601)
>> >              g_free (iso8601);
>>
>> I know this is not part of your patch, but there's no need to check
>> for NULL before doing g_free(). I'll gladly accept a patch revising
>> this whole file and changing those.
>>
>> > +
>> > +       if (tz)
>> > +           g_free (tz);
>>
>> tz is actually a MMNetworkTimezone, which is a GObject. The proper fix would be:
>>    if (tz)
>>       g_object_unref (tz);
>>
>> >      }
>> >  }
>> >
>> > @@ -1128,6 +1131,8 @@ test_time (void)
>> >
>> >          g_assert_cmpstr (time_tests[i].iso8601, ==, iso8601);
>> >
>> > +       g_clear_error (&error);
>> > +
>>
>> Looks like the test never checks if error is set.
>>
>> I'd update it so that:
>>
>>    if (ret)
>>       g_assert_no_error (error);
>>    else {
>>       g_assert_error (error, ....);
>>       g_error_free (error);
>>    }
>>
>> Could you also review the remaining tests to check if error is being
>> checked or not?
>>
>>
>> >          if (iso8601)
>> >              g_free (iso8601);
>>
>> Same as above, this wasn't part of your patch, but I would gladly
>> accept a patch changing that to just do a g_free().
>>
>> >      }
>> > --
>> > 2.2.0.rc0.207.ga3a616c
>> >
>> >
>> > _______________________________________________
>> > ModemManager-devel mailing list
>> > ModemManager-devel at lists.freedesktop.org
>> > http://lists.freedesktop.org/mailman/listinfo/modemmanager-devel
>> >
>>
>>
>>
>> --
>> Aleksander
>> https://aleksander.es
>
>



-- 
Aleksander
https://aleksander.es