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

On 6/29/23 11:52 PM, Simon J. Gerraty 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);

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?

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

> -	if (!cn.status)
> +	if (!cn.status) {
>   		buf = NULL;
> +		if (err == 0)		/* keep compiler happy */
> +			buf = NULL;
> +	}
>   	return (buf);
>   }
>   

-- 
John Baldwin