[PATCH] tools/mxsimage: Remove fclose on empty FILE pointer

[PATCH] tools/mxsimage: Remove fclose on empty FILE pointer

[Mattias Hansson]

If `sb_load_cmdfile()` fails to open the configuration file it will jump
to error handling where the code will try to `fclose()` the FILE pointer
which is NULL causing `mkimage` to segfault.

This patch removes the `fclose()` since `fopen()` always returns NULL
instead of the file descriptor when failing.
---
 tools/mxsimage.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tools/mxsimage.c b/tools/mxsimage.c
index 002f4b525a..c7bd86ce52 100644
--- a/tools/mxsimage.c
+++ b/tools/mxsimage.c
@@ -1618,7 +1618,6 @@ static int sb_load_cmdfile(struct sb_image_ctx *ictx)
 	return 0;
 
 err_file:
-	fclose(fp);
 	fprintf(stderr, "ERR: Failed to load file \"%s\"\n",
 		ictx->cfg_filename);
 	return -EINVAL;
-- 
2.27.0

Re: [PATCH] tools/mxsimage: Remove fclose on empty FILE pointer

[Fabio Estevam]

Hi Mattias,

On Tue, Nov 23, 2021 at 4:10 PM Mattias Hansson
<hansson.mattias@gmail.com> wrote:
>
> If `sb_load_cmdfile()` fails to open the configuration file it will jump
> to error handling where the code will try to `fclose()` the FILE pointer
> which is NULL causing `mkimage` to segfault.
>
> This patch removes the `fclose()` since `fopen()` always returns NULL
> instead of the file descriptor when failing.

You missed your Signed-off-line tag.

Re: [PATCH] tools/mxsimage: Remove fclose on empty FILE pointer

[Wolfgang Denk]

Dear Mattias Hansson,

In message <20211123080633.8318-1-hansson.mattias@gmail.com> you wrote:
> If `sb_load_cmdfile()` fails to open the configuration file it will jump
> to error handling where the code will try to `fclose()` the FILE pointer
> which is NULL causing `mkimage` to segfault.
>
> This patch removes the `fclose()` since `fopen()` always returns NULL
> instead of the file descriptor when failing.
> ---
>  tools/mxsimage.c | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/tools/mxsimage.c b/tools/mxsimage.c
> index 002f4b525a..c7bd86ce52 100644
> --- a/tools/mxsimage.c
> +++ b/tools/mxsimage.c
> @@ -1618,7 +1618,6 @@ static int sb_load_cmdfile(struct sb_image_ctx *ictx)
>  	return 0;
>  
>  err_file:
> -	fclose(fp);
>  	fprintf(stderr, "ERR: Failed to load file \"%s\"\n",
>  		ictx->cfg_filename);
>  	return -EINVAL;

The whole code with the goto is ugly.  Such an error exit may make
sense when used in several places, but here it just makes reading
the code more difficult.  I suggest to fix this like this:


diff --git a/tools/mxsimage.c b/tools/mxsimage.c
index 002f4b525a..801aaa62ce 100644
--- a/tools/mxsimage.c
+++ b/tools/mxsimage.c
@@ -1595,8 +1595,11 @@ static int sb_load_cmdfile(struct sb_image_ctx *ictx)
 	size_t len;
 
 	fp = fopen(ictx->cfg_filename, "r");
-	if (!fp)
-		goto err_file;
+	if (!fp) {
+		fprintf(stderr, "ERR: Failed to load file \"%s\"\n",
+			ictx->cfg_filename);
+		return -EINVAL;
+	}
 
 	while ((rlen = getline(&line, &len, fp)) > 0) {
 		memset(&cmd, 0, sizeof(cmd));
@@ -1616,12 +1619,6 @@ static int sb_load_cmdfile(struct sb_image_ctx *ictx)
 	fclose(fp);
 
 	return 0;
-
-err_file:
-	fclose(fp);
-	fprintf(stderr, "ERR: Failed to load file \"%s\"\n",
-		ictx->cfg_filename);
-	return -EINVAL;
 }
 
 static int sb_build_tree_from_cfg(struct sb_image_ctx *ictx)



Thanks.


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,      Managing Director: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd@denx.de
Applying computer technology is simply finding the  right  wrench  to
pound in the correct screw.

Re: [PATCH] tools/mxsimage: Remove fclose on empty FILE pointer

[Mattias Hansson]

Thank you for the quick reply. I share your opinion about the goto. I
will work on a new patch. Thank you for the feedback. 

[PATCH] tools/mxsimage: Remove fclose on empty FILE pointer

[Mattias Hansson]

If `sb_load_cmdfile()` fails to open the configuration file it will jump
to error handling where the code will try to `fclose()` the FILE pointer
which is NULL causing `mkimage` to segfault.

This patch removes the `fclose()` since `fopen()` always returns NULL
instead of the file descriptor when failing.

Signed-off-by: Mattias Hansson <hansson.mattias@gmail.com>
---
 tools/mxsimage.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tools/mxsimage.c b/tools/mxsimage.c
index 002f4b525a..c7bd86ce52 100644
--- a/tools/mxsimage.c
+++ b/tools/mxsimage.c
@@ -1618,7 +1618,6 @@ static int sb_load_cmdfile(struct sb_image_ctx *ictx)
 	return 0;
 
 err_file:
-	fclose(fp);
 	fprintf(stderr, "ERR: Failed to load file \"%s\"\n",
 		ictx->cfg_filename);
 	return -EINVAL;
-- 
2.27.0

[PATCH] tools/mxsimage: Remove fclose on empty FILE pointer

[Mattias Hansson]

If `sb_load_cmdfile()` fails to open the configuration file it will jump
to error handling where the code will try to `fclose()` the FILE pointer
which is NULL causing `mkimage` to segfault.

This patch removes the `fclose()` since `fopen()` always returns NULL
instead of the file descriptor when failing.
---
 tools/mxsimage.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/tools/mxsimage.c b/tools/mxsimage.c
index 002f4b525a..c7bd86ce52 100644
--- a/tools/mxsimage.c
+++ b/tools/mxsimage.c
@@ -1618,7 +1618,6 @@ static int sb_load_cmdfile(struct sb_image_ctx *ictx)
 	return 0;
 
 err_file:
-	fclose(fp);
 	fprintf(stderr, "ERR: Failed to load file \"%s\"\n",
 		ictx->cfg_filename);
 	return -EINVAL;
-- 
2.27.0