Closed
Bug 1444532
Opened 7 years ago
Closed 7 years ago
the static nsICryptoHash in SHA256 (nsHttpConnectionInfo.cpp) is never released and thus leaks
Categories
(Core :: Networking, defect, P1)
Core
Networking
Tracking
()
RESOLVED
FIXED
mozilla61
People
(Reporter: keeler, Assigned: keeler)
References
(Blocks 1 open bug)
Details
(Whiteboard: [necko-triaged])
Attachments
(1 file)
59 bytes,
text/x-review-board-request
|
mayhemer
:
review+
jcristau
:
approval-mozilla-esr60+
|
Details |
static nsresult
SHA256(const char* aPlainText, nsAutoCString& aResult)
{
static nsICryptoHash* hasher = nullptr;
nsresult rv;
if (!hasher) {
rv = CallCreateInstance("@mozilla.org/security/hash;1", &hasher);
if (NS_FAILED(rv)) {
LOG(("nsHttpDigestAuth: no crypto hash!\n"));
return rv;
}
}
rv = hasher->Init(nsICryptoHash::SHA256);
NS_ENSURE_SUCCESS(rv, rv);
rv = hasher->Update((unsigned char*) aPlainText, strlen(aPlainText));
NS_ENSURE_SUCCESS(rv, rv);
rv = hasher->Finish(false, aResult);
return rv;
}
`hasher` is never released. Also hopefully this is only ever run on one thread?
Comment hidden (mozreview-request) |
Updated•7 years ago
|
Attachment #8957720 -
Flags: review?(mcmanus) → review?(honzab.moz)
Updated•7 years ago
|
Priority: P1 → P3
Whiteboard: [necko-triaged]
![]() |
||
Comment 3•7 years ago
|
||
mozreview-review |
Comment on attachment 8957720 [details]
bug 1444532 - fix a leak in SHA256 in nsHttpConnectionInfo.cpp
https://reviewboard.mozilla.org/r/226678/#review233686
::: netwerk/protocol/http/nsHttpConnectionInfo.cpp:29
(Diff revision 1)
> static nsresult
> SHA256(const char* aPlainText, nsAutoCString& aResult)
> {
> - static nsICryptoHash* hasher = nullptr;
> - nsresult rv;
> + nsresult rv;
> - if (!hasher) {
> + nsCOMPtr<nsICryptoHash> hasher =
we also have static refptrs (StaticRefPtr)
Attachment #8957720 -
Flags: review?(honzab.moz) → review+
![]() |
Assignee | |
Comment 4•7 years ago
|
||
Thanks! I think having this be non-static is best (there's very little benefit to doing it that way).
Flags: needinfo?(honzab.moz)
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ad1e07a06363
fix a leak in SHA256 in nsHttpConnectionInfo.cpp r=mayhemer
Comment 6•7 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 7 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Comment 7•7 years ago
|
||
Thanks everyone here for finding and fixing this bug, and sorry for my mistake. Would it be possible to backport the patch to esr60?
Updated•7 years ago
|
status-firefox61:
--- → fixed
![]() |
Assignee | |
Comment 8•7 years ago
|
||
Comment on attachment 8957720 [details]
bug 1444532 - fix a leak in SHA256 in nsHttpConnectionInfo.cpp
[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: This may have inadvertently fixed a crash bug - see https://crash-stats.mozilla.com/signature/?signature=PK11_ExitContextMonitor and bug 1462386.
User impact if declined: See bug 1462386. Apparently "Setting a SOCKS proxy with password in browser.proxy.onRequest WebExtension API crashes the browser after navigating a while."
Fix Landed on Version: 61
Risk to taking this patch (and alternatives if risky): low - the change is small, localized, and the idiom in use is common for that implementation
String or UUID changes made by this patch: none
See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8957720 -
Flags: approval-mozilla-esr60?
Comment 10•7 years ago
|
||
Comment on attachment 8957720 [details]
bug 1444532 - fix a leak in SHA256 in nsHttpConnectionInfo.cpp
leak fix, approved for 60.1esr
Attachment #8957720 -
Flags: approval-mozilla-esr60? → approval-mozilla-esr60+
Comment 11•7 years ago
|
||
bugherder uplift |
Updated•7 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•