804663 - Create a CryptoTask API to simplify the creation of correct async crypto operations

Status: RESOLVED FIXED

(Core :: Security: PSM, defect, P1)

Description

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Create a CryptoTask API to simplify the creation of correct async crypto operations

Many (most) certificate and crypto operations need to happen off the main thread. One reason is that asymmetric crypto operations are slow. (Note that many existing PSM APIs do not follow this.) Another reason is that certificate operations may do disk I/O and/or network I/O behind the schemes.

When doing async operations, we need to handle race conditions between NSS shutdown and the execution and/or completion of our async operation. The nsNSSShutdownObject interface helps for this, but for any operations, a simpler interface makes more sense.

Comment 1

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Create a CryptoTask API to simplify the creation of correct async crypto operations and add more utilities to ScopedNSSTypes.h

These are the utility changes needed for B2G signature verification and also BrowserID signature verification. Suggestions welcome on a better place to put the new utilities that I stuffed in ScopedNSSTypes.h.

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Create a CryptoTask API to simplify the creation of correct async crypto operations and add more utilities to ScopedNSSTypes.h [v2]

This version fixes a potential race condition at shutdown. I think that this type of race condition (destructor running on a background thread while NSS is looping through the nsNSSShutDownList on the main thread) is also the cause of the other NSS shutdown crashes.

Comment 3

[Honza Bambas (:mayhemer)]

Comment on attachment 678233 [details] [diff] [review]
Create a CryptoTask API to simplify the creation of correct async crypto operations and add more utilities to ScopedNSSTypes.h [v2]

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

::: security/manager/ssl/src/CryptoTask.cpp
@@ +23,5 @@
> +{
> +  nsCOMPtr<nsIThread> thread;
> +  nsresult rv = NS_NewThread(getter_AddRefs(thread), this);
> +  if (thread) {
> +    NS_SetThreadName(thread, taskThreadName);

If there is a chance this can go in parallel, could we somehow use the pool naming here?

@@ +64,5 @@
> +                    "virtualDestroyNSSReference called off the main thread");
> +  if (!mReleasedNSSResources) {
> +    mReleasedNSSResources = true;
> +    ReleaseNSSResources();
> +  }

I think you should follow [1].  Adding the shutdown lock to dtor is a nice addition.


[1] http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/nsNSSShutDown.h#203

::: security/manager/ssl/src/CryptoTask.h
@@ +29,5 @@
> + * except CalculateResult might be skipped if NSS is shut down before it can
> + * be called; in that case ReleaseNSSResources will be called and then
> + * CallCallback will be called with an error code.
> + */
> +class CryptoTask : public nsRunnable, public nsNSSShutDownObject

each on a new line please.

Comment 4

[Honza Bambas (:mayhemer)]

Comment on attachment 678233 [details] [diff] [review]
Create a CryptoTask API to simplify the creation of correct async crypto operations and add more utilities to ScopedNSSTypes.h [v2]

Dropping r since I don't think the shutdown implementation is 100% correct.

Comment 5

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Create a CryptoTask API to simplify the creation of correct async crypto operations and add more utilities to ScopedNSSTypes.h [v3]

(In reply to Honza Bambas (:mayhemer) from comment #3)
> ::: security/manager/ssl/src/CryptoTask.cpp
> @@ +23,5 @@
> > +{
> > +  nsCOMPtr<nsIThread> thread;
> > +  nsresult rv = NS_NewThread(getter_AddRefs(thread), this);
> > +  if (thread) {
> > +    NS_SetThreadName(thread, taskThreadName);
> 
> If there is a chance this can go in parallel, could we somehow use the pool
> naming here?

Presumably, each different type of crypto task should have its own name. But, we'd have to extend the pooling API to support that. Right now, because of time constraints, I think it isn't worth doing that. But, that would not affect this patch anyway; it would affect the patches that use CryptoTask.

> 
> @@ +64,5 @@
> > +                    "virtualDestroyNSSReference called off the main thread");
> > +  if (!mReleasedNSSResources) {
> > +    mReleasedNSSResources = true;
> > +    ReleaseNSSResources();
> > +  }
> 
> I think you should follow [1].  Adding the shutdown lock to dtor is a nice
> addition.

The pattern in [1] doesn't make sense for this implementation because CryptoTask always makes sure that the NSS resources are released before the destructor is ever called. (This is asserted in the destructor.) Therefore, we don't need a "destructor-safe" cleanup operation. In fact, this is one of the key reasons why I created this helper class, to reduce the boilerplate that subclasses need to implement compared to normal nsNSSShutDownObject.

> [1]
> http://mxr.mozilla.org/mozilla-central/source/security/manager/ssl/src/
> nsNSSShutDown.h#203
> 
> ::: security/manager/ssl/src/CryptoTask.h
> @@ +29,5 @@
> > + * except CalculateResult might be skipped if NSS is shut down before it can
> > + * be called; in that case ReleaseNSSResources will be called and then
> > + * CallCallback will be called with an error code.
> > + */
> > +class CryptoTask : public nsRunnable, public nsNSSShutDownObject
> 
> each on a new line please.

Fixed.

Comment 6

[Honza Bambas (:mayhemer)]

Comment on attachment 680367 [details] [diff] [review]
Create a CryptoTask API to simplify the creation of correct async crypto operations and add more utilities to ScopedNSSTypes.h [v3]

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

r=honzab

Thanks.

Comment 7

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Attachment: Create a CryptoTask API to simplify the creation of correct async crypto operations and add more utilities to ScopedNSSTypes.h [v4]

I switched the usage of HASH_* to PK11_* because HASH_* functions not return error codes from all the operations, whereas PK11_* do, so I think the PK11_* functions are safer/better.

Comment 8

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ef6db4ced917

Comment 9

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Backed out of m-i: https://hg.mozilla.org/integration/mozilla-inbound/rev/58b9ad5a8043

Comment 10

[Alex Keybl [:akeybl]]

This bug has been called out as likely having risk to non-B2G platforms. Given that, marking as P1, and moving into the C2 milestone. We should prioritize this landing to mozilla-beta as soon as possible, to prevent late-breaking regressions to other platforms.

Comment 11

[Andrew Overholt [:overholt]]

Any updates here, Brian?

Comment 12

[JP Rosevear [:jpr]]

I think its unlikely we should take an NSS update at this late date.

Comment 13

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

(In reply to JP Rosevear [:jpr] from comment #12)
> I think its unlikely we should take an NSS update at this late date.

Please see bug 816392 comment 0. I have already attached the patch to backport the three *build-time-only* NSS patches to mozilla-beta, so that we will not have to update NSS in mozilla-beta.

Comment 14

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/84bf2e19c701

Comment 15

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Backed out: https://hg.mozilla.org/integration/mozilla-inbound/rev/5e09d599229c

Comment 16

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f24c48b21e3
https://hg.mozilla.org/releases/mozilla-aurora/rev/36f6c5a2c0d5
https://hg.mozilla.org/releases/mozilla-beta/rev/5c4883456c4e

Comment 17

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/9f24c48b21e3