Re: [dpdk-dev] [PATCH] lib/metrics: fix memory leak

Hello Gaurav,

On Tue, Sep 22, 2020 at 12:27 PM Power, Ciara [off-list ref] wrote:

quoted

-----Original Message-----
From: dev <redacted> On Behalf Of Honnappa Nagarahalli
Sent: Tuesday 11 August 2020 22:01
To: Gaurav Singh <redacted>; dev@dpdk.org
Cc: nd <redacted>; Honnappa Nagarahalli
[off-list ref]; nd [off-list ref]
Subject: Re: [dpdk-dev] [PATCH] lib/metrics: fix memory leak

<snip>

quoted

fix memory leak

Fixes: c5b7197f66 ("telemetry: move some functions to metrics
library")

Signed-off-by: Gaurav Singh <redacted>
---
 lib/librte_metrics/rte_metrics_telemetry.c | 21 ++++++++++++++++-----
 1 file changed, 16 insertions(+), 5 deletions(-)

I think this commit message should be more descriptive, and is missing Cc: stable@dpdk.org

quoted

quoted

diff --git a/lib/librte_metrics/rte_metrics_telemetry.c

b/lib/librte_metrics/rte_metrics_telemetry.c
index 289ebae0b..7b6d1063c 100644

--- a/lib/librte_metrics/rte_metrics_telemetry.c
+++ b/lib/librte_metrics/rte_metrics_telemetry.c

@@ -41,12 +41,17 @@

rte_metrics_tel_reg_port_ethdev_to_metrics(uint16_t
port_id)
     }

     xstats_names = malloc(sizeof(*xstats_names) * num_xstats);
+    if (xstats_names == NULL) {
+            METRICS_LOG_ERR("Failed to malloc memory for
xstats_names");
+            return -ENOMEM;
+    }
+
     eth_xstats_names = malloc(sizeof(struct rte_eth_xstat_name)
                     * num_xstats);
-    if (eth_xstats_names == NULL || xstats_names == NULL) {
-            METRICS_LOG_ERR("Failed to malloc memory for
xstats_names");
-            ret = -ENOMEM;
-            goto free_xstats;
+    if (eth_xstats_names == NULL) {
+            METRICS_LOG_ERR("Failed to malloc memory for
eth_xstats_names");
+            free(xstats_names);
+            return -ENOMEM;
     }

Is there a reason for the above changes? I think they are unrelated to
the memory leak this patch is fixing.

quoted

quoted

     if (rte_eth_xstats_get_names(port_id,

@@ -167,9 +172,15 @@ rte_metrics_tel_format_port(uint32_t pid, json_t

*ports,
     }

     metrics = malloc(sizeof(struct rte_metric_value) * num_metrics);
+    if (metrics == NULL) {
+            METRICS_LOG_ERR("Cannot allocate memory");
+            return -ENOMEM;
+    }
+
     names = malloc(sizeof(struct rte_metric_name) * num_metrics);
-    if (metrics == NULL || names == NULL) {
+    if (names == NULL) {
             METRICS_LOG_ERR("Cannot allocate memory");
+            free(metrics);
             return -ENOMEM;
     }

This does fix the resource leak, but I do think it can be done in a
simpler way, as shown in the patch I sent to fix the logged coverity issue
for this: https://patchwork.dpdk.org/patch/78052/
I will remove my patch seeing as this patch is fixing the same thing.

I agree with Ciara.
Could you respin?

Thanks.


-- 
David Marchand