[PATCH v1 1/1] CryptoPkg/BaseCryptLib: remove some duplicate initializations.

public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed

* [PATCH v1 1/1] CryptoPkg/BaseCryptLib: remove some duplicate initializations.
@ 2017-10-20 15:10 Peter Jones
  2017-10-20 17:12 ` Laszlo Ersek
  0 siblings, 1 reply; 6+ messages in thread
From: Peter Jones @ 2017-10-20 15:10 UTC (permalink / raw)
  To: edk2-devel

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.

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.

Contributed-under: TianoCore Contribution Agreement 1.1
Signed-off-by: Peter Jones <pjones@redhat.com>
---
 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++) {
-- 
2.14.2



^ permalink raw reply related	[flat|nested] 6+ messages in thread

* Re: [PATCH v1 1/1] CryptoPkg/BaseCryptLib: remove some duplicate initializations.
  2017-10-20 15:10 [PATCH v1 1/1] CryptoPkg/BaseCryptLib: remove some duplicate initializations Peter Jones
@ 2017-10-20 17:12 ` Laszlo Ersek
  2017-10-20 18:21   ` Peter Jones
  0 siblings, 1 reply; 6+ messages in thread
From: Laszlo Ersek @ 2017-10-20 17:12 UTC (permalink / raw)
  To: Peter Jones; +Cc: edk2-devel, Shi, Steven, Qin Long, Ting Ye

(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 <pjones@redhat.com>
> ---
>  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 <lersek@redhat.com>

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


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v1 1/1] CryptoPkg/BaseCryptLib: remove some duplicate initializations.
  2017-10-20 17:12 ` Laszlo Ersek
@ 2017-10-20 18:21   ` Peter Jones
  2017-10-23  3:02     ` Long, Qin
  0 siblings, 1 reply; 6+ messages in thread
From: Peter Jones @ 2017-10-20 18:21 UTC (permalink / raw)
  To: Laszlo Ersek; +Cc: edk2-devel, Shi, Steven, Qin Long, Ting Ye

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

Well, that's why I said "initializations" instead of "initializers", but if
it's more clear to you, I'm fine with your way.

> (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.)

I had considered this and guessed it was probably against the coding style or
it would have been done this way already.  IMO it's better in every way.

-- 
  Peter


^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v1 1/1] CryptoPkg/BaseCryptLib: remove some duplicate initializations.
  2017-10-20 18:21   ` Peter Jones
@ 2017-10-23  3:02     ` Long, Qin
  2017-10-24  7:51       ` Laszlo Ersek
  0 siblings, 1 reply; 6+ messages in thread
From: Long, Qin @ 2017-10-23  3:02 UTC (permalink / raw)
  To: Peter Jones, Laszlo Ersek; +Cc: edk2-devel@lists.01.org, Shi, Steven, Ye, Ting

This looks good to me.
Reviewed-by: Long Qin qin.long@intel.com<mailto:qin.long@intel.com>


Best Regards & Thanks,
LONG, Qin

From: Peter Jones [mailto:pjones@redhat.com]
Sent: Saturday, October 21, 2017 2:22 AM
To: Laszlo Ersek <lersek@redhat.com>
Cc: edk2-devel@lists.01.org; Shi, Steven <steven.shi@intel.com>; Long, Qin <qin.long@intel.com>; Ye, Ting <ting.ye@intel.com>
Subject: Re: [edk2] [PATCH v1 1/1] CryptoPkg/BaseCryptLib: remove some duplicate initializations.

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

Well, that's why I said "initializations" instead of "initializers", but if
it's more clear to you, I'm fine with your way.

> (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.)

I had considered this and guessed it was probably against the coding style or
it would have been done this way already.  IMO it's better in every way.

--
  Peter

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v1 1/1] CryptoPkg/BaseCryptLib: remove some duplicate initializations.
  2017-10-23  3:02     ` Long, Qin
@ 2017-10-24  7:51       ` Laszlo Ersek
  2017-10-24  8:15         ` Long, Qin
  0 siblings, 1 reply; 6+ messages in thread
