Re: [PATCH] tools: image-host: Fix potential memory leak

[Quentin Schulz]

Hi,

On 4/18/25 10:19 AM, ant.v.morya...@gmail.com wrote:
From: Maks Mishin <maks.mishi...@gmail.com>
Signed-off-by: Maks Mishin <maks.mishi...@gmail.com>
---
  tools/image-host.c | 7 +++++--
  1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/tools/image-host.c b/tools/image-host.c
index 4a24dee8..6b17b810 100644
--- a/tools/image-host.c
+++ b/tools/image-host.c
@@ -1024,10 +1024,13 @@ static int fit_config_process_sig(const char *keydir, 
const char *keyfile,
        int ret;
  
  	node_name = fit_get_name(fit, noffset, NULL);
-       if (fit_config_get_regions(fit, conf_noffset, noffset, &region,
+       ret = fit_config_get_regions(fit, conf_noffset, noffset, &region,
                                   &region_count, &region_prop,
-                                  &region_proplen))
+                                  &region_proplen);
+       if (ret) {
+               free(region_prop);

We have a handful of other error code paths after that that would need 
to free region_prop as well.

value is likely not freed "enough" as well, please check.

Same for region.

I would suggest possibly something like:

diff --git a/tools/image-host.c b/tools/image-host.c
index a9b86902763..f97af2109ad 100644
--- a/tools/image-host.c
+++ b/tools/image-host.c
@@ -1076,51 +1076,54 @@ static int fit_config_process_sig(const char 
*keydir, const char *keyfile,
 {
        struct image_sign_info info;
        const char *node_name;
-       struct image_region *region;
-       char *region_prop;
+       struct image_region *region = NULL;
+       char *region_prop = NULL;
        int region_proplen;
        int region_count;
-       uint8_t *value;
+       uint8_t *value = NULL;
        uint value_len;
        int ret;

        node_name = fit_get_name(fit, noffset, NULL);
        if (fit_config_get_regions(fit, conf_noffset, noffset, &region,
                                   &region_count, &region_prop,
-                                  &region_proplen))
-               return -1;
+                                  &region_proplen)) {
+               ret = -1;
+               goto out;
+       }

        if (fit_image_setup_sig(&info, keydir, keyfile, fit, conf_name, noffset,
                                require_keys ? "conf" : NULL, engine_id,
-                               algo_name))
-               return -1;
+                               algo_name)) {
+               ret = -1;
+               goto out;
+       }

        ret = info.crypto->sign(&info, region, region_count, &value,
                                &value_len);
-       free(region);
        if (ret) {
 		fprintf(stderr, "Failed to sign '%s' signature node in '%s' conf 
node\n",
                        node_name, conf_name);

                /* We allow keys to be missing */
-               if (ret == -ENOENT)
-                       return 0;
-               return -1;
+               ret = (ret == -ENOENT) ? 0 : -1;
+               goto out;
        }

        ret = fit_image_write_sig(fit, noffset, value, value_len, comment,
                                  region_prop, region_proplen, cmdname,
                                  algo_name);
        if (ret) {
-               if (ret == -FDT_ERR_NOSPACE)
-                       return -ENOSPC;
+               if (ret == -FDT_ERR_NOSPACE) {
+                       ret = -ENOSPC;
+                       goto out;
+               }
                fprintf(stderr,
 			"Can't write signature for '%s' signature node in '%s' conf node: 
%s\n",
                        node_name, conf_name, fdt_strerror(ret));
-               return -1;
+               ret = -1;
+               goto out;
        }
-       free(value);
-       free(region_prop);

        /* Get keyname again, as FDT has changed and invalidated our pointer */
        info.keyname = fdt_getprop(fit, noffset, FIT_KEY_HINT, NULL);
@@ -1133,10 +1136,14 @@ static int fit_config_process_sig(const char 
*keydir, const char *keyfile,
 				"Failed to add verification data for '%s' signature node in '%s' 
configuration node\n",
                                node_name, conf_name);
                }
-               return ret;
        }

-       return 0;
+out:
+       free(value);
+       free(region_prop);
+       free(region);
+
+       return ret;
 }

 static int fit_config_add_verification_data(const char *keydir,


NOT TESTED!

Cheers,
Quentin