1309145 - [Static Analysis][Logically dead code] In function AuthCertificate

Status: RESOLVED DUPLICATE of bug 807711

(Core :: Security: PSM, defect, P1)

Description

[Andi [:andi]]

The Static Analysis tool Coverity detected that a dead code in present in the following context:

>>    if (rv == SECSuccess) {
>>      // Certificate verification succeeded delete any potential record
>>      // of certificate error bits.
>>      RememberCertErrorsTable::GetInstance().RememberCertHasError(infoObject,
>>                                                                  nullptr, rv);
>>    }
>>    else {
>>      // Certificate verification failed, update the status' bits.
>>      RememberCertErrorsTable::GetInstance().LookupCertErrorBits(
>>        infoObject, status);
>>    }

The else branch will never be executed since the context of the if is situated under:

>>  if (rv == SECSuccess) {
>>    GatherSuccessfulValidationTelemetry(certList);
>>    GatherCertificateTransparencyTelemetry(certList, 

What i propose to do is to put the statement under the else branch outside of the if statement and insert it under the statement:

>>  if (rv != SECSuccess) {
>>    // Certificate validation failed; store the peer certificate chain on
>>    // infoObject so it can be used for error reporting.
>>    infoObject->SetFailedCertChain(Move(peerCertChain));
>>    PR_SetError(savedErrorCode, 0);
>>  }

Comment 1

[Andi [:andi]]

Attachment: Bug 1309145 - if certificate validation fails update the status bits.

Review commit: https://reviewboard.mozilla.org/r/84788/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/84788/

Comment 2

[:Cykesiopka]

Comment on attachment 8799647 [details]
Bug 1309145 - if certificate validation fails update the status bits.

https://reviewboard.mozilla.org/r/84788/#review83382

Just FYI, you're going to need to rebase on top of https://hg.mozilla.org/mozilla-central/rev/12c51a960f26 .

Comment 3

[Andi [:andi]]

Comment on attachment 8799647 [details]
Bug 1309145 - if certificate validation fails update the status bits.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/84788/diff/1-2/

Comment 4

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Thanks for the patch. We actually already have a bug on this (coverity keeps thinking it's discovered a new issue each time the code moves around a bit).

Comment 5

[Dana Keeler (she/her) (use needinfo) [:keeler]]

Comment on attachment 8799647 [details]
Bug 1309145 - if certificate validation fails update the status bits.

https://reviewboard.mozilla.org/r/84788/#review83586