From: Laszlo Ersek @ 2017-10-24  7:51 UTC (permalink / raw)
  To: Long, Qin, Peter Jones; +Cc: edk2-devel@lists.01.org, Shi, Steven, Ye, Ting

Qin,

On 10/23/17 05:02, Long, Qin wrote:
> This looks good to me.
> Reviewed-by: Long Qin qin.long@intel.com<mailto:qin.long@intel.com>

Do you want me to push the patch, or do you prefer to push it yourself?

Thanks!
Laszlo

> From: Peter Jones [mailto:pjones@redhat.com]
> Sent: Saturday, October 21, 2017 2:22 AM
> To: Laszlo Ersek <lersek@redhat.com>
> Cc: edk2-devel@lists.01.org; Shi, Steven <steven.shi@intel.com>; Long, Qin <qin.long@intel.com>; Ye, Ting <ting.ye@intel.com>
> Subject: Re: [edk2] [PATCH v1 1/1] CryptoPkg/BaseCryptLib: remove some duplicate initializations.
> 
>> 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.
> 
> Well, that's why I said "initializations" instead of "initializers", but if
> it's more clear to you, I'm fine with your way.
> 
>> (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.)
> 
> I had considered this and guessed it was probably against the coding style or
> it would have been done this way already.  IMO it's better in every way.
> 
> --
>   Peter
> 



^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH v1 1/1] CryptoPkg/BaseCryptLib: remove some duplicate initializations.
  2017-10-24  7:51       ` Laszlo Ersek
@ 2017-10-24  8:15         ` Long, Qin
  0 siblings, 0 replies; 6+ messages in thread
From: Long, Qin @ 2017-10-24  8:15 UTC (permalink / raw)
  To: Laszlo Ersek, Peter Jones; +Cc: Ye, Ting, edk2-devel@lists.01.org

The patch was already push @b5a985ca9237b551618cd97b1b71af2fff55e209
I forgot to inform that. Thanks, Laszlo.


Best Regards & Thanks,
LONG, Qin


-----Original Message-----
From: edk2-devel [mailto:edk2-devel-bounces@lists.01.org] On Behalf Of Laszlo Ersek
Sent: Tuesday, October 24, 2017 3:51 PM
To: Long, Qin <qin.long@intel.com>; Peter Jones <pjones@redhat.com>
Cc: Ye, Ting <ting.ye@intel.com>; edk2-devel@lists.01.org
Subject: Re: [edk2] [PATCH v1 1/1] CryptoPkg/BaseCryptLib: remove some duplicate initializations.

Qin,

On 10/23/17 05:02, Long, Qin wrote:
> This looks good to me.
> Reviewed-by: Long Qin qin.long@intel.com<mailto:qin.long@intel.com>

Do you want me to push the patch, or do you prefer to push it yourself?

Thanks!
Laszlo

> From: Peter Jones [mailto:pjones@redhat.com]
> Sent: Saturday, October 21, 2017 2:22 AM
> To: Laszlo Ersek <lersek@redhat.com>
> Cc: edk2-devel@lists.01.org; Shi, Steven <steven.shi@intel.com>; Long, 
> Qin <qin.long@intel.com>; Ye, Ting <ting.ye@intel.com>
> Subject: Re: [edk2] [PATCH v1 1/1] CryptoPkg/BaseCryptLib: remove some duplicate initializations.
> 
>> 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.
> 
> Well, that's why I said "initializations" instead of "initializers", 
> but if it's more clear to you, I'm fine with your way.
> 
>> (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.)
> 
> I had considered this and guessed it was probably against the coding 
> style or it would have been done this way already.  IMO it's better in every way.
> 
> --
>   Peter
> 

_______________________________________________
edk2-devel mailing list
edk2-devel@lists.01.org
https://lists.01.org/mailman/listinfo/edk2-devel


^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2017-10-24  8:12 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-10-20 15:10 [PATCH v1 1/1] CryptoPkg/BaseCryptLib: remove some duplicate initializations Peter Jones
2017-10-20 17:12 ` Laszlo Ersek
2017-10-20 18:21   ` Peter Jones
2017-10-23  3:02     ` Long, Qin
2017-10-24  7:51       ` Laszlo Ersek
2017-10-24  8:15         ` Long, Qin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox