Closed
Bug 1102443
Opened 10 years ago
Closed 10 years ago
the call to CERT_GetCommonName leaks memory in PublicKeyPinningService::EvalChainWithHashType
Categories
(Core :: Security: PSM, defect)
Core
Security: PSM
Tracking
()
RESOLVED
FIXED
mozilla39
People
(Reporter: keeler, Assigned: keeler)
References
Details
Attachments
(3 files)
(deleted),
patch
|
Cykesiopka
:
review+
Sylvestre
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
(deleted),
text/plain
|
Details | |
(deleted),
text/plain
|
Details |
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);
Assignee | ||
Updated•10 years ago
|
Mentor: dkeeler
Whiteboard: [good first bug]
Comment 1•10 years ago
|
||
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.
Comment 2•10 years ago
|
||
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.
Assignee | ||
Comment 3•10 years ago
|
||
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
Assignee | ||
Comment 4•10 years ago
|
||
Michael, how's this bug going? Is there anything I can do to help?
Flags: needinfo?(mdcrawford)
Assignee | ||
Comment 6•10 years ago
|
||
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]
Assignee | ||
Comment 7•10 years ago
|
||
Attachment #8576914 -
Flags: review?(cykesiopka.bmo)
Assignee | ||
Comment 8•10 years ago
|
||
Assignee | ||
Comment 9•10 years ago
|
||
For reference, compare these two files to see the changes in the logging output made by the patch.
Comment 10•10 years ago
|
||
Comment on attachment 8576914 [details] [diff] [review]
patch
Review of attachment 8576914 [details] [diff] [review]:
-----------------------------------------------------------------
LGTM.
Attachment #8576914 -
Flags: review?(cykesiopka.bmo) → review+
Assignee | ||
Comment 11•10 years ago
|
||
Comment 12•10 years ago
|
||
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Assignee | ||
Comment 13•10 years ago
|
||
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.
Assignee | ||
Comment 14•10 years ago
|
||
This missed the boat for 37, but it should probably get in 38 at least.
status-firefox37:
--- → wontfix
status-firefox38:
--- → affected
Assignee | ||
Comment 15•10 years ago
|
||
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?
Updated•10 years ago
|
Attachment #8576914 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 16•10 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•