Closed Bug 1247847 Opened 5 years ago Closed 5 years ago

Use smart pointers in nsNSSCertHelper.cpp to manage NSS resources

Categories

(Core :: Security: PSM, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla47
Tracking Status
firefox47 --- fixed

People

(Reporter: Cykesiopka, Assigned: Cykesiopka)

Details

Attachments

(1 file)

nsNSSCertHelper.cpp currently uses raw pointers, which are not as nice and easier to get wrong.
This lets us remove things like gotos in the code, and makes resource ownership slightly clearer.

Review commit: https://reviewboard.mozilla.org/r/34721/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/34721/
Attachment #8718715 - Flags: review?(dkeeler)
https://reviewboard.mozilla.org/r/34721/#review31355

::: security/manager/ssl/nsNSSCertHelper.cpp:963
(Diff revision 1)
> -	if (SEC_ASN1DecodeItem(arena, &decoded, 
> +        if (SEC_ASN1DecodeItem(arena.get(), &decoded, 

Trailing whitespace.

::: security/manager/ssl/nsNSSCertHelper.cpp:1113
(Diff revision 1)
> -  if (SEC_QuickDERDecodeItem(arena, &decoded, 
> +  if (SEC_QuickDERDecodeItem(arena.get(), &decoded, 

Trailing whitespace.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ccc3b28a19de

I did brief testing with the cert viewer, and things seemed to work OK.
At the very least, this builds.
As a heads-up, I probably won't get to this until Tuesday.
Comment on attachment 8718715 [details]
MozReview Request: Bug 1247847 - Use smart pointers in nsNSSCertHelper.cpp to manage NSS resources. r=keeler

https://reviewboard.mozilla.org/r/34721/#review31437

LGTM.

::: security/manager/ssl/ScopedNSSTypes.h:17
(Diff revision 1)
>  #ifndef MOZ_NO_MOZALLOC

For these, I've found it's useful to separate them out below the rest of the #includes (that way, selecting all other includes and sorting works without reordering the #ifdef/#endif)

::: security/manager/ssl/nsNSSCertHelper.cpp:1177
(Diff revision 1)
>    SECItem **itemList;

nit: ** placement if you want to address it here

::: security/manager/ssl/nsNSSCertHelper.cpp:1338
(Diff revision 1)
>    CRLDistributionPoint **points, *point;

nit: * again
Also, these should really be declared on separate lines.
Comment on attachment 8718715 [details]
MozReview Request: Bug 1247847 - Use smart pointers in nsNSSCertHelper.cpp to manage NSS resources. r=keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/34721/diff/1-2/
Attachment #8718715 - Attachment description: MozReview Request: Bug 1247847 - Use smart pointers in nsNSSCertHelper.cpp to manage NSS resources. → MozReview Request: Bug 1247847 - Use smart pointers in nsNSSCertHelper.cpp to manage NSS resources. r=keeler
https://reviewboard.mozilla.org/r/34721/#review31437

Thanks for the review!

> For these, I've found it's useful to separate them out below the rest of the #includes (that way, selecting all other includes and sorting works without reordering the #ifdef/#endif)

Makes sense.

> nit: ** placement if you want to address it here

Done.

> nit: * again
> Also, these should really be declared on separate lines.

Done. There are still some of these remaining, but I will defer these and other cleanups to another time, just so the changes here don't spiral out of control.
https://hg.mozilla.org/mozilla-central/rev/866193a135f8
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.