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

Status: RESOLVED DUPLICATE of bug 807711

(Core :: Security: PSM, defect)

Description

[Andi [:andi]]

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.

Comment 1

[Andi [:andi]]

Attachment: Bug 1228296.diff

Comment 2

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

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.