1260643 - Convert most uses of ScopedCERTCertificate in PSM to UniqueCERTCertificate

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[:Cykesiopka]

ScopedCERTCertificate is based on Scoped.h, which is deprecated. We should use the UniquePtr equivalent.

Comment 1

[:Cykesiopka]

s/Convert uses/Convert most uses/ because there's been new code landed recently using Scoped[...] that's being actively worked upon, and it seems best to avoid conflicts for now.

Comment 2

[:Cykesiopka]

Attachment: MozReview Request: Bug 1260643 - Convert most uses of ScopedCERTCertificate in PSM to UniqueCERTCertificate. r=keeler

Review commit: https://reviewboard.mozilla.org/r/46939/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/46939/

Comment 3

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

Comment on attachment 8742096 [details]
MozReview Request: Bug 1260643 - Convert most uses of ScopedCERTCertificate in PSM to UniqueCERTCertificate. r=keeler

https://reviewboard.mozilla.org/r/46939/#review44293

LGTM.

::: security/certverifier/CertVerifier.cpp:662
(Diff revision 1)
>                   /*optional out*/ OCSPStaplingStatus* ocspStaplingStatus,
>                   /*optional out*/ KeySizeStatus* keySizeStatus,
>                   /*optional out*/ SHA1ModeResult* sha1ModeResult,
>                   /*optional out*/ PinningTelemetryInfo* pinningTelemetryInfo)
>  {
> -  PR_ASSERT(peerCert);
> +  PR_ASSERT(peerCert.get());

Does PR_ASSERT not cause operator bool() to be used on the UniqueCERTCertificate? (As in, why does .get() need to be added here?) (I guess I'm a little unsure when .get() needs to be used vs. not used.)

Comment 4

[:Cykesiopka]

https://reviewboard.mozilla.org/r/46939/#review44293

Thanks!

> Does PR_ASSERT not cause operator bool() to be used on the UniqueCERTCertificate? (As in, why does .get() need to be added here?) (I guess I'm a little unsure when .get() needs to be used vs. not used.)

I originally changed these because I wasn't 100% sure operator bool() would work correctly, and I wanted to play it safe.
I did some testing, and it looks like all the ASSERT and NS_ENSURE macros use operator bool() correctly, so I will revert these changes.

Comment 5

[:Cykesiopka]

Comment on attachment 8742096 [details]
MozReview Request: Bug 1260643 - Convert most uses of ScopedCERTCertificate in PSM to UniqueCERTCertificate. r=keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/46939/diff/1-2/

Comment 6

[:Cykesiopka]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2f87c417d7f60cbdb5b9e58c252e387ec8b152e6

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9c98c0300a89

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/9c98c0300a89