certificate reference leaks in IsCertificateDistrustImminent

VERIFIED FIXED in Firefox 58

Status

()

defect
P1
normal
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: keeler, Assigned: keeler)

Tracking

unspecified
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox57 unaffected, firefox58+ verified, firefox59 verified)

Details

(Whiteboard: [psm-assigned])

Attachments

(1 attachment)

nsIX509Cert::GetCert returns a CERTCertificate whose reference count has already been increased. When IsCertificateDistrustImminent calls CertDNIsInList(rootCert->GetCert(), RootSymantecDNs) and CertDNIsInList(aCert->GetCert(), RootAppleAndGoogleDNs), the reference count on those certificates never gets a corresponding decrement, so we keep those certificates alive until shut down. This can be demonstrated by setting MOZ_LOG="pipnss:4", running Firefox, connecting to any host, and then shutting down. "[Main Thread]: E/pipnss NSS SHUTDOWN FAILURE" will be in the console output.
[Tracking Requested - why for this release]: this could potentially keep more certificates alive in memory than before (note that all certificates are ref-counted/de-duplicated, so the excess memory would probably only be on the order of how many distinct domains are visited per session) (and it may be that things like history, the network cache, etc., keep these certificate alive anyway, in which case this wouldn't have a noticeable impact).
Comment on attachment 8935559 [details]
bug 1424085 - add owning handles so cert references don't leak in IsCertificateDistrustImminent

https://reviewboard.mozilla.org/r/206434/#review212108
Attachment #8935559 - Flags: review?(jjones) → review+
Pushed by dkeeler@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/89457644e922
add owning handles so cert references don't leak in IsCertificateDistrustImminent r=jcj
https://hg.mozilla.org/mozilla-central/rev/89457644e922
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
Please request Beta approval on this when you get a chance.
Flags: needinfo?(dkeeler)
Comment on attachment 8935559 [details]
bug 1424085 - add owning handles so cert references don't leak in IsCertificateDistrustImminent

Approval Request Comment
[Feature/Bug causing the regression]: bug 1409259
[User impact if declined]: potentially unnecessary extra memory usage
[Is this code covered by automated tests?]: the feature added in bug 1409259 has tests, but not the resource leak
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: yes - steps to verify in comment 0
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: very low risk
[Why is the change risky/not risky?]: the fix involves adding a smart pointer to keep track of a resource - it's unlikely this will cause any problems
[String changes made/needed]: none
Flags: needinfo?(dkeeler)
Attachment #8935559 - Flags: approval-mozilla-beta?
Comment on attachment 8935559 [details]
bug 1424085 - add owning handles so cert references don't leak in IsCertificateDistrustImminent

Memory leak that regressed in 58. Let's uplift for 58.0b12.
Attachment #8935559 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Andrei, another issue good to verify in beta once this lands.
Flags: needinfo?(andrei.vaida)
I have managed to reproduce this issue using an affected 58 Beta 10 build. The following failure "[Main Thread]: E/pipnss NSS SHUTDOWN FAILURE" was displayed in the logs/console output after closing Firefox.

This is verified fixed on 58.0b12 (20171218174357), 59.0a1 (2017-12-19) across platforms: Win 10 x64, Mac OS  X 10.11 and Ubuntu 16.04 x64. I can no longer see failures related to pipnss, "[Main Thread]: D/pipnss NSS shutdown =====>> OK <<=====".
Status: RESOLVED → VERIFIED
Flags: needinfo?(andrei.vaida)
You need to log in before you can comment on or make changes to this bug.