Re: [PATCH 2/2] ASoC: topology: Fix a small leak in soc_tplg_dapm_graph_elems_load()

["Amadeusz Sławiński" <amadeuszx.slawinski@linux.intel.com>]

On 5/20/2021 7:07 AM, Dan Carpenter wrote:
> We have to kfree(routes) on this error path.
> 
> Fixes: ff9226224437 ("ASoC: topology: Change allocations to resource managed")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
>   sound/soc/soc-topology.c | 7 +++++--
>   1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
> index 6b7a813bc264..5730fcaa7bc6 100644
> --- a/sound/soc/soc-topology.c
> +++ b/sound/soc/soc-topology.c
> @@ -1135,8 +1135,10 @@ static int soc_tplg_dapm_graph_elems_load(struct soc_tplg *tplg,
>   	 */
>   	for (i = 0; i < count; i++) {
>   		routes[i] = devm_kzalloc(tplg->dev, sizeof(*routes[i]), GFP_KERNEL);
> -		if (!routes[i])
> -			return -ENOMEM;
> +		if (!routes[i]) {
> +			ret = -ENOMEM;
> +			goto free_routes;
> +		}
>   	}
>   
>   	for (i = 0; i < count; i++) {
> @@ -1198,6 +1200,7 @@ static int soc_tplg_dapm_graph_elems_load(struct soc_tplg *tplg,
>   	 * The memory allocated for each dapm route will be freed
>   	 * when it is removed in remove_route().
>   	 */
> +free_routes:
>   	kfree(routes);
>   
>   	return ret;
> 

Yes, that's right, however looking at this function again, I wonder if 
instead we can just get rid of the routes array and kcalloc call?

Something along those lines (hope that copy paste won't mess it up):



diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
index 73076d425efb..13db9cfe223f 100644
--- a/sound/soc/soc-topology.c
+++ b/sound/soc/soc-topology.c
@@ -1104,7 +1104,7 @@ static int soc_tplg_dapm_graph_elems_load(struct 
soc_tplg *tplg,
  {
         struct snd_soc_dapm_context *dapm = &tplg->comp->dapm;
         struct snd_soc_tplg_dapm_graph_elem *elem;
-       struct snd_soc_dapm_route **routes;
+       struct snd_soc_dapm_route *route;
         int count, i;
         int ret = 0;

@@ -1122,28 +1122,15 @@ static int soc_tplg_dapm_graph_elems_load(struct 
soc_tplg *tplg,
         dev_dbg(tplg->dev, "ASoC: adding %d DAPM routes for index 
%d\n", count,
                 hdr->index);

-       /* allocate memory for pointer to array of dapm routes */
-       routes = kcalloc(count, sizeof(struct snd_soc_dapm_route *),
-                        GFP_KERNEL);
-       if (!routes)
-               return -ENOMEM;
-
-       /*
-        * allocate memory for each dapm route in the array.
-        * This needs to be done individually so that
-        * each route can be freed when it is removed in remove_route().
-        */
         for (i = 0; i < count; i++) {
-               routes[i] = devm_kzalloc(tplg->dev, sizeof(*routes[i]), 
GFP_KERNEL);
-               if (!routes[i])
+               route = devm_kzalloc(tplg->dev, sizeof(*route), GFP_KERNEL);
+               if (!route)
                         return -ENOMEM;
-       }

-       for (i = 0; i < count; i++) {
                 elem = (struct snd_soc_tplg_dapm_graph_elem *)tplg->pos;
                 tplg->pos += sizeof(struct snd_soc_tplg_dapm_graph_elem);

-               /* validate routes */
+               /* validate route */
                 if (strnlen(elem->source, SNDRV_CTL_ELEM_ID_NAME_MAXLEN) ==
                             SNDRV_CTL_ELEM_ID_NAME_MAXLEN) {
                         ret = -EINVAL;
@@ -1160,46 +1147,32 @@ static int soc_tplg_dapm_graph_elems_load(struct 
soc_tplg *tplg,
                         break;
                 }

-               routes[i]->source = elem->source;
-               routes[i]->sink = elem->sink;
+               route->source = elem->source;
+               route->sink = elem->sink;

                 /* set to NULL atm for tplg users */
-               routes[i]->connected = NULL;
+               route->connected = NULL;
                 if (strnlen(elem->control, 
SNDRV_CTL_ELEM_ID_NAME_MAXLEN) == 0)
-                       routes[i]->control = NULL;
+                       route->control = NULL;
                 else
-                       routes[i]->control = elem->control;
+                       route->control = elem->control;

                 /* add route dobj to dobj_list */
-               routes[i]->dobj.type = SND_SOC_DOBJ_GRAPH;
-               routes[i]->dobj.ops = tplg->ops;
-               routes[i]->dobj.index = tplg->index;
-               list_add(&routes[i]->dobj.list, &tplg->comp->dobj_list);
+               route->dobj.type = SND_SOC_DOBJ_GRAPH;
+               route->dobj.ops = tplg->ops;
+               route->dobj.index = tplg->index;
+               list_add(&route->dobj.list, &tplg->comp->dobj_list);

-               ret = soc_tplg_add_route(tplg, routes[i]);
+               ret = soc_tplg_add_route(tplg, route);
                 if (ret < 0) {
                         dev_err(tplg->dev, "ASoC: topology: add_route 
failed: %d\n", ret);
-                       /*
-                        * this route was added to the list, it will
-                        * be freed in remove_route() so increment the
-                        * counter to skip it in the error handling
-                        * below.
-                        */
-                       i++;
                         break;
                 }

                 /* add route, but keep going if some fail */
-               snd_soc_dapm_add_routes(dapm, routes[i], 1);
+               snd_soc_dapm_add_routes(dapm, route, 1);
         }

-       /*
-        * free pointer to array of dapm routes as this is no longer needed.
-        * The memory allocated for each dapm route will be freed
-        * when it is removed in remove_route().
-        */
-       kfree(routes);
-
         return ret;
  }