| Message ID | 20211123080633.8318-1-hansson.mattias@gmail.com |
|---|---|
| State | Superseded |
| Delegated to: | Stefano Babic |
| Headers | show |
| Series | tools/mxsimage: Remove fclose on empty FILE pointer | expand |
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.
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
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.
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;