1444532 - the static nsICryptoHash in SHA256 (nsHttpConnectionInfo.cpp) is never released and thus leaks

Status: RESOLVED FIXED

(Core :: Networking, defect, P1)

Description

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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 1

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Attachment: bug 1444532 - fix a leak in SHA256 in nsHttpConnectionInfo.cpp

The original code (from bug 1200802) declared an XPCOM object as a static bare
pointer, which for future reference is probably never the right thing to do. It
might have worked if it was cleared before shutdown but it never was.

Review commit: https://reviewboard.mozilla.org/r/226678/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/226678/

Comment 2

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Review ping, Honza?

Comment 3

[Honza Bambas (:mayhemer)]

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)

Comment 4

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

Thanks! I think having this be non-static is best (there's very little benefit to doing it that way).

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ad1e07a06363
fix a leak in SHA256 in nsHttpConnectionInfo.cpp r=mayhemer

Comment 6

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/ad1e07a06363

Comment 7

[Arthur Edelstein [:arthur]]

Thanks everyone here for finding and fixing this bug, and sorry for my mistake. Would it be possible to backport the patch to esr60?

Comment 8

[Dana Keeler (she/her) (use needinfo) [:keeler] (on leave)]

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.

Comment 10

[Julien Cristau [:jcristau]]

Comment on attachment 8957720 [details]
bug 1444532 - fix a leak in SHA256 in nsHttpConnectionInfo.cpp

leak fix, approved for 60.1esr

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/7aec8de1c9d5