Closed Bug 804663 Opened 8 years ago Closed 8 years ago

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

Categories

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

defect

Tracking

()

RESOLVED FIXED
B2G C2 (20nov-10dec)
blocking-basecamp +
Tracking Status
firefox18 --- fixed
firefox19 --- fixed

People

(Reporter: briansmith, Assigned: briansmith)

References

Details

Attachments

(1 file, 4 obsolete files)

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.
Assignee: nobody → bsmith
Blocks: 769856
Depends on: 804441
Priority: -- → P1
Target Milestone: --- → mozilla19
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.
Attachment #674284 - Attachment is obsolete: true
Attachment #677914 - Flags: review?(honzab.moz)
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.
Attachment #677914 - Attachment is obsolete: true
Attachment #677914 - Flags: review?(honzab.moz)
Attachment #678233 - Flags: review?(honzab.moz)
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 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.
Attachment #678233 - Flags: review?(honzab.moz)
(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.
Attachment #680367 - Flags: review?(honzab.moz)
Attachment #678233 - Attachment is obsolete: true
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.
Attachment #680367 - Flags: review?(honzab.moz) → review+
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.
Attachment #680367 - Attachment is obsolete: true
Attachment #682489 - Flags: review?(honzab.moz)
Attachment #682489 - Flags: review?(honzab.moz) → review+
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.
Target Milestone: mozilla19 → B2G C2 (20nov-10dec)
Any updates here, Brian?
Flags: needinfo?(bsmith)
I think its unlikely we should take an NSS update at this late date.
blocking-basecamp: + → ?
Flags: needinfo?(bsmith)
(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.
blocking-basecamp: ? → +
https://hg.mozilla.org/mozilla-central/rev/9f24c48b21e3
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
No longer blocks: 776060
You need to log in before you can comment on or make changes to this bug.