The default bug view has changed. See this FAQ.

TLSServerSocket doesn't properly set up the session cache

RESOLVED FIXED in Firefox 49, Firefox OS v2.6

Status

()

Core
Networking
RESOLVED FIXED
10 months ago
10 months 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.
(Assignee)

Comment 1

10 months ago
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

Updated

10 months ago
Whiteboard: [necko-active]
(Assignee)

Comment 3

10 months ago
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/
(Assignee)

Comment 4

10 months ago
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

10 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b8f35a4774e

Comment 6

10 months ago
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)

Comment 7

10 months ago
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

10 months 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.
(Assignee)

Comment 9

10 months ago
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)
(Assignee)

Comment 10

10 months ago
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/
(Assignee)

Comment 11

10 months ago
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

10 months ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6a065dc1103

Comment 13

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

Comment 14

10 months 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 16

10 months ago
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

10 months 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.