Closed Bug 978185 Opened 6 years ago Closed 6 years ago

nsIX509CertDB.addCert and nsIX509CertDB.setCertTrust return unhelpful vague error codes when they fail

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla30

People

(Reporter: briansmith, Assigned: briansmith)

References

Details

Attachments

(1 file)

No description provided.
Comment on attachment 8383840 [details] [diff] [review]
improve-nsIX509CertDB-error-codes.patch

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

LGTM.

::: security/manager/ssl/src/nsNSSCertificateDB.cpp
@@ +1601,5 @@
>    der.len = 0;
>  
>    if (!tmpCert) {
>      NS_ERROR("Couldn't create cert from DER blob");
> +    return MapSECStatus(SECFailure);

Are we sure there won't be any calls to PR_SetError between CERT_NewTempCertificate and here? It looks like it, but maybe it would be safest to cache the error and then re-set it.
Attachment #8383840 - Flags: review?(dkeeler) → review+
(In reply to David Keeler (:keeler) from comment #2)
> Are we sure there won't be any calls to PR_SetError between
> CERT_NewTempCertificate and here? It looks like it, but maybe it would be
> safest to cache the error and then re-set it.

Cleanup functions like nsMemory::Free mustn't change the error code because otherwise things (like this and more complicated) get really messed up. I agree it is not ideal that we don't have this documented though.
https://hg.mozilla.org/mozilla-central/rev/39a90347ea46
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.