Closed Bug 1102443 Opened 5 years ago Closed 5 years ago

the call to CERT_GetCommonName leaks memory in PublicKeyPinningService::EvalChainWithHashType

Categories

(Core :: Security: PSM, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox37 --- wontfix
firefox38 --- fixed
firefox39 --- fixed

People

(Reporter: keeler, Assigned: keeler)

References

Details

Attachments

(3 files)

PublicKeyPinningService.cpp:

133     PR_LOG(gPublicKeyPinningLog, PR_LOG_DEBUG,
134            ("pkpin: certArray common_name: '%s'\n",
135             CERT_GetCommonName(&(currentCert->issuer))));

cert.h says:

759 /* The return value must be freed with PORT_Free. */
760 extern char *CERT_GetCommonName(const CERTName *name);
Mentor: dkeeler
Whiteboard: [good first bug]
I'll take this as my very first bug fix.

In C++, one must in general use something like smart pointers to manage resources, including resources other than memory - file handles, network sockets, mutexes, database locks and so on.  To manage a mutex, you'd create a "smart mutex" that unlocks the underlying, real mutex when it goes out of scope, or if a pointer to the smart mutex is deleted.

If - as in this case - it's a memory resource, but requires a special function to free it, then you need a smart pointer whose destructor calls that special function, in this case PORT_Free.

To the extent you can, you want to strive for empty constructor bodies as well as empty destructor bodies.  The main case in which you want a nontrivial destructor is in this case, when you delete the resource that a smart pointer-like object is managing.  But even in this case you want an empty constructor body as the resource is allocated in the constructor's initialization list.

This is known as RIAA, for Resource Acquisition Is Initialization.
I am unable to assign this bug to myself.  Could an admin please grant me the "editbugs" permission?  I promise to play nice in The Series of Tubes.
I'm not an admin, but I've assigned you to the bug. I recommend using the ScopedPtr in mozilla::pkix like so: http://hg.mozilla.org/mozilla-central/annotate/912036eeb024/security/pkix/test/lib/pkixtestnss.cpp#l163
Let me know if you need additional resources or pointers.
Thanks for taking this on!
Assignee: nobody → mdcrawford
Michael, how's this bug going? Is there anything I can do to help?
Flags: needinfo?(mdcrawford)
Duplicate of this bug: 1142723
Haven't heard from Michael in a while, and this needs fixing sooner rather than later. I'll just go ahead and take care of this.
Assignee: mdcrawford → dkeeler
Mentor: dkeeler
Flags: needinfo?(mdcrawford)
Whiteboard: [good first bug]
Attached patch patchSplinter Review
Attachment #8576914 - Flags: review?(cykesiopka.bmo)
Attached file 1102443-after.txt
For reference, compare these two files to see the changes in the logging output made by the patch.
Comment on attachment 8576914 [details] [diff] [review]
patch

Review of attachment 8576914 [details] [diff] [review]:
-----------------------------------------------------------------

LGTM.
Attachment #8576914 - Flags: review?(cykesiopka.bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/6ef6ea1d4437
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
For posterity, Michael emailed and asked me to mention that he hasn't been able to log in to bugzilla, which is why we haven't heard from him.
This missed the boat for 37, but it should probably get in 38 at least.
Comment on attachment 8576914 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: key pinning (bug 744204)
[User impact if declined]: leaks in debug builds
[Describe test coverage new/current, TreeHerder]: n/a (although there are tests that make sure this change doesn't break the overall feature)
[Risks and why]: low (in fact, this really only makes the code more safe)
[String/UUID change made/needed]: none
Attachment #8576914 - Flags: approval-mozilla-aurora?
Attachment #8576914 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.