Closed Bug 1351779 Opened 4 years ago Closed 4 years ago

Unused variable loopDetected in pkixbuild.cpp:164

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: ttaubert, Assigned: ttaubert)

Details

(Whiteboard: [psm-assigned])

Attachments

(1 file)

> bool loopDetected = false;
> for (const BackCert* prev = potentialIssuer.childCert;
>      !loopDetected && prev != nullptr; prev = prev->childCert) {
>   if (InputsAreEqual(potentialIssuer.GetSubjectPublicKeyInfo(),
>                      prev->GetSubjectPublicKeyInfo()) &&
>       InputsAreEqual(potentialIssuer.GetSubject(), prev->GetSubject())) {
>     // XXX: error code
>     return RecordResult(Result::ERROR_UNKNOWN_ISSUER, keepGoing);
>   }
> }

`loopDetected` is not and was never used. I checked the implementation of the loop check against the RFC and it seems correct. I think we can simply get rid of the variable.
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Comment on attachment 8852605 [details] [diff] [review]
0001-Bug-1351779-Removed-unused-variable-loopDetected-fro.patch

Review of attachment 8852605 [details] [diff] [review]:
-----------------------------------------------------------------

d'oh. Thanks for the fix.

::: security/pkix/lib/pkixbuild.cpp
@@ +160,5 @@
>  
>    // Loop prevention, done as recommended by RFC4158 Section 5.2
>    // TODO: this doesn't account for subjectAltNames!
>    // TODO(perf): This probably can and should be optimized in some way.
> +  for (const BackCert* prev = potentialIssuer.childCert; prev != nullptr;

nit: we should be able to use just `prev` as the loop condition, right?
Attachment #8852605 - Flags: review?(dkeeler) → review+
Priority: -- → P1
Whiteboard: [psm-assigned]
(In reply to David Keeler [:keeler] (use needinfo?) from comment #2)
> >    // Loop prevention, done as recommended by RFC4158 Section 5.2
> >    // TODO: this doesn't account for subjectAltNames!
> >    // TODO(perf): This probably can and should be optimized in some way.
> > +  for (const BackCert* prev = potentialIssuer.childCert; prev != nullptr;
> 
> nit: we should be able to use just `prev` as the loop condition, right?

Yeah, I'll make that change before landing. Thanks!
Pushed by ttaubert@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa294dc82ca5
Removed unused variable 'loopDetected' from PathBuildingStep::Check() r=keeler
https://hg.mozilla.org/mozilla-central/rev/fa294dc82ca5
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.