Closed Bug 1228296 Opened 4 years ago Closed 4 years ago

[Static Analysis][Dead Code] In function AuthCertificate from SSLServerCertVerification.cpp

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

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

People

(Reporter: andi, Assigned: andi)

References

(Blocks 1 open bug)

Details

(Keywords: coverity, Whiteboard: CID 1340236)

Attachments

(1 file)

The Static Analysis tool Coverity signaled a logically dead code at line 1301, since rv == SECSuccess the second time when this comparison is done on the true branch is useless so the false branch from the second comparison will never be hit.
Whiteboard: CID 1340236
Flags: needinfo?(dkeeler)
Comment on attachment 8692475 [details] [diff] [review]
Bug 1228296.diff

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

Thanks for the patch. I noted some nits. We already have a bug on this issue, so I'm going to mark this one as a duplicate of that one. Feel free to update this patch, attach it to the original bug, and ask for review again.

::: security/manager/ssl/SSLServerCertVerification.cpp
@@ +1290,5 @@
>      if (!status) {
>        status = new nsSSLStatus();
>        infoObject->SetSSLStatus(status);
>      }
> +    

nit: trailing whitespace

@@ +1291,5 @@
>        status = new nsSSLStatus();
>        infoObject->SetSSLStatus(status);
>      }
> +    
> +    // Certificate verification succeeded delete any potential record

nit: it would be nice if this comment could be updated to be a well-formed sentence or two.

@@ +1293,5 @@
>      }
> +    
> +    // Certificate verification succeeded delete any potential record
> +    // of certificate error bits.
> +    RememberCertErrorsTable::GetInstance().RememberCertHasError(infoObject, nullptr, rv);

nit: this line is >80 characters

@@ -1299,5 @@
> -                                                                  nullptr, rv);
> -    }
> -    else {
> -      // Certificate verification failed, update the status' bits.
> -      RememberCertErrorsTable::GetInstance().LookupCertErrorBits(

Hmmm - do we still need this to happen in the error case? It would be nice to investigate this.
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: needinfo?(dkeeler)
Resolution: --- → DUPLICATE
Duplicate of bug: 807711
You need to log in before you can comment on or make changes to this bug.