(adding CryptoPkg maintainers from Maintainers.txt, plus Steven for clang) On 10/20/17 17:10, Peter Jones wrote: > clang-analyzer noticed this: > > Pk/CryptPkcs7Verify.c:600:5: warning: Value stored to 'OldSize' is never read > OldSize = BufferSize; > ^ ~~~~~~~~~~ > Pk/CryptPkcs7Verify.c:644:5: warning: Value stored to 'OldSize' is never read > OldSize = BufferSize; > ^ ~~~~~~~~~~ > 2 warnings generated.
Interesting; Steven runs clang (incl. clang-analyzer AFAIK) frequently, and I don't recall an earlier report of this. Anyway, > > These are each immediately followed by a loop that initializes them (to > the same values) a second time, and are otherwise only referenced inside > that loop, so there's just no point to these assignments at all. I agree. While each of both loops might fail to reach the inner assignment to OldSize -- in case the first X509PopCertificate() call fails in the loop, for Index=0 --, OldSize is still never read after each loop (before another assignment to it is reached). So setting OldSize to anything at all before the loops is superfluous. > > Contributed-under: TianoCore Contribution Agreement 1.1 > Signed-off-by: Peter Jones <[email protected]> > --- > CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Verify.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Verify.c > b/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Verify.c > index d564591cb7f9..bf67a1f569a2 100644 > --- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Verify.c > +++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Verify.c > @@ -612,7 +612,6 @@ Pkcs7GetCertificatesList ( > > if (CtxChain != NULL) { > BufferSize = sizeof (UINT8); > - OldSize = BufferSize; > CertBuf = NULL; > > for (Index = 0; ; Index++) { > @@ -656,7 +655,6 @@ Pkcs7GetCertificatesList ( > > if (CtxUntrusted != NULL) { > BufferSize = sizeof (UINT8); > - OldSize = BufferSize; > CertBuf = NULL; > > for (Index = 0; ; Index++) { > For your patch: Reviewed-by: Laszlo Ersek <[email protected]> Assuming the maintainers are fine with the patch as well, I suggest that they please replace the word "initializations" with "assignments" in the subject, to be pedantic on the C-lang level. (Side note: I would even move OldSize to a lot tighter scope: > diff --git a/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Verify.c > b/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Verify.c > index d564591cb7f9..31a9ecd59ff6 100644 > --- a/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Verify.c > +++ b/CryptoPkg/Library/BaseCryptLib/Pk/CryptPkcs7Verify.c > @@ -477,7 +477,6 @@ Pkcs7GetCertificatesList ( > UINT8 *CertBuf; > UINT8 *OldBuf; > UINTN BufferSize; > - UINTN OldSize; > UINT8 *SingleCert; > UINTN CertSize; > > @@ -612,10 +611,11 @@ Pkcs7GetCertificatesList ( > > if (CtxChain != NULL) { > BufferSize = sizeof (UINT8); > - OldSize = BufferSize; > CertBuf = NULL; > > for (Index = 0; ; Index++) { > + UINTN OldSize; > + > Status = X509PopCertificate (CtxChain, &SingleCert, &CertSize); > if (!Status) { > break; > @@ -656,10 +656,11 @@ Pkcs7GetCertificatesList ( > > if (CtxUntrusted != NULL) { > BufferSize = sizeof (UINT8); > - OldSize = BufferSize; > CertBuf = NULL; > > for (Index = 0; ; Index++) { > + UINTN OldSize; > + > Status = X509PopCertificate (CtxUntrusted, &SingleCert, &CertSize); > if (!Status) { > break; However, many edk2 maintainers don't like tight scoping like this.) Thanks Laszlo _______________________________________________ edk2-devel mailing list [email protected] https://lists.01.org/mailman/listinfo/edk2-devel
