Date: Fri, 30 Jun 2023 18:07:58 +0100 From: Jessica Clarke <jrtc27@freebsd.org> To: Mitchell Horne <mhorne@freebsd.org> Cc: "src-committers@freebsd.org" <src-committers@FreeBSD.org>, "dev-commits-src-all@freebsd.org" <dev-commits-src-all@FreeBSD.org>, "dev-commits-src-main@freebsd.org" <dev-commits-src-main@FreeBSD.org> Subject: Re: git: 56f3f2d2491e - main - libsecureboot: avoid set but not used errors Message-ID: <B59305E1-680B-4793-9779-B872E76B6A34@freebsd.org> In-Reply-To: <498f3ba2-dc7a-e7d3-626a-76ca68cee5b2@freebsd.org> References: <202306300652.35U6qpgP027126@gitrepo.freebsd.org> <667C347E-B7C7-405B-AFEC-F0A0FD0656F6@freebsd.org> <498f3ba2-dc7a-e7d3-626a-76ca68cee5b2@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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: >>>=20 >>> The branch main has been updated by sjg: >>>=20 >>> URL: = https://cgit.FreeBSD.org/src/commit/?id=3D56f3f2d2491e30f369f9461c3cb2a366= bdffbe1d >>>=20 >>> 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 >>>=20 >>> libsecureboot: avoid set but not used errors >>>=20 >>> Reviewed by: stevek >>> --- >>> lib/libsecureboot/openpgp/opgp_sig.c | 22 ++++++++++++---------- >>> lib/libsecureboot/vets.c | 7 +++++-- >>> 2 files changed, 17 insertions(+), 12 deletions(-) >>>=20 >>> 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 =3D NULL; >>> if ((sdata =3D read_file(sigfile, &sbytes))) { >>> n =3D strlcpy(pbuf, sigfile, sizeof(pbuf)); >>> - if ((cp =3D strrchr(pbuf, '.'))) >>> - *cp =3D '\0'; >>> - if ((fdata =3D read_file(pbuf, &fbytes))) { >>> - if (openpgp_verify(pbuf, fdata, fbytes, sdata, >>> - sbytes, flags)) { >>> - free(fdata); >>> - fdata =3D NULL; >>> + if (n < sizeof(pbuf)) { >>> + if ((cp =3D strrchr(pbuf, '.'))) >>> + *cp =3D '\0'; >>> + if ((fdata =3D read_file(pbuf, &fbytes))) { >>> + if (openpgp_verify(pbuf, fdata, fbytes, sdata, >>> + sbytes, flags)) { >>> + free(fdata); >>> + fdata =3D NULL; >>> + } >>> } >>> } >>> - } else >>> - fdata =3D 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 =3D mc.vtable->end_chain(&mc.vtable); >>>=20 >>> - if (!cn.status) >>> + if (!cn.status) { >>> buf =3D NULL; >>> + if (err =3D=3D 0) /* keep compiler happy */ >>> + buf =3D NULL; >> This is nonsense code. >> Jess >=20 > And yours is a needlessly abrasive and unhelpful reply. >=20 > 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=E2=80=99re right that I can be, and was, overly blunt, for which I = apologise, but I don=E2=80=99t 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=E2=80=99t for both GCC and Clang, perhaps = because the control dependency has ordering implications for atomics. = Since it appears you=E2=80=99re 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 >=20 >>> + } >>> return (buf); >>> }
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?B59305E1-680B-4793-9779-B872E76B6A34>