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

On 6/30/23 2:04 PM, Simon J. Gerraty wrote:
> 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.

Oh that's fine, just didn't seem related to the warning described in the
commit log is all. :)

>>> 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.

I'm actually surprised you could see err in the debugger at all in that case
since the compiler probably elides it (and associated debug info) entirely.
However, one thing you can do is to step inside the function and then use
'finish' which will print the return value after completing the function.
If you've just stepped over the function you can also check the relevant register
to see the return value (e.g. eax/rax on x86).  I think the most recent GDB
even has a patch now (possibly controlled by an off by default knob) to print
the return value from functions when you are stepping similar to the behavior
you get with 'finish'.

-- 
John Baldwin