Vladimir Sementsov-Ogievskiy <vsement...@virtuozzo.com> writes: > 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),
Yes. > and in this case you may note in your cover letter, that > errp = NULL is broken here too (may be nobady pass it?), You're right, null @errp is wrong, too. > and normal errp is handled wrong, as *errp doesn't set to NULL after > error_free(*errp) Yes, that's wrong, too. fit_load_fdt() itself doesn't use *errp after freeing it, but it sets a trap for its callers. > (still, the only caller rely on return value more than on > err, so the problem can't be triggered with current code) True. New commit message (based on v2's): hw/core: Fix fit_load_fdt() error API violations fit_load_fdt() passes @errp to fit_image_addr(), then recovers from ENOENT failures. Passing @errp is wrong, because it works only as long as @errp is neither @error_fatal nor @error_abort. Error recovery dereferences @errp. That's also wrong; see the big comment in error.h. Error recovery can leave *errp pointing to a freed Error object. Wrong, it must be null on success. Messed up in commit 3eb99edb48 "loader-fit: Wean off error_printf()". No caller actually passes such values, or uses *errp on success. Fix anyway: splice in a local Error *err, and error_propagate(). Signed-off-by: Markus Armbruster <arm...@redhat.com> Okay? > >> --- >> 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; > } Looks like a sensible separate cleanup patch to me. Care to post it? Thanks!