Add an explicit function to init NSS for SSL server sockets

Add an explicit function to init NSS for SSL server sockets

BUG=131622
TEST=tsan goes green for existing tests


Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=141955

[Ryan Sleevi, 2012-06-12 00:09:02]

wez: Can you provide any context as to where the remoting (host/client) init
code is? I found the pre-sandbox init code in
chrome/renderer/chrome_render_process_observer.cc, but it wasn't clear to me
where the transfer of control happens into the remoting client happens
(post-sandbox). We'd want to call this function there.

[Wez, 2012-06-12 16:29:46]

On 2012/06/12 00:09:02, Ryan Sleevi wrote:
> wez: Can you provide any context as to where the remoting (host/client) init
> code is? I found the pre-sandbox init code in
> chrome/renderer/chrome_render_process_observer.cc, but it wasn't clear to me
> where the transfer of control happens into the remoting client happens
> (post-sandbox). We'd want to call this function there.

The Chromoting client is a PPAPI plugin - see
remoting/client/plugin/chromoting_instance.*

The Chromoting IT2Me host is an NPAPI plugin - see
remoting/host/plugin/host_plugin.*

The Chromoting Me2Me host is a stand-alone executable - see
remoting/host/remoting_me2me_host.*

Are we sure that this is how we want to address the issue, though? Surely the
locking in the initializer is at fault, otherwise we wouldn't be seeing issues?

[Ryan Sleevi, 2012-06-12 17:22:24]

On 2012/06/12 16:29:46, Wez wrote:
> On 2012/06/12 00:09:02, Ryan Sleevi wrote:
> > wez: Can you provide any context as to where the remoting (host/client) init
> > code is? I found the pre-sandbox init code in
> > chrome/renderer/chrome_render_process_observer.cc, but it wasn't clear to me
> > where the transfer of control happens into the remoting client happens
> > (post-sandbox). We'd want to call this function there.
> 
> The Chromoting client is a PPAPI plugin - see
> remoting/client/plugin/chromoting_instance.*
> 
> The Chromoting IT2Me host is an NPAPI plugin - see
> remoting/host/plugin/host_plugin.*
> 
> The Chromoting Me2Me host is a stand-alone executable - see
> remoting/host/remoting_me2me_host.*
> 
> Are we sure that this is how we want to address the issue, though? Surely the
> locking in the initializer is at fault, otherwise we wouldn't be seeing
issues?

SSL_ConfigServerSessionIDCache is not thread-safe and not meant to be.

It's meant to be called early in process start-up, before any other threads and
before the process has forked. That's because it's meant to setup structures
that will be usable for multi-process SSL session caches (via SHMs)

Once initialized, the further use of the structures is thread-safe.

[Wez, 2012-06-12 17:27:13]

http://codereview.chromium.org/10543106/diff/9001/net/socket/nss_ssl_util.h
File net/socket/nss_ssl_util.h (right):

http://codereview.chromium.org/10543106/diff/9001/net/socket/nss_ssl_util.h#n...
net/socket/nss_ssl_util.h:15: // Initialize NSS SSL library.
Can you clarify what level of initialization this provides? From the layout you
have here, it's not clear that EnsureNSSSSLServerInit() does more than
EnsureNSSSSLInit() alone, rather than the other way around.
(Oh, and these function names are horrible. ):

[Ryan Sleevi, 2012-06-12 17:45:29]

http://codereview.chromium.org/10543106/diff/9001/net/socket/nss_ssl_util.h
File net/socket/nss_ssl_util.h (right):

http://codereview.chromium.org/10543106/diff/9001/net/socket/nss_ssl_util.h#n...
net/socket/nss_ssl_util.h:15: // Initialize NSS SSL library.
On 2012/06/12 17:27:13, Wez wrote:
> Can you clarify what level of initialization this provides? From the layout
you
> have here, it's not clear that EnsureNSSSSLServerInit() does more than
> EnsureNSSSSLInit() alone, rather than the other way around.
> (Oh, and these function names are horrible. ):

Yup. We've had the style discussion as to whether NSS or Nss is the preferred
means. Same with SSL vs Ssl.

The closest the style guide gets to naming is in the 'enhanced' Chromium style
guide ( http://dev.chromium.org/developers/coding-style ), which reads "Prefer
to capitalize well-known abbreviations as if they were words: HttpUrl instead of
HTTPURL". SSL may be well known, but is NSS?

The style for both has been all caps (SSLClientSocketNSS), hence keeping up with
the style.

[wtc, 2012-06-12 18:27:38]

Patch set 3 LGTM.  (I know patch set 3 is still missing
changes to some chrome files.)

The approach used in patch set 3 gets us the best performance
without creating an unused SSL server session ID cache.  But it is harder to
maintain, because the effect of
not calling SSL_ConfigServerSessionIDCache could be hard to
notice.

I remember we added the SSL_ConfigServerSessionIDCache call
for two reasons:
1. To enable session resumption
2. To fix crashes when there was no session cache.

We fixed several such crashes recently, so I suspect NSS may
be able to function without a server session cache now.

Some other approaches we can consider.

1. Add an SSL_Init function to initialize the session cache
locks early.  There was an attempt to add such a function
in https://bugzilla.mozilla.org/show_bug.cgi?id=172051 recently,
but we ended up adding just an internal ssl_Init function.

2. Patch our copy of NSS to only initialize those locks
lazily. The PR_CallOnce overhead should be acceptable.
It'll be hard to upstream this change.

3. Always call SSL_ConfigServerSessionIDCache in
NSSSSLInitSingleton.  This may create an unused server
session cache.  If peer-to-peer SSL becomes common, this
won't be a problem.

http://codereview.chromium.org/10543106/diff/9001/net/socket/nss_ssl_util.h
File net/socket/nss_ssl_util.h (right):

http://codereview.chromium.org/10543106/diff/9001/net/socket/nss_ssl_util.h#n...
net/socket/nss_ssl_util.h:18: // Initialize NSS SSL server sockets support.

We should document why this needs to be initialized separately
(server side code is not always used), and that this needs to
be called early to accomplish the "early init" effect.

http://codereview.chromium.org/10543106/diff/9001/net/socket/nss_ssl_util_int...
File net/socket/nss_ssl_util_internal.h (right):

http://codereview.chromium.org/10543106/diff/9001/net/socket/nss_ssl_util_int...
net/socket/nss_ssl_util_internal.h:14: // declared in nss_util.h.  Hence, they
are declared here.

nss_util.h => nss_ssl_util.h

[Ryan Sleevi, 2012-06-12 19:20:48]

wtc, wez: PTAL. Slightly different approach based on your feedback.

I've added a DCHECK to CreateSSLServerSocket to ensure it's been init'd, which
should hopefully reduce/eliminate the risk of someone trying to use the server
sockets before initializing the session cache.

[wtc, 2012-06-12 21:29:45]

Patch set 4 LGTM. wez needs to verify that the EnableSSLServerSocket
calls are at the right places.

http://codereview.chromium.org/10543106/diff/13001/net/socket/ssl_server_sock...
File net/socket/ssl_server_socket.h (right):

http://codereview.chromium.org/10543106/diff/13001/net/socket/ssl_server_sock...
net/socket/ssl_server_socket.h:38: // has been used.

Please add something like:
  If a process doesn't use SSL server sockets, this call may be
  omitted.

[Wez, 2012-06-12 21:30:35]

LGTM w/ nits for remoting/, plus I think simple_remoting_host.cc needs updating
to make the Init call as well.

http://codereview.chromium.org/10543106/diff/13001/net/socket/ssl_server_sock...
File net/socket/ssl_server_socket_openssl.cc (right):

http://codereview.chromium.org/10543106/diff/13001/net/socket/ssl_server_sock...
net/socket/ssl_server_socket_openssl.cc:10: void EnableSSLServerSockets() {}
nit: Consider NOTIMPLEMENTED(); here.

http://codereview.chromium.org/10543106/diff/13001/net/socket/ssl_server_sock...
net/socket/ssl_server_socket_openssl.cc:14: // provide a stub for it for now.
nit: Move this to a file-scope comment explaining that these stubs allow code
using SSL server sockets to compile, rather than us having to disable them, when
building for OpenSSL?

http://codereview.chromium.org/10543106/diff/13001/remoting/client/plugin/chr...
File remoting/client/plugin/chromoting_instance.cc (right):

http://codereview.chromium.org/10543106/diff/13001/remoting/client/plugin/chr...
remoting/client/plugin/chromoting_instance.cc:191:
net::EnableSSLServerSockets();
In practice I doubt it's an issue, but the Chromoting PPAPI plugin currently
runs in the renderer, which may contain other threads that could trigger an
action on an SSL server socket.

http://codereview.chromium.org/10543106/diff/13001/remoting/client/plugin/chr...
remoting/client/plugin/chromoting_instance.cc:191:
net::EnableSSLServerSockets();
nit: Add a comment to explain why we need to have the call at this point.

http://codereview.chromium.org/10543106/diff/13001/remoting/host/plugin/host_...
File remoting/host/plugin/host_plugin.cc (right):

http://codereview.chromium.org/10543106/diff/13001/remoting/host/plugin/host_...
remoting/host/plugin/host_plugin.cc:118: net::EnableSSLServerSockets();
nit: Add a comment explaining why we call this at this point.

http://codereview.chromium.org/10543106/diff/13001/remoting/host/remoting_me2...
File remoting/host/remoting_me2me_host.cc (right):

http://codereview.chromium.org/10543106/diff/13001/remoting/host/remoting_me2...
remoting/host/remoting_me2me_host.cc:102: net::EnableSSLServerSockets();
nit: Add a comment explaining why we have to do this at this point.

http://codereview.chromium.org/10543106/diff/13001/remoting/run_all_unittests.cc
File remoting/run_all_unittests.cc (right):

http://codereview.chromium.org/10543106/diff/13001/remoting/run_all_unittests...
remoting/run_all_unittests.cc:11: net::EnableSSLServerSockets();
nit: Comment explaining why we need this?

[Ryan Sleevi, 2012-06-12 22:35:40]

http://codereview.chromium.org/10543106/diff/13001/remoting/client/plugin/chr...
File remoting/client/plugin/chromoting_instance.cc (right):

http://codereview.chromium.org/10543106/diff/13001/remoting/client/plugin/chr...
remoting/client/plugin/chromoting_instance.cc:191:
net::EnableSSLServerSockets();
On 2012/06/12 21:30:35, Wez wrote:
> In practice I doubt it's an issue, but the Chromoting PPAPI plugin currently
> runs in the renderer, which may contain other threads that could trigger an
> action on an SSL server socket.

Right. I was trying to find where the post-sandbox Renderer init code lives. The
Pre-sandbox code configures NSS (for all renderer types), so it would make sense
that the post-sandbox code would initialize the server sockets.

If it is an issue though, we'll hit the DCHECK right away.

[commit-bot: I haz the power, 2012-06-13 18:06:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rsleevi@chromium.org/10543106/7013

[commit-bot: I haz the power, 2012-06-13 20:19:52]

Change committed as 141955