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

On 30 Jun 2023, at 17:43, Mitchell Horne <mhorne@freebsd.org> wrote:
> 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.

You’re right that I can be, and was, overly blunt, for which I apologise, but I don’t think this kind of pointed attack on my character on-list is an appropriate response. To reword and expand my original email:

A conditional assignment of the same value that has already been assigned unconditionally is unidiomatic code generally regarded as a code smell and should be avoided. In an ideal world that would give a compiler warning, but it doesn’t for both GCC and Clang, perhaps because the control dependency has ordering implications for atomics. Since it appears you’re doing this just to insert a dummy read of err, either err should be marked __unused or it should be removed entirely and the return value of end_chain explicitly marked as unused by casting it to void.

Jess

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