Closed
Bug 1273677
Opened 8 years ago
Closed 8 years ago
TLSServerSocket doesn't properly set up the session cache
Categories
(Core :: Networking, defect)
Core
Networking
Tracking
()
People
(Reporter: keeler, Assigned: keeler)
References
Details
(Whiteboard: [ft:conndevices][necko-active])
Attachments
(1 file)
(deleted),
text/x-review-board-request
|
mcmanus
:
review+
jocheng
:
approval-mozilla-b2g48+
|
Details |
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•8 years ago
|
||
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: flyweb
Updated•8 years ago
|
Attachment #8753565 -
Flags: review?(mcmanus) → review+
Comment 2•8 years ago
|
||
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•8 years ago
|
Whiteboard: [necko-active]
![]() |
Assignee | |
Comment 3•8 years 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•8 years 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
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•8 years 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•8 years 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•8 years 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•8 years 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 13•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/b6a065dc1103
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Comment 14•8 years 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?
Comment 15•8 years ago
|
||
please ni me again after approval to make sure I can see this in time.
Flags: needinfo?(xeonchen)
Comment 16•8 years 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•8 years ago
|
blocking-b2g: --- → 2.6+
Whiteboard: [necko-active] → [ft:conndevices][necko-active]
Comment 17•8 years ago
|
||
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.
Description
•