Closed Bug 1313715 Opened 5 years ago Closed 4 years ago

Avoid unnecessary use of PR_SetError() under security/apps/ and security/certverifier

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: Cykesiopka, Assigned: Cykesiopka)

References

Details

(Whiteboard: [psm-assigned])

Attachments

(1 file)

The PR_SetError() + PR_GetError() pattern is error prone and unnecessary.
Comment on attachment 8817896 [details]
Bug 1313715 - Avoid unnecessary uses of PR_SetError() under security/apps/ and security/certverifier/.

https://reviewboard.mozilla.org/r/98084/#review98716

Cool - lgtm. Just the one comment.

::: security/apps/AppTrustDomain.cpp:166
(Diff revision 1)
>    }
>  
>    mTrustedRoot.reset(CERT_NewTempCertificate(CERT_GetDefaultCertDB(),
>                                               &trustedDER, nullptr, false, true));
>    if (!mTrustedRoot) {
> -    return SECFailure;
> +    return NS_ERROR_OUT_OF_MEMORY;

We probably actually want to return mozilla::psm::GetXPCOMFromNSSError(PR_GetError()); here since this could be a more informative error (e.g. reused issuer/serial).

::: security/certverifier/NSSCertDBTrustDomain.cpp
(Diff revision 1)
>  
>  static const uint64_t ServerFailureDelaySeconds = 5 * 60;
>  
>  namespace mozilla { namespace psm {
>  
> -const char BUILTIN_ROOTS_MODULE_DEFAULT_NAME[] = "Builtin Roots Module";

Looks like this also takes care of bug 1254403.
Attachment #8817896 - Flags: review?(dkeeler) → review+
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/676ca54f13db
Avoid unnecessary uses of PR_SetError() under security/apps/ and security/certverifier/. r=keeler
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/676ca54f13db
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Blocks: 1254403
You need to log in before you can comment on or make changes to this bug.