[PATCH] cmd: avb: free partition buffer upon verify completion

[Gary Bisson]

Hi Igor,

On Sun, May 10, 2020 at 01:30:53PM +0300, Igor Opaniuk wrote:
> Hi Gary,
> 
> Thanks for the patch and sorry for a late reply.

No problem.

> On Wed, May 6, 2020 at 5:06 PM Gary Bisson
> <gary.bisson at boundarydevices.com> wrote:
> >
> > Allows to run 'avb verify' multiple times which can be useful after a
> > failure to be able to re-flash the partition and try again.
> >
> > Signed-off-by: Gary Bisson <gary.bisson at boundarydevices.com>
> > ---
> > Hi,
> >
> > This was added because of the following scenario:
> > 1- fastboot flash boot boot.img
> > 2- avb verify
> >   -> fails because vbmeta wasn't updated
> > 3- fastboot flash vbmeta vbmeta.img
> > 4- avb verify
> >   -> fails because it can't allocate memory as previous buffer wasn't
> > freed
> >
> > Let me know if you have any questions.
> >
> > Regards,
> > Gary
> > ---
> >  cmd/avb.c | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/cmd/avb.c b/cmd/avb.c
> > index a4de5c40a2..67154de4e1 100644
> > --- a/cmd/avb.c
> > +++ b/cmd/avb.c
> > @@ -312,6 +312,14 @@ int do_avb_verify_part(cmd_tbl_t *cmdtp, int flag,
> >                 printf("Unknown error occurred\n");
> >         }
> >
> > +       /* Free image buffers now that verification is complete */
> > +       if (slot_result != AVB_SLOT_VERIFY_RESULT_ERROR_OOM) {
> Freeing out_data can be done not only when res is
> AVB_SLOT_VERIFY_RESULT_ERROR_OOM.
> For example, in the line 1196 in [1]:
> 
> AvbSlotVerifyResult sub_ret = avb_append_options(ops,
>                                                  slot_data,
>                                                  &toplevel_vbmeta,
>                                                  algorithm_type,
>                                                  hashtree_error_mode);
> if (sub_ret != AVB_SLOT_VERIFY_RESULT_OK) {
>         ret = sub_ret;
>         goto fail;
> }

Indeed.

> > +               int i;
> > +               for (i = 0; i < (int)out_data->num_loaded_partitions; i++)
> > +                       if (out_data->loaded_partitions[i].data)
> > +                               free(out_data->loaded_partitions[i].data);
> 
> It's better to use avb_slot_verify_data_free()  for the whole out_data instead
> for proper cleanup. Check how it's done in unittests for libavb [2].

I'll check it out and offer a v2. Thanks for your feedback.

Regards,
Gary