libmm-glib usage: freeing object allocations correctly

[Aleksander Morgado]

Hey,

> I am using the libmm-glib API to query bearer status periodically and
> synchronously from MM(v1.6.4) from my own linux process.  I have used mmcli
> as an example as was suggested on this forum previously.
>
> (https://lists.freedesktop.org/archives/modemmanager-devel/2017-September/005700.html)
>
> After few queries of bearer status, I see my process leaking memory. I think
> I am freeing all the allocations using g_object_unref and g_free but not
> sure what I am leaving behind.
>
> I am not very familiar with glib memory management routines or with
> libmm-glib internals and so I am asking for help here hoping someone can
> give me some direction.
>

Run your program as:
$ G_SLICE=always-malloc valgrind --leak-check=full your-program .....

And then look at the "definitely lost" memory leak blocks valgrind
reports. You'll see exactly where the leaks are happening.

Anyway, see below more commetns.


>
>
> The following function is called periodically to get the bearer object(to
> read bearer connection status) and I have marked the object memory
> allocations and frees. I think I’m freeing all the allocations yet I see my
> process leaking memory due to calling this function.
>
> Is there something obviously wrong? Is there a better way to query bearer
> connectivity status from MM?
>

Yes, there is something wrong, why are you periodically doing all
those checks!?? :)
You could do it once and rely on asynchronous "real time" updates
happening on the background.

>
>
> func_get_bearer() {
>
>
>
> /* ALLOC 1 */
>
> ctx = g_new0(Context, 1); /* Context a struct which has MMManager MMObject,
> MMMOdem etc. */
>
>
>
> /* ALLOC 2 */
>
> mgr = mm_manager_new_sync(connection_object,
>
>
> G_DBUS_OBJECT_MANAGER_CLIENT_FLAGS_DO_NOT_AUTO_START,
>
>                           NULL, &error);
>

If mgr is NULL, error is set. You'd need to catch that and
g_error_free() the GError.

>
>
> /* ALLOC 3 */
>
> name =
> g_dbus_object_manager_client_get_name_owner(G_DBUS_OBJECT_MANAGER_CLIENT(mgr));
>
> if(name == NULL)
>
> {
>
>    g_object_unref(mgr);
>
>    /* Error */
>
>    return;
>
> }
>
> /* FREE 3 */
>
> g_free(name);
>
>
>
> /* Get modem path */
>
> /* ALLOC 4 */
>
> modem_path = g_strdup_printf(MM_DBUS_MODEM_PREFIX "/%s", modem_id); /*
> modem_id may be “0” */
>

This is not ok, you cannot just rely on  a specific modem index. If
the modem resets itself it will get a new index, it won't be 0.

>
>
> /* Get modem object */
>
> /* ALLOC 5 */
>
> modems = g_dbus_object_manager_get_objects(G_DBUS_OBJECT_MANAGER(mgr));
>
> for(mdm = modems; mdm; mdm = g_list_next(mdm))
>
> {
>
>    mdm = MM_OBJECT(mdm->data);

Wait, this is not right. "mdm" is what? "mdm" was a GList (I assume)
because you use it to iterate the "modems" list, you shouldn't re-use
it to cast mdm->data because then the list iteration will break.
You could do:

MMObject *iter = MM_OBJECT(mdm->data);


>
>    if((modem_path != NULL) && g_str_equal(mm_object_get_path(mm_obj),
> modem_path))
>
>    {

As said, don't compare from the list against a predefined index, that
is not right, won't work as you expect. If you only expect one modem
in your system, as soon as you find a modem reported, that is the one
you want, regardless of whether it's index 0 or 1 or whatever.

>
>       /* ALLOC 6 */
>
>       found = g_object_ref(mm_obj);
>
>       break;
>
>    }
>
> }
>
> /* FREE 5 */
>
> g_list_free_full(modems, g_object_unref);
>
> /* FREE 4 */
>
> g_free(modem_path);
>
>
>
> ctx->m_generic_object = found;
>
>
>
> /* ALLOC 7 */
>
> ctx->mdm_intferface_obj = mm_object_get_modem(ctx->m_generic_object);
>
>
>
> /* Get list of bearers */
>
> /* ALLOC 8 */
>
> b_list = mm_modem_list_bearers_sync(ctx->mdm_intferface_obj, NULL, &error);

If b_list is NULL, error is set, so you would need to catch that and
g_error_free the GError.

>
>
>
> /* Construct bearer path */
>
> /* ALLOC 9 */
>
> bearer_path = g_strdup_printf(MM_DBUS_BEARER_PREFIX "/%s", path_or_idx);
>

This is also not needed at all, don't match against a specific index again.

>
>
> /* Get bearer object */
>
> GList *bearer;
>
> MMBearer *bearer_obj;
>
>
>
> for(bearer = b_list; bearer; bearer = g_list_next(bearer))
>
> {
>
>    bearer_obj = MM_BEARER(bearer->data);
>
>    if(bearer_obj == NULL)
>
>    {
>
>       continue;
>
>    }

This previous if can never happen. The list of bearers will always
have a valid data.

>
>    if(g_str_equal(mm_bearer_get_path(bearer_obj), bearer_path))
>
>    {

As said earlier, don't match, that's wrong. You can just rely on "if I
find one bearer, that's the one I want".

>
>       /* Got bearer object */
>
>       /* ALLOC 10 */
>
>       ctx->bearer_obj = g_object_ref(bearer_obj);
>
>    }
>
> }
>
>
>
> /* Free the allocations */
>
> /* FREE 8 */
>
> g_list_free_full(b_list, g_object_unref);
>
>
>
> /* FREE 10 */
>
> g_object_unref(ctx->bearer_obj);
>
> /* FREE 6 */
>
> g_object_unref(ctx->m_generic_object);
>
> /* FREE 7 */
>
> g_object_unref(ctx->mdm_intferface_obj);
>
> /* FREE 2 */
>
> g_object_unref(ctx->mgr);
>
>
>
> /* FREE 9 */
>
> g_free(bearer_path);
>
> /* FREE 1 */
>
> g_free(ctx);
>
>
>
> } /* End */
>
>

I think you need to run valgrind to see exactly where it's leaking
memory. Also, you may want to use a newer ModemManager, 1.6.4 is
extremely old, not even the latest in the 1.6.x series, so leaks in
libmm-glib itself may have been fixed already.

-- 
Aleksander
https://aleksander.es