Closed Bug 1309145 Opened 3 years ago Closed 3 years ago

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

Categories

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

defect

Tracking

()

RESOLVED DUPLICATE of bug 807711
Tracking Status
firefox52 --- affected

People

(Reporter: andi, Assigned: andi)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: CID 1373580 [psm-assigned])

Attachments

(1 file)

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);
>>  }
Attachment #8799647 - Flags: review?(dkeeler)
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 .
Status: NEW → ASSIGNED
Component: Security → Security: PSM
Priority: -- → P1
Whiteboard: CID 1373580 → CID 1373580 [psm-assigned]
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).
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 807711
Comment on attachment 8799647 [details]
Bug 1309145 - if certificate validation fails update the status bits.

https://reviewboard.mozilla.org/r/84788/#review83586
Attachment #8799647 - Flags: review?(dkeeler)
You need to log in before you can comment on or make changes to this bug.