Closed Bug 1102443 Opened 5 years ago Closed 5 years ago
the call to CERT
_Get Common Name leaks memory in Public Key Pinning Service::Eval Chain With Hash Type
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);
5 years ago
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?
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
Whiteboard: [good first bug]
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+
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.