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

On 6/30/23 11:42, Jessica Clarke wrote:
> On 30 Jun 2023, at 07:52, Simon J. Gerraty <sjg@FreeBSD.org> wrote:
>>
>> The branch main has been updated by sjg:
>>
>> URL: https://cgit.FreeBSD.org/src/commit/?id=56f3f2d2491e30f369f9461c3cb2a366bdffbe1d
>>
>> commit 56f3f2d2491e30f369f9461c3cb2a366bdffbe1d
>> Author:     Simon J. Gerraty <sjg@FreeBSD.org>
>> AuthorDate: 2023-06-30 06:52:17 +0000
>> Commit:     Simon J. Gerraty <sjg@FreeBSD.org>
>> CommitDate: 2023-06-30 06:52:17 +0000
>>
>>     libsecureboot: avoid set but not used errors
>>
>>     Reviewed by:    stevek
>> ---
>> 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);
>> }
>> 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);
>>
>> - if (!cn.status)
>> + if (!cn.status) {
>> buf = NULL;
>> + if (err == 0) /* keep compiler happy */
>> + buf = NULL;
> 
> This is nonsense code.
> 
> Jess

And yours is a needlessly abrasive and unhelpful reply.

Seriously Jess, if your choice is to read through and nit-pick peoples' 
changes, then at least learn to deliver your feedback with an ounce of tact.

Mitchell

> 
>> + }
>> return (buf);
>> }
>>
>