Closed Bug 1271496 Opened 4 years ago Closed 4 years ago

Stop using Scoped.h in non-exported PSM code

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: Cykesiopka, Assigned: Cykesiopka)

References

Details

(Whiteboard: [psm-assigned])

Attachments

(1 file)

Scoped.h is deprecated in favour of the standardised UniquePtr.

This change will remove use of Scoped.h everywhere in PSM except ScopedNSSTypes.h, which is exported. Other consumers of ScopedNSSTypes.h can move off Scoped.h at their own pace.
Scoped.h is deprecated in favour of the standardised UniquePtr.

This patch removes use of Scoped.h everywhere in PSM except ScopedNSSTypes.h,
which is exported. Other consumers of ScopedNSSTypes.h can move off Scoped.h
at their own pace.

This patch also changes parameters and return types of various functions to make
ownership more explicit.

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

::: security/certverifier/NSSCertDBTrustDomain.cpp:104
(Diff revision 1)
>      const SECItem encodedIssuerNameItem = {
>        siBuffer,
>        const_cast<unsigned char*>(encodedIssuerName.UnsafeGetData()),
>        encodedIssuerName.GetLength()
>      };
> -    ScopedSECItem nameConstraints(::SECITEM_AllocItem(nullptr, nullptr, 0));
> +    UniqueSECItem nameConstraints(::SECITEM_AllocItem(nullptr, nullptr, 0));

Actually, this probably makes more sense as a ScopedAutoSECItem.
Comment on attachment 8755243 [details]
MozReview Request: Bug 1271496 - Stop using Scoped.h in non-exported PSM code. r=keeler

https://reviewboard.mozilla.org/r/54456/#review51282

LGTM.

::: security/manager/ssl/SSLServerCertVerification.cpp:946
(Diff revision 1)
>    UniquePLArenaPool arena(PORT_NewArena(DER_DEFAULT_CHUNKSIZE));
>    CERTGeneralName* subjectAltNames =
>      CERT_DecodeAltNameExtension(arena.get(), &altNameExtension);
>    // CERT_FindCertExtension takes a pointer to a SECItem and allocates memory
>    // in its data field. This is a bad way to do this because we can't use a
> -  // ScopedSECItem and neither is that memory tracked by an arena. We have to
> +  // UniqueSECItem and neither is that memory tracked by an arena. We have to

I think this is what ScopedAutoSECItem is for, actually (as you pointed out in a similar situation, above).

::: security/manager/ssl/nsNSSU2FToken.cpp:114
(Diff revision 1)
>      if (aNickname == freeKeyName.get()) {
>        MOZ_LOG(gNSSTokenLog, LogLevel::Debug, ("Symmetric key found!"));
> -      return freeKey.forget();
> +      return freeKey;
>      }
>  
>      keyList = PK11_GetNextSymKey(keyList);

Unfortunately, it looks like we have a preexisting leak here - if it happens that we don't return the last element in the list, all remaining keys in the list will be leaked. This can be addressed in a follow-up, however.

::: security/manager/ssl/nsNSSU2FToken.cpp:128
(Diff revision 1)
> -             ScopedSECKEYPublicKey& aPubKey, const nsNSSShutDownPreventionLock&)
> +     /*out*/ UniqueSECKEYPrivateKey& aPrivKey,
> +     /*out*/ UniqueSECKEYPublicKey& aPubKey,
> +             const nsNSSShutDownPreventionLock&)
>  {
> +  MOZ_ASSERT(aSlot);
> +  NS_ENSURE_ARG_POINTER(aSlot);

Well, aSlot isn't really a pointer, right? This might be best as |if (!aSlot) { return NS_ERROR_INVALID_ARG; }|

::: security/manager/ssl/nsNSSU2FToken.cpp:168
(Diff revision 1)
>  nsresult
>  nsNSSU2FToken::GetOrCreateWrappingKey(const UniquePK11SlotInfo& aSlot,
>                                        const nsNSSShutDownPreventionLock& locker)
>  {
> +  MOZ_ASSERT(aSlot);
> +  NS_ENSURE_ARG_POINTER(aSlot);

Same here.
Attachment #8755243 - Flags: review?(dkeeler) → review+
https://reviewboard.mozilla.org/r/54456/#review51282

Thanks!

> I think this is what ScopedAutoSECItem is for, actually (as you pointed out in a similar situation, above).

Indeed. I'll remove the comment and switch to using ScopedAutoSECItem in the relevant code.

> Unfortunately, it looks like we have a preexisting leak here - if it happens that we don't return the last element in the list, all remaining keys in the list will be leaked. This can be addressed in a follow-up, however.

Good point. I filed Bug 1275197.

> Well, aSlot isn't really a pointer, right? This might be best as |if (!aSlot) { return NS_ERROR_INVALID_ARG; }|

Will change as suggested.
(I wish we could just use mozilla::NotNull being added in Bug 1272203, but it doesn't support UniquePtr. Oh well.)
Comment on attachment 8755243 [details]
MozReview Request: Bug 1271496 - Stop using Scoped.h in non-exported PSM code. r=keeler

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54456/diff/1-2/
Attachment #8755243 - Attachment description: MozReview Request: Bug 1271496 - Stop using Scoped.h in non-exported PSM code. → MozReview Request: Bug 1271496 - Stop using Scoped.h in non-exported PSM code. r=keeler
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d58a4ede1eea
(The failures appear to be existing intermittents.)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/34f82d838f03
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
You need to log in before you can comment on or make changes to this bug.