[U-Boot] [PATCH 2/2] libavb: Fix build warnings after updating the lib

[David Zeuthen]

Hi,

As for upstreaming libavb patches, I'd be interested in landing them
upstream... makes it easier for anyone.

Our upstream is AOSP and we use gerrit for code-review:
https://android-review.googlesource.com/q/project:platform%252Fexternal%252Favb

Here's a guide to contributing:
https://source.android.com/setup/contribute/submit-patches ... hope it's
not too painful to use the AOSP process!

Thanks,
David



On Fri, Aug 16, 2019 at 9:35 AM Eugeniu Rosca <erosca at de.adit-jv.com> wrote:

> Hi Sam,
>
> CC: LIBAVB people (w.r.t. libavb fixes in U-Boot)
>
> I can reproduce the compiler warnings myself and I confirm they are
> fixed with this patch. More comments below.
>
> On Thu, Aug 15, 2019 at 11:04:03PM +0300, Sam Protsenko wrote:
> > After updating libavb to most recent version from AOSP/master, two new
> > warnings appear:
> >
> > Warning #1:
> >
> >     lib/libavb/avb_cmdline.c: In function 'avb_append_options':
> >     lib/libavb/avb_cmdline.c:365:15: warning: 'dm_verity_mode' may be
> >                                      used uninitialized in this function
> >                                      [-Wmaybe-uninitialized]
> >          new_ret = avb_replace(
> >                    ^~~~~~~~~~~~
> >              slot_data->cmdline, "$(ANDROID_VERITY_MODE)",
> dm_verity_mode);
> >
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >     lib/libavb/avb_cmdline.c:374:8: warning: 'verity_mode' may be used
> >                                     uninitialized in this function
> >                                     [-Wmaybe-uninitialized]
> >        if (!cmdline_append_option(
> >             ^~~~~~~~~~~~~~~~~~~~~~
> >                slot_data, "androidboot.veritymode", verity_mode)) {
> >                ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >
> > Warning #2:
> >
> >     lib/libavb/avb_slot_verify.c: In function 'avb_slot_verify':
> >     lib/libavb/avb_slot_verify.c:1349:23: warning: 'ret' may be used
> >                                           uninitialized in this function
> >                                           [-Wmaybe-uninitialized]
> >        AvbSlotVerifyResult ret;
> >                            ^~~
>
> FWIW, few of Linux commits do word-wrapping of ASCII/console dumps for
> the sake of improved readability and git grepping. Recent checkpatch
> versions don't warn on that.
>
> >
> > Fix those by providing default return values to affected functions.
> >
> > Signed-off-by: Sam Protsenko <semen.protsenko at linaro.org>
> > ---
> >  lib/libavb/avb_cmdline.c     | 3 ++-
> >  lib/libavb/avb_slot_verify.c | 2 +-
> >  2 files changed, 3 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/libavb/avb_cmdline.c b/lib/libavb/avb_cmdline.c
> > index cb5b98e423..684c512bb9 100644
> > --- a/lib/libavb/avb_cmdline.c
> > +++ b/lib/libavb/avb_cmdline.c
> > @@ -357,7 +357,8 @@ AvbSlotVerifyResult avb_append_options(
> >          // Should never get here because MANAGED_RESTART_AND_EIO is
> >          // remapped by avb_manage_hashtree_error_mode().
> >          avb_assert_not_reached();
> > -        break;
> > +        ret = AVB_SLOT_VERIFY_RESULT_ERROR_INVALID_ARGUMENT;
> > +        goto out;
>
> With AVB_ENABLE_DEBUG enabled, PVS-Studio reports:
> lib/libavb/avb_cmdline.c        360     warn    V779 Unreachable code
> detected. It is possible that an error is present.
>
> How about replacing the 'break' statement with a 'fallthrough' comment?
> It shuts down the warning w/o changing the functionality.
>
> >        default:
> >          ret = AVB_SLOT_VERIFY_RESULT_ERROR_INVALID_ARGUMENT;
> >          goto out;
> > diff --git a/lib/libavb/avb_slot_verify.c b/lib/libavb/avb_slot_verify.c
> > index 5d400b38aa..c0defdf9c9 100644
> > --- a/lib/libavb/avb_slot_verify.c
> > +++ b/lib/libavb/avb_slot_verify.c
> > @@ -1346,7 +1346,7 @@ AvbSlotVerifyResult avb_slot_verify(AvbOps* ops,
> >                                      AvbSlotVerifyFlags flags,
> >                                      AvbHashtreeErrorMode
> hashtree_error_mode,
> >                                      AvbSlotVerifyData** out_data) {
> > -  AvbSlotVerifyResult ret;
> > +  AvbSlotVerifyResult ret =
> AVB_SLOT_VERIFY_RESULT_ERROR_INVALID_ARGUMENT;
>
> What should we do with these out-of-tree libavb fixes? IMHO they are not
> specific to U-Boot and should be upstream-able. IMHO it is not healthy
> to accumulate too many of them, since this will make future libavb sync
> more painful.
>
> --
> Best Regards,
> Eugeniu.
>


-- 

David Zeuthen |  zeuthen at google.com |
 Google
| Android Hardware-Backed Security