Disable NSS_ALLOW_SSLKEYLOGFILE

RESOLVED FIXED in Firefox -esr60

Status

()

defect
RESOLVED FIXED
4 months ago
4 months ago

People

(Reporter: erahm, Assigned: erahm)

Tracking

(Blocks 1 bug)

65 Branch
Firefox 66
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr60 fixed, firefox65 fixed, firefox66 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

4 months ago

Due to potential shutdown hangs we'd like to temporarily disable NSS_ALLOW_SSLKEYLOGFILE [1] by default in non-nightly releases.

[1] https://searchfox.org/mozilla-central/rev/13788edbabb04d004e4a1ceff41d4de68a8320a2/security/nss/lib/ssl/ssl.gyp#97

Assignee

Comment 1

4 months ago
This disables NSS_ALLOW_SSLKEYLOGFILE in beta in release in order to avoid shutdown hangs until the NSS project has time to fix the root cause of the issue.

Nathan can you review this? It's really a build system change at this point. I have a [beta simulation running](https://treeherder.mozilla.org/#/jobs?repo=try&revision=837654f11970c3269bcf0d08ec8a68d3c30e94f5) to make sure it does the right thing.
Attachment #9035763 - Flags: review?(nfroyd)
Assignee

Updated

4 months ago
Assignee: nobody → erahm
Status: NEW → ASSIGNED
Comment on attachment 9035763 [details] [diff] [review]
Disable NSS_ALLOW_SSLKEYLOGFILE in beta and release

Review of attachment 9035763 [details] [diff] [review]:
-----------------------------------------------------------------

Note that we're trying to do this without changing NSS at all.  The Right Thing in some respect is to make this setting configurable at the NSS level, but since we're not taking that route (for now?), this was the best thing I could think of.  glandium might have strong opinions about this, and I thought up the idea, so I don't think I should be the person to review this.
Attachment #9035763 - Flags: review?(nfroyd) → review?(mh+mozilla)
Comment on attachment 9035763 [details] [diff] [review]
Disable NSS_ALLOW_SSLKEYLOGFILE in beta and release

Review of attachment 9035763 [details] [diff] [review]:
-----------------------------------------------------------------

Considering how things work for defines and gyp, this might well be the only solution without touching NSS.

::: python/mozbuild/mozbuild/frontend/gyp_reader.py
@@ +250,5 @@
>                  if '=' in define:
>                      name, value = define.split('=', 1)
> +                    # The NSS gyp file doesn't expose a way to override this
> +                    # currently, so we do so here.
> +                    if name == 'NSS_ALLOW_SSLKEYLOGFILE' and config.substs.get('RELEASE_OR_BETA', False):

You don't need that `, False`.
Attachment #9035763 - Flags: review?(mh+mozilla) → review+

Comment 4

4 months ago
Pushed by erahm@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0801165e3175
Disable NSS_ALLOW_SSLKEYLOGFILE in beta and release. r=glandium
Assignee

Comment 5

4 months ago

Comment on attachment 9035763 [details] [diff] [review]
Disable NSS_ALLOW_SSLKEYLOGFILE in beta and release

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: Bug 1188657

User impact if declined: This can cause random hangs in the networking threads when working with secure connections. This particularly shows up as shutdown hangs in crash stats.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: No

Needs manual test from QE?: No

If yes, steps to reproduce:

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): We're just disabling the option to enable a feature which works around a bug in NSS related to checking if the feature is enabled.

String changes made/needed: N/A

[ESR Uplift Approval Request]

If this is not a sec:{high,crit} bug, please state case for ESR consideration: This disables a feature that can cause networking and shutdown hangs.

User impact if declined: This can cause random hangs in the networking threads when working with secure connections. This particularly shows up as shutdown hangs in crash stats.

Fix Landed on Version: 66

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): We're just disabling the option to enable a feature which works around a bug in NSS related to checking if the feature is enabled.

String or UUID changes made by this patch: N/A

Attachment #9035763 - Flags: approval-mozilla-esr60?
Attachment #9035763 - Flags: approval-mozilla-beta?
Assignee

Comment 6

4 months ago

(In reply to Eric Rahm [:erahm] from comment #1)

I have a [beta simulation
running](https://treeherder.mozilla.org/#/
jobs?repo=try&revision=837654f11970c3269bcf0d08ec8a68d3c30e94f5) to make
sure it does the right thing.

I've verified that the SSLKEYLOGFILE string used to check if an env var is set is no longer present in a simulated beta build by running:

strings libssl3.so | grep SSLKEYLOGFILE

Comment 7

4 months ago
bugherder
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 66

Comment on attachment 9035763 [details] [diff] [review]
Disable NSS_ALLOW_SSLKEYLOGFILE in beta and release

[Triage Comment]
Enables NSS_ALLOW_SSLKEYLOGFILE on Nightly builds only to avoid shutdown hangs. Approved for 65.0b11 and 60.5.0esr.

Attachment #9035763 - Flags: approval-mozilla-esr60?
Attachment #9035763 - Flags: approval-mozilla-esr60+
Attachment #9035763 - Flags: approval-mozilla-beta?
Attachment #9035763 - Flags: approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.