2019-07-03 23:04:04

by Stotland, Inga

Subject: [PATCH BlueZ] mesh: Fix clean up of node's DBUS assosiated resources

This consolidates multiple places where node's object paths,
interfaces, etc. are de-allocated, into one routine:
free_node_dbus_resources().
This also addresses memory leaks assosiated with inconsistent freeing
of object path strings.
---
mesh/node.c | 66 +++++++++++++++++++++++------------------------------
1 file changed, 29 insertions(+), 37 deletions(-)

diff --git a/mesh/node.c b/mesh/node.c
index adc2aa93e..67e0dd014 100644
--- a/mesh/node.c
+++ b/mesh/node.c
@@ -226,34 +226,46 @@ static void element_free(void *data)
l_free(element);
}

-static void free_node_resources(void *data)
+static void free_node_dbus_resources(struct mesh_node *node)
{
- struct mesh_node *node = data;
+ if (!node)
+ return;

- /* Unregister io callbacks */
- if (node->net)
- mesh_net_detach(node->net);
- mesh_net_free(node->net);
+ if (node->disc_watch) {
+ l_dbus_remove_watch(dbus_get_bus(), node->disc_watch);
+ node->disc_watch = 0;
+ }

- l_queue_destroy(node->elements, element_free);
- l_free(node->comp);
- l_free(node->app_path);
+ l_queue_foreach(node->elements, free_element_path, NULL);
l_free(node->owner);
- l_free(node->node_path);
-
- if (node->disc_watch)
- l_dbus_remove_watch(dbus_get_bus(), node->disc_watch);
+ node->owner = NULL;
+ l_free(node->app_path);
+ node->app_path = NULL;

if (node->path) {
l_dbus_object_remove_interface(dbus_get_bus(), node->path,
MESH_NODE_INTERFACE);

l_dbus_object_remove_interface(dbus_get_bus(), node->path,
- MESH_MANAGEMENT_INTERFACE);
+ MESH_MANAGEMENT_INTERFACE);
+ l_free(node->path);
+ node->path = NULL;
}
+}

- l_free(node->path);
+static void free_node_resources(void *data)
+{
+ struct mesh_node *node = data;

+ /* Unregister io callbacks */
+ if (node->net)
+ mesh_net_detach(node->net);
+ mesh_net_free(node->net);
+
+ l_queue_destroy(node->elements, element_free);
+ l_free(node->comp);
+
+ free_node_dbus_resources(node);
l_free(node);
}

@@ -1033,24 +1045,9 @@ static void app_disc_cb(struct l_dbus *bus, void *user_data)
l_info("App %s disconnected (%u)", node->owner, node->disc_watch);

node->disc_watch = 0;
-
- l_queue_foreach(node->elements, free_element_path, NULL);
-
- l_free(node->owner);
- node->owner = NULL;
-
- if (node->path) {
- l_dbus_object_remove_interface(dbus_get_bus(), node->path,
- MESH_NODE_INTERFACE);
-
- l_dbus_object_remove_interface(dbus_get_bus(), node->path,
- MESH_MANAGEMENT_INTERFACE);
- l_free(node->app_path);
- node->app_path = NULL;
- }
+ free_node_dbus_resources(node);
}

-
static bool validate_model_property(struct node_element *ele,
struct l_dbus_message_iter *property,
uint8_t *num_models, bool vendor)
@@ -1611,14 +1608,9 @@ fail:
/* Handle failed Attach request */
node_ready_func_t cb = req->cb;

- l_queue_foreach(node->elements, free_element_path, NULL);
- l_free(node->app_path);
- node->app_path = NULL;
+ free_node_dbus_resources(node);

- l_free(node->owner);
- node->owner = NULL;
cb(req->user_data, MESH_ERROR_FAILED, node);
-
} else {
/* Handle failed Join and Create requests */
if (node)
--
2.21.0

2019-07-04 00:18:45

by Gix, Brian

Subject: Re: [PATCH BlueZ] mesh: Fix clean up of node's DBUS assosiated resources

Ap[plied

On Wed, 2019-07-03 at 16:02 -0700, Inga Stotland wrote:
> This consolidates multiple places where node's object paths,
> interfaces, etc. are de-allocated, into one routine:
> free_node_dbus_resources().
> This also addresses memory leaks assosiated with inconsistent freeing
> of object path strings.
> ---
> mesh/node.c | 66 +++++++++++++++++++++++------------------------------
> 1 file changed, 29 insertions(+), 37 deletions(-)
>
> diff --git a/mesh/node.c b/mesh/node.c
> index adc2aa93e..67e0dd014 100644
> --- a/mesh/node.c
> +++ b/mesh/node.c
> @@ -226,34 +226,46 @@ static void element_free(void *data)
> l_free(element);
> }
>
> -static void free_node_resources(void *data)
> +static void free_node_dbus_resources(struct mesh_node *node)
> {
> - struct mesh_node *node = data;
> + if (!node)
> + return;
>
> - /* Unregister io callbacks */
> - if (node->net)
> - mesh_net_detach(node->net);
> - mesh_net_free(node->net);
> + if (node->disc_watch) {
> + l_dbus_remove_watch(dbus_get_bus(), node->disc_watch);
> + node->disc_watch = 0;
> + }
>
> - l_queue_destroy(node->elements, element_free);
> - l_free(node->comp);
> - l_free(node->app_path);
> + l_queue_foreach(node->elements, free_element_path, NULL);
> l_free(node->owner);
> - l_free(node->node_path);
> -
> - if (node->disc_watch)
> - l_dbus_remove_watch(dbus_get_bus(), node->disc_watch);
> + node->owner = NULL;
> + l_free(node->app_path);
> + node->app_path = NULL;
>
> if (node->path) {
> l_dbus_object_remove_interface(dbus_get_bus(), node->path,
> MESH_NODE_INTERFACE);
>
> l_dbus_object_remove_interface(dbus_get_bus(), node->path,
> - MESH_MANAGEMENT_INTERFACE);
> + MESH_MANAGEMENT_INTERFACE);
> + l_free(node->path);
> + node->path = NULL;
> }
> +}
>
> - l_free(node->path);
> +static void free_node_resources(void *data)
> +{
> + struct mesh_node *node = data;
>
> + /* Unregister io callbacks */
> + if (node->net)
> + mesh_net_detach(node->net);
> + mesh_net_free(node->net);
> +
> + l_queue_destroy(node->elements, element_free);
> + l_free(node->comp);
> +
> + free_node_dbus_resources(node);
> l_free(node);
> }
>
> @@ -1033,24 +1045,9 @@ static void app_disc_cb(struct l_dbus *bus, void *user_data)
> l_info("App %s disconnected (%u)", node->owner, node->disc_watch);
>
> node->disc_watch = 0;
> -
> - l_queue_foreach(node->elements, free_element_path, NULL);
> -
> - l_free(node->owner);
> - node->owner = NULL;
> -
> - if (node->path) {
> - l_dbus_object_remove_interface(dbus_get_bus(), node->path,
> - MESH_NODE_INTERFACE);
> -
> - l_dbus_object_remove_interface(dbus_get_bus(), node->path,
> - MESH_MANAGEMENT_INTERFACE);
> - l_free(node->app_path);
> - node->app_path = NULL;
> - }
> + free_node_dbus_resources(node);
> }
>
> -
> static bool validate_model_property(struct node_element *ele,
> struct l_dbus_message_iter *property,
> uint8_t *num_models, bool vendor)
> @@ -1611,14 +1608,9 @@ fail:
> /* Handle failed Attach request */
> node_ready_func_t cb = req->cb;
>
> - l_queue_foreach(node->elements, free_element_path, NULL);
> - l_free(node->app_path);
> - node->app_path = NULL;
> + free_node_dbus_resources(node);
>
> - l_free(node->owner);
> - node->owner = NULL;
> cb(req->user_data, MESH_ERROR_FAILED, node);
> -
> } else {
> /* Handle failed Join and Create requests */
> if (node)