Re: git: 56f3f2d2491e - main - libsecureboot: avoid set but not used errors

John Baldwin <jhb@FreeBSD.org> wrote:
> > ---
> >   lib/libsecureboot/openpgp/opgp_sig.c | 22 ++++++++++++----------
> >   lib/libsecureboot/vets.c             |  7 +++++--
> >   2 files changed, 17 insertions(+), 12 deletions(-)
> >
> > diff --git a/lib/libsecureboot/openpgp/opgp_sig.c b/lib/libsecureboot/openpgp/opgp_sig.c
> > index eec3469e3457..7f4e6fb98fd1 100644
> > --- a/lib/libsecureboot/openpgp/opgp_sig.c
> > +++ b/lib/libsecureboot/openpgp/opgp_sig.c
> > @@ -464,20 +464,22 @@ verify_asc(const char *sigfile, int flags)
> >       size_t n;
> >       unsigned char *fdata, *sdata;
> >       size_t fbytes, sbytes;
> > -
> > +
> > +     fdata = NULL;
> >       if ((sdata = read_file(sigfile, &sbytes))) {
> >               n = strlcpy(pbuf, sigfile, sizeof(pbuf));
> > -             if ((cp = strrchr(pbuf, '.')))
> > -                     *cp = '\0';
> > -             if ((fdata = read_file(pbuf, &fbytes))) {
> > -                     if (openpgp_verify(pbuf, fdata, fbytes, sdata,
> > -                             sbytes, flags)) {
> > -                             free(fdata);
> > -                             fdata = NULL;
> > +             if (n < sizeof(pbuf)) {
> > +                     if ((cp = strrchr(pbuf, '.')))
> > +                             *cp = '\0';
> > +                     if ((fdata = read_file(pbuf, &fbytes))) {
> > +                             if (openpgp_verify(pbuf, fdata, fbytes, sdata,
> > +                                     sbytes, flags)) {
> > +                                     free(fdata);
> > +                                     fdata = NULL;
> > +                             }
> >                       }
> >               }
> > -     } else
> > -             fdata = NULL;
> > +     }
> >       free(sdata);
> >       return (fdata);
> 
> Most of this change seems to be avoiding reading the "real" file
> if the filename from the signature file was too long to fit into
> pbuf which I think is a different change?

This is all just levels of paranoia.
strlcpy will truncate the data anyway, but if the buf isn't big enough
to hold the name, someone is playing games and we don't want to play along.

> > diff --git a/lib/libsecureboot/vets.c b/lib/libsecureboot/vets.c
> > index 4375dfa76a89..12191097ff8c 100644
> > --- a/lib/libsecureboot/vets.c
> > +++ b/lib/libsecureboot/vets.c
> > @@ -241,11 +241,14 @@ x509_cn_get(br_x509_certificate *xc, char *buf, size_t len)
> >       mc.vtable->start_cert(&mc.vtable, xc->data_len);
> >       mc.vtable->append(&mc.vtable, xc->data, xc->data_len);
> >       mc.vtable->end_cert(&mc.vtable);
> > -     /* we don' actually care about cert status - just its name */
> > +     /* we don't actually care about cert status - just its name */
> >       err = mc.vtable->end_chain(&mc.vtable);
> 
> For cases like this I've removed the variable and used a (void) cast instead to indicate
> that the return value is intentionally unused.

Right, but I actually want err, so it can be seen in a debugger easily.
It was at least useful when first getting this stuff to work.