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

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: improve-nsIX509CertDB-error-codes.patch

Comment 2

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

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.

Comment 3

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(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.

Comment 4

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/39a90347ea46

Comment 5

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/39a90347ea46