[PATCH] tools: imagetool: Remove unnecessary check from toc0_verify_cert_item()

[Seung-Woo Kim]

Hi,

> -----Original Message-----
> From: Andre Przywara <andre.przywara at arm.com>
> Sent: Thursday, August 1, 2024 6:28 PM
> Subject: Re: [PATCH] tools: imagetool: Remove unnecessary check from
> toc0_verify_cert_item()
> 
> On Thu,  1 Aug 2024 10:01:00 +0900
> Seung-Woo Kim <sw0312.kim at samsung.com> wrote:
> 
> Hi,
> 
> > Remove unnecessary null check from toc0_verify_cert_item() because the
> > array digest is always not null.
> 
> Do you mean it's always not NULL *as currently used*? Because digest is a
> function parameter, so within the scope of this function definition can be
> NULL.
> I agree that *currently* there is only one caller, and this passes the
> addresses of a local variable from the stack, though it's never NULL.
> 
> But I don't think this check hurts (looks like the compiler removes it
> anyway), and it makes the code more robust. So is there any problem with
> this check? Why do you want it to be removed?

Because in my gcc-14 environment, it gives -Wnonnull-compare warning.

tools/sunxi_toc0.c: In function 'toc0_verify_cert_item':
tools/sunxi_toc0.c:447:12: warning: 'nonnull' argument 'digest' compared to
NULL [-Wnonnull-compare]
  447 |         if (digest && memcmp(&extension->digest, digest,
SHA256_DIGEST_LENGTH)) {
      |            ^

Regards,
- Seung-Woo Kim

> 
> Cheers,
> Andre
> 
> >
> > Signed-off-by: Seung-Woo Kim <sw0312.kim at samsung.com>
> > ---
> >  tools/sunxi_toc0.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tools/sunxi_toc0.c b/tools/sunxi_toc0.c index
> > 292649fe90f1..76693647a095 100644
> > --- a/tools/sunxi_toc0.c
> > +++ b/tools/sunxi_toc0.c
> > @@ -444,7 +444,7 @@ static int toc0_verify_cert_item(const uint8_t
> > *buf, uint32_t len, RSA *fw_key,
> >
> >  	/* If a digest was provided, compare it to the embedded digest. */
> >  	extension = &totalSequence->mainSequence.explicit3.extension;
> > -	if (digest && memcmp(&extension->digest, digest,
> SHA256_DIGEST_LENGTH)) {
> > +	if (memcmp(&extension->digest, digest, SHA256_DIGEST_LENGTH)) {
> >  		pr_err("Wrong firmware digest in certificate\n");
> >  		goto err;
> >  	}