| Message ID | 20180609182235.33532-5-sjg@chromium.org |
|---|---|
| State | Superseded |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Fix some coverity warnings | expand |
On 06/09/2018 08:22 PM, Simon Glass wrote: > This loop never actually exits, but the way the code is written this is > not obvious. Add an explicit error check. > > Reported-by: Coverity (CID: 131280) > > Signed-off-by: Simon Glass <sjg@chromium.org> > --- > > tools/fdtgrep.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) > > diff --git a/tools/fdtgrep.c b/tools/fdtgrep.c > index f2b8b71ed7..c4563e2289 100644 > --- a/tools/fdtgrep.c > +++ b/tools/fdtgrep.c > @@ -801,7 +801,7 @@ static int do_fdtgrep(struct display_info *disp, const char *filename) > * The first pass will count the regions, but if it is too many, > * we do another pass to actually record them. > */ > - for (i = 0; i < 3; i++) { > + for (i = 0; i < 2; i++) { > region = malloc(count * sizeof(struct fdt_region)); > if (!region) { > fprintf(stderr, "Out of memory for %d regions\n", Can't we call fdtgrep_find_regions() with max_regions = 0 and region = NULL to do the counting and get rid of the loop? That may be a bit slower but the code will be much easier to read. > @@ -820,6 +820,8 @@ static int do_fdtgrep(struct display_info *disp, const char *filename) Have a look at the lines in between: if (count < 0) { report_error("fdt_find_regions", count); return -1; } Here a free(region) is missing. Best regards Heinrich > if (count <= max_regions) > break; > free(region); > + fprintf(stderr, "Internal error with fdtgrep_find_region)(\n"); > + return -1; > } > > /* Optionally print a list of regions */ >
Hi Heinrich, On 9 June 2018 at 13:44, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote: > On 06/09/2018 08:22 PM, Simon Glass wrote: >> This loop never actually exits, but the way the code is written this is >> not obvious. Add an explicit error check. >> >> Reported-by: Coverity (CID: 131280) >> >> Signed-off-by: Simon Glass <sjg@chromium.org> >> --- >> >> tools/fdtgrep.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) >> >> diff --git a/tools/fdtgrep.c b/tools/fdtgrep.c >> index f2b8b71ed7..c4563e2289 100644 >> --- a/tools/fdtgrep.c >> +++ b/tools/fdtgrep.c >> @@ -801,7 +801,7 @@ static int do_fdtgrep(struct display_info *disp, const char *filename) >> * The first pass will count the regions, but if it is too many, >> * we do another pass to actually record them. >> */ >> - for (i = 0; i < 3; i++) { >> + for (i = 0; i < 2; i++) { >> region = malloc(count * sizeof(struct fdt_region)); >> if (!region) { >> fprintf(stderr, "Out of memory for %d regions\n", > > Can't we call fdtgrep_find_regions() with max_regions = 0 and region = > NULL to do the counting and get rid of the loop? That may be a bit > slower but the code will be much easier to read. Yes I think that would be possible. It would require a change to fdtgrep_first_region since it assumes there is space for its region. It seems like a good idea to me, but is beyond the scope of this coverity fix. Regards, Simon
diff --git a/tools/fdtgrep.c b/tools/fdtgrep.c index f2b8b71ed7..c4563e2289 100644 --- a/tools/fdtgrep.c +++ b/tools/fdtgrep.c @@ -801,7 +801,7 @@ static int do_fdtgrep(struct display_info *disp, const char *filename) * The first pass will count the regions, but if it is too many, * we do another pass to actually record them. */ - for (i = 0; i < 3; i++) { + for (i = 0; i < 2; i++) { region = malloc(count * sizeof(struct fdt_region)); if (!region) { fprintf(stderr, "Out of memory for %d regions\n", @@ -820,6 +820,8 @@ static int do_fdtgrep(struct display_info *disp, const char *filename) if (count <= max_regions) break; free(region); + fprintf(stderr, "Internal error with fdtgrep_find_region)(\n"); + return -1; } /* Optionally print a list of regions */
This loop never actually exits, but the way the code is written this is not obvious. Add an explicit error check. Reported-by: Coverity (CID: 131280) Signed-off-by: Simon Glass <sjg@chromium.org> --- tools/fdtgrep.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)