30.11.2019 22:42, Markus Armbruster wrote: > fit_load_fdt() recovers from fit_image_addr() failing with ENOENT. > Except it doesn't when its @errp argument is &error_fatal or > &error_abort, because it blindly passes @errp to fit_image_addr(). > Messed up in commit 3eb99edb48 "loader-fit: Wean off error_printf()". > > The bug can't bite as no caller actually passes &error_fatal or > &error_abort. Fix it anyway. > > Signed-off-by: Markus Armbruster <arm...@redhat.com>
Reviewed-by: Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> Hmm, actually it makes my "[PATCH v7 01/21] hw/core/loader-fit: fix freeing errp in fit_load_fdt" unnecessary. If you want you may drop my 01 (as it covers less problems), and in this case you may note in your cover letter, that errp = NULL is broken here too (may be nobady pass it?), and normal errp is handled wrong, as *errp doesn't set to NULL after error_free(*errp) (still, the only caller rely on return value more than on err, so the problem can't be triggered with current code) > --- > hw/core/loader-fit.c | 15 ++++++++------- > 1 file changed, 8 insertions(+), 7 deletions(-) > > diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c > index 953b16bc82..c465921b8f 100644 > --- a/hw/core/loader-fit.c > +++ b/hw/core/loader-fit.c > @@ -178,11 +178,12 @@ static int fit_load_fdt(const struct fit_loader *ldr, > const void *itb, > int cfg, void *opaque, const void *match_data, > hwaddr kernel_end, Error **errp) > { > + Error *err = NULL; > const char *name; > const void *data; > const void *load_data; > hwaddr load_addr; > - int img_off, err; > + int img_off; > size_t sz; > int ret; > > @@ -197,13 +198,13 @@ static int fit_load_fdt(const struct fit_loader *ldr, > const void *itb, > return -EINVAL; > } > > - err = fit_image_addr(itb, img_off, "load", &load_addr, errp); > - if (err == -ENOENT) { > + ret = fit_image_addr(itb, img_off, "load", &load_addr, &err); > + if (ret == -ENOENT) { > load_addr = ROUND_UP(kernel_end, 64 * KiB) + (10 * MiB); > - error_free(*errp); > - } else if (err) { > - error_prepend(errp, "unable to read FDT load address from FIT: "); > - ret = err; > + error_free(err); > + } else if (ret) { > + error_propagate_prepend(errp, err, > + "unable to read FDT load address from FIT: > "); > goto out; > } > > So much attention to that function :) I'd also propose the following: diff --git a/hw/core/loader-fit.c b/hw/core/loader-fit.c index c465921b8f..2c9efeef7a 100644 --- a/hw/core/loader-fit.c +++ b/hw/core/loader-fit.c @@ -180,8 +180,8 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb, { Error *err = NULL; const char *name; - const void *data; - const void *load_data; + void *data; + void *load_data; hwaddr load_addr; int img_off; size_t sz; @@ -218,9 +218,9 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb, ret = 0; out: - g_free((void *) data); + g_free(data); if (data != load_data) { - g_free((void *) load_data); + g_free(load_data); } return ret; } Or, even better: --- a/hw/core/loader-fit.c +++ b/hw/core/loader-fit.c @@ -180,7 +180,8 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb, { Error *err = NULL; const char *name; - const void *data; + g_autofree void *data = NULL; + g_autofree void *fdt_filter_data = NULL; const void *load_data; hwaddr load_addr; int img_off; @@ -202,27 +203,23 @@ static int fit_load_fdt(const struct fit_loader *ldr, const void *itb, if (ret == -ENOENT) { load_addr = ROUND_UP(kernel_end, 64 * KiB) + (10 * MiB); error_free(err); + return 0; } else if (ret) { error_propagate_prepend(errp, err, "unable to read FDT load address from FIT: "); - goto out; + return ret; } if (ldr->fdt_filter) { - load_data = ldr->fdt_filter(opaque, data, match_data, &load_addr); + load_data = fdt_filter_data = + ldr->fdt_filter(opaque, data, match_data, &load_addr); } load_addr = ldr->addr_to_phys(opaque, load_addr); sz = fdt_totalsize(load_data); rom_add_blob_fixed(name, load_data, sz, load_addr); - ret = 0; -out: - g_free((void *) data); - if (data != load_data) { - g_free((void *) load_data); - } - return ret; + return 0; } -- Best regards, Vladimir