TLSServerSocket doesn't properly set up the session cache

RESOLVED FIXED in Firefox 49, Firefox OS v2.6

Status

()

Core
Networking
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: keeler, Assigned: keeler)

Tracking

(Blocks: 1 bug)

Trunk
mozilla49
Points:
---

Firefox Tracking Flags

(blocking-b2g:2.6+, firefox49 fixed, b2g-v2.6 fixed)

Details

(Whiteboard: [ft:conndevices][necko-active])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

TLSServerSocket (implementation of nsITLSServerSocket) doesn't set up the session cache, which is a problem as the cache defaults to being enabled. This issue can be exposed by running a server and having a client attempt to connect to it more than once.
Created attachment 8753565 [details]
MozReview Request: bug 1273677 - ensure session cache is properly configured and torn down for TLSServerSocket r?mcmanus

Review commit: https://reviewboard.mozilla.org/r/53352/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53352/
Attachment #8753565 - Flags: review?(mcmanus)
Blocks: 1228662
Attachment #8753565 - Flags: review?(mcmanus) → review+
Comment on attachment 8753565 [details]
MozReview Request: bug 1273677 - ensure session cache is properly configured and torn down for TLSServerSocket r?mcmanus

https://reviewboard.mozilla.org/r/53352/#review50368
Whiteboard: [necko-active]
Comment on attachment 8753565 [details]
MozReview Request: bug 1273677 - ensure session cache is properly configured and torn down for TLSServerSocket r?mcmanus

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53352/diff/1-2/
Thanks for the review. I forgot to add SSL_ShutdownServerSessionIDCache to nss.symbols, and I decreased the maximum cache size from 10000 (the default) to 1000 to save a bit of memory. Here's the try run:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5124406b3a4c

Comment 5

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b8f35a4774e
This caused valgrind leaks like https://treeherder.mozilla.org/logviewer.html#?job_id=28298947&repo=mozilla-inbound

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/7c847046bb5e
Flags: needinfo?(dkeeler)
Just for the record, this also leaked outside of valgrind jobs: https://treeherder.mozilla.org/logviewer.html#?job_id=28310651&repo=mozilla-inbound

Comment 8

a year ago
https://reviewboard.mozilla.org/r/53352/#review51074

::: security/manager/ssl/nsNSSComponent.cpp:1604
(Diff revision 2)
> +  // TLSServerSocket may be run with the session cache enabled. It is necessary
> +  // to call this once before that can happen. This specifies a maximum of 1000
> +  // cache entries (the default number of cache entries is 10000, which seems a
> +  // little excessive as there probably won't be that many clients connecting to
> +  // any TLSServerSockets the browser runs.)
> +  SSL_ConfigServerSessionIDCache(1000, 0, 0, nullptr);

Drive by since this got backed out: the return value should probably be checked or explicitly ignored.

::: security/manager/ssl/nsNSSComponent.cpp:1658
(Diff revision 2)
>      ShutdownSmartCardThreads();
>  #endif
>      SSL_ClearSessionCache();
> +    // TLSServerSocket may be run with the session cache enabled. This ensures
> +    // those resources are cleaned up.
> +    SSL_ShutdownServerSessionIDCache();

Same here.
It looks like SSL_ConfigServerSessionIDCache has to occur before all calls to SSL_ClearSessionCache. This should do it:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=19df616e6738
Flags: needinfo?(dkeeler)
Comment on attachment 8753565 [details]
MozReview Request: bug 1273677 - ensure session cache is properly configured and torn down for TLSServerSocket r?mcmanus

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53352/diff/2-3/
https://reviewboard.mozilla.org/r/53352/#review51074

> Drive by since this got backed out: the return value should probably be checked or explicitly ignored.

Good call.

Comment 12

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6a065dc1103

Comment 13

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/b6a065dc1103
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox49: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla49

Comment 14

a year ago
Comment on attachment 8753565 [details]
MozReview Request: bug 1273677 - ensure session cache is properly configured and torn down for TLSServerSocket r?mcmanus

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 1273677
User impact if declined: tls reconnection speed will be a little bit slow
Testing completed: self-tested
Risk to taking this patch (and alternatives if risky): low
String or UUID changes made by this patch: N/A
Flags: needinfo?(xeonchen)
Attachment #8753565 - Flags: approval-mozilla-b2g48?
please ni me again after approval to make sure I can see this in time.
Flags: needinfo?(xeonchen)
Comment on attachment 8753565 [details]
MozReview Request: bug 1273677 - ensure session cache is properly configured and torn down for TLSServerSocket r?mcmanus

Approve for TV 2.6
Attachment #8753565 - Flags: approval-mozilla-b2g48? → approval-mozilla-b2g48+

Updated

a year ago
blocking-b2g: --- → 2.6+
Whiteboard: [necko-active] → [ft:conndevices][necko-active]
https://github.com/mozilla-b2g/gecko-b2g/commit/00121045e713f47c3d440d87911632c58fec1146
status-b2g-v2.6: --- → fixed
You need to log in before you can comment on or make changes to this bug.