Hi Francois,
On 1/19/26 3:47 PM, Francois Berder wrote:
If an error occurs in fit_config_get_hash_list function,
one needs to free the node_inc string list.
Signed-off-by: Francois Berder <[email protected]>
---
tools/image-host.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/tools/image-host.c b/tools/image-host.c
index 48d69191c92..635a75afe5c 100644
--- a/tools/image-host.c
+++ b/tools/image-host.c
@@ -924,19 +924,23 @@ static int fit_config_get_hash_list(const void *fit, int
conf_noffset,
Aren't we also missing an strlist_free() before the goto err_mem?
I would suggest to rework the error paths so that they all end up with
the same logic and make it harder to forget about freeing the list,
something like:
"""
diff --git a/tools/image-host.c b/tools/image-host.c
index 54df86316ae..8f9343f65d9 100644
--- a/tools/image-host.c
+++ b/tools/image-host.c
@@ -894,8 +894,12 @@ static int fit_config_get_hash_list(const void
*fit, int conf_noffset,
strlist_init(node_inc);
snprintf(name, sizeof(name), "%s/%s", FIT_CONFS_PATH, conf_name);
if (strlist_add(node_inc, "/") ||
- strlist_add(node_inc, name))
- goto err_mem;
+ strlist_add(node_inc, name)) {
+ fprintf(stderr, "Out of memory processing configuration
'%s/%s'\n",
+ conf_name, sig_name);
+ ret = -ENOMEM;
+ goto err;
+ }
/* Get a list of images that we intend to sign */
prop = fit_config_get_image_list(fit, sig_offset, &len,
@@ -923,13 +927,14 @@ static int fit_config_get_hash_list(const void
*fit, int conf_noffset,
if (allow_missing)
continue;
- return -ENOENT;
+ ret = -ENOENT;
+ goto err;
}
ret = fit_config_add_hash(fit, image_noffset, node_inc,
conf_name, sig_name, iname);
if (ret < 0)
- return ret;
+ goto err;
image_count++;
}
@@ -938,15 +943,15 @@ static int fit_config_get_hash_list(const void
*fit, int conf_noffset,
if (!image_count) {
fprintf(stderr, "Failed to find any images for configuration
'%s/%s'\n",
conf_name, sig_name);
- return -ENOMSG;
+ ret = -ENOMSG;
+ goto err;
}
return 0;
-err_mem:
- fprintf(stderr, "Out of memory processing configuration '%s/%s'\n",
conf_name,
- sig_name);
- return -ENOMEM;
+err:
+ strlist_free(node_inc);
+ return ret;
}
/**
"""
What do you think?
Although.... I'm wondering if this isn't just backwards. The caller is
already responsible for providing the strlist pointer AND freeing it, so
maybe the answer is simply for the caller to *also* init the strlist,
something like:
"""
diff --git a/tools/image-host.c b/tools/image-host.c
index 54df86316ae..fd56e603114 100644
--- a/tools/image-host.c
+++ b/tools/image-host.c
@@ -864,7 +864,8 @@ err_path:
* fit_config_get_hash_list() - Get the regions to sign
*
* This calculates a list of nodes to hash for this particular
configuration,
- * returning it as a string list (struct strlist, not a devicetree
string list)
+ * returning it as a string list (struct strlist, not a devicetree
string list).
+ * The struct strlist must already be initialized.
*
* @fit: Pointer to the FIT format image header
* @conf_noffset: Offset of configuration node to sign (child of
@@ -891,7 +892,6 @@ static int fit_config_get_hash_list(const void *fit,
int conf_noffset,
* Build a list of nodes we need to hash. We always need the root
* node and the configuration.
*/
- strlist_init(node_inc);
snprintf(name, sizeof(name), "%s/%s", FIT_CONFS_PATH, conf_name);
if (strlist_add(node_inc, "/") ||
strlist_add(node_inc, name))
@@ -995,11 +995,13 @@ static int fit_config_get_regions(const void *fit,
int conf_noffset,
sig_name = fit_get_name(fit, sig_offset, NULL);
debug("%s: conf='%s', sig='%s'\n", __func__, conf_name, sig_name);
+ strlist_init(&node_inc);
+
/* Get a list of nodes we want to hash */
ret = fit_config_get_hash_list(fit, conf_noffset, sig_offset,
&node_inc);
if (ret)
- return ret;
+ goto out;
/* Get a list of regions to hash */
count = fdt_find_regions(fit, node_inc.strings, node_inc.count,
@@ -1009,12 +1011,14 @@ static int fit_config_get_regions(const void
*fit, int conf_noffset,
if (count < 0) {
fprintf(stderr, "Failed to hash configuration '%s/%s': %s\n",
conf_name,
sig_name, fdt_strerror(ret));
- return -EIO;
+ ret = -EIO;
+ goto out;
}
if (count == 0) {
fprintf(stderr, "No data to hash for configuration '%s/%s':
%s\n",
conf_name, sig_name, fdt_strerror(ret));
- return -EINVAL;
+ ret = -EINVAL;
+ goto out;
}
/* Build our list of data blocks */
@@ -1022,7 +1026,8 @@ static int fit_config_get_regions(const void *fit,
int conf_noffset,
if (!region) {
fprintf(stderr, "Out of memory hashing configuration '%s/%s'\n",
conf_name, sig_name);
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto out;
}
/* Create a list of all hashed properties */
@@ -1035,19 +1040,21 @@ static int fit_config_get_regions(const void
*fit, int conf_noffset,
if (!region_prop) {
fprintf(stderr, "Out of memory setting up regions for configuration
'%s/%s'\n",
conf_name, sig_name);
- return -ENOMEM;
+ ret = -ENOMEM;
+ goto out;
}
for (i = len = 0; i < node_inc.count;
len += strlen(node_inc.strings[i]) + 1, i++)
strcpy(region_prop + len, node_inc.strings[i]);
- strlist_free(&node_inc);
*region_countp = count;
*regionp = region;
*region_propp = region_prop;
*region_proplen = len;
- return 0;
+out:
+ strlist_free(&node_inc);
+ return ret;
}
/**
"""
instead?
Cheers,
Quentin
