Start adding threading checks to nss_util.

Start adding threading checks to nss_util.

BUG=125848
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=234388

[mattm, 2013-11-08 22:40:02]

message: (cc:rsleevi as fyi)

I was talking with Ryan a bit about the races in nss_util accesses, and about
adding checks enforcing threading constraints.  We can't check the BrowserThread
directly due to being in crypto/, but we can use NonThreadSafe.

Some of the functions just do DVLOG instead of DCHECK since there are a lot of
call sites that need to be fixed.

[Ryan Sleevi, 2013-11-09 05:26:20]

could you use ThreadChecker (as a member var) instead of deriving from
NonThreadSafe?

On Fri, Nov 8, 2013 at 2:40 PM,  <mattm@chromium.org> wrote:
> Reviewers: wtc,
>
> Message:
> message: (cc:rsleevi as fyi)
>
> I was talking with Ryan a bit about the races in nss_util accesses, and
> about
> adding checks enforcing threading constraints.  We can't check the
> BrowserThread
> directly due to being in crypto/, but we can use NonThreadSafe.
>
> Some of the functions just do DVLOG instead of DCHECK since there are a lot
> of
> call sites that need to be fixed.
>
>
> Description:
> Start adding threading checks to nss_util.
>
> BUG=125848
>
> Please review this at https://codereview.chromium.org/64723006/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src
>
> Affected files (+43, -1 lines):
>   M crypto/nss_util.cc
>
>
> Index: crypto/nss_util.cc
> diff --git a/crypto/nss_util.cc b/crypto/nss_util.cc
> index
>
04080ed3afc41245e4fce341f88cf9280e20bf2d..ecc131cd58c65b855578b3f3ec5a3a092cf843c5
> 100644
> --- a/crypto/nss_util.cc
> +++ b/crypto/nss_util.cc
> @@ -24,6 +24,7 @@
>  #include <vector>
>
>  #include "base/debug/alias.h"
> +#include "base/debug/stack_trace.h"
>  #include "base/environment.h"
>  #include "base/file_util.h"
>  #include "base/files/file_path.h"
> @@ -34,6 +35,7 @@
>  #include "base/metrics/histogram.h"
>  #include "base/native_library.h"
>  #include "base/strings/stringprintf.h"
> +#include "base/threading/non_thread_safe.h"
>  #include "base/threading/thread_restrictions.h"
>  #include "build/build_config.h"
>
> @@ -213,10 +215,12 @@ void CrashOnNSSInitFailure() {
>    LOG(FATAL) << "nss_error=" << nss_error << ", os_error=" << os_error;
>  }
>
> -class NSSInitSingleton {
> +class NSSInitSingleton : public base::NonThreadSafe {
>   public:
>  #if defined(OS_CHROMEOS)
>    void OpenPersistentNSSDB() {
> +    DCHECK(CalledOnValidThread());
> +
>      if (!chromeos_user_logged_in_) {
>        // GetDefaultConfigDirectory causes us to do blocking IO on UI
> thread.
>        // Temporarily allow it until we fix http://crbug.com/70119
> @@ -233,6 +237,8 @@ class NSSInitSingleton {
>    }
>
>    void EnableTPMTokenForNSS() {
> +    DCHECK(CalledOnValidThread());
> +
>      // If this gets set, then we'll use the TPM for certs with
>      // private keys, otherwise we'll fall back to the software
>      // implementation.
> @@ -242,6 +248,8 @@ class NSSInitSingleton {
>    bool InitializeTPMToken(const std::string& token_name,
>                            int token_slot_id,
>                            const std::string& user_pin) {
> +    DCHECK(CalledOnValidThread());
> +
>      // If EnableTPMTokenForNSS hasn't been called, return false.
>      if (!tpm_token_enabled_for_nss_)
>        return false;
> @@ -281,6 +289,12 @@ class NSSInitSingleton {
>    }
>
>    void GetTPMTokenInfo(std::string* token_name, std::string* user_pin) {
> +    // TODO(mattm): Change to DCHECK when callers have been fixed.
> +    if (!CalledOnValidThread()) {
> +      DVLOG(1) << "Called on wrong thread.\n"
> +               << base::debug::StackTrace().ToString();
> +    }
> +
>      if (!tpm_token_enabled_for_nss_) {
>        LOG(ERROR) << "GetTPMTokenInfo called before TPM Token is ready.";
>        return;
> @@ -292,6 +306,12 @@ class NSSInitSingleton {
>    }
>
>    bool IsTPMTokenReady() {
> +    // TODO(mattm): Change to DCHECK when callers have been fixed.
> +    if (!CalledOnValidThread()) {
> +      DVLOG(1) << "Called on wrong thread.\n"
> +               << base::debug::StackTrace().ToString();
> +    }
> +
>      return tpm_slot_ != NULL;
>    }
>
> @@ -299,6 +319,8 @@ class NSSInitSingleton {
>    // id as an int. This should be safe since this is only used with chaps,
> which
>    // we also control.
>    PK11SlotInfo* GetTPMSlotForId(CK_SLOT_ID slot_id) {
> +    DCHECK(CalledOnValidThread());
> +
>      if (!chaps_module_)
>        return NULL;
>
> @@ -316,6 +338,8 @@ class NSSInitSingleton {
>
>
>    bool OpenTestNSSDB() {
> +    DCHECK(CalledOnValidThread());
> +
>      if (test_slot_)
>        return true;
>      if (!g_test_nss_db_dir.Get().CreateUniqueTempDir())
> @@ -325,6 +349,8 @@ class NSSInitSingleton {
>    }
>
>    void CloseTestNSSDB() {
> +    DCHECK(CalledOnValidThread());
> +
>      if (!test_slot_)
>        return;
>      SECStatus status = SECMOD_CloseUserDB(test_slot_);
> @@ -336,6 +362,12 @@ class NSSInitSingleton {
>    }
>
>    PK11SlotInfo* GetPublicNSSKeySlot() {
> +    // TODO(mattm): Change to DCHECK when callers have been fixed.
> +    if (!CalledOnValidThread()) {
> +      DVLOG(1) << "Called on wrong thread.\n"
> +               << base::debug::StackTrace().ToString();
> +    }
> +
>      if (test_slot_)
>        return PK11_ReferenceSlot(test_slot_);
>      if (software_slot_)
> @@ -344,6 +376,12 @@ class NSSInitSingleton {
>    }
>
>    PK11SlotInfo* GetPrivateNSSKeySlot() {
> +    // TODO(mattm): Change to DCHECK when callers have been fixed.
> +    if (!CalledOnValidThread()) {
> +      DVLOG(1) << "Called on wrong thread.\n"
> +               << base::debug::StackTrace().ToString();
> +    }
> +
>      if (test_slot_)
>        return PK11_ReferenceSlot(test_slot_);
>
> @@ -487,6 +525,10 @@ class NSSInitSingleton {
>                             base::TimeDelta::FromMilliseconds(10),
>                             base::TimeDelta::FromHours(1),
>                             50);
> +
> +    // It's safe to construct on any thread, since LazyInstance will
> prevent any
> +    // other threads from accessing until the constructor is done.
> +    DetachFromThread();
>    }
>
>    // NOTE(willchan): We don't actually execute this code since we leak NSS
> to
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[wtc, 2013-11-11 22:10:15]

Patch set 1 LGTM. Please make the change Ryan suggested.

https://codereview.chromium.org/64723006/diff/80001/crypto/nss_util.cc
File crypto/nss_util.cc (right):

https://codereview.chromium.org/64723006/diff/80001/crypto/nss_util.cc#newcod...
crypto/nss_util.cc:531: DetachFromThread();

If this can be called at the beginning of this function, we should do that. This
information should be more visible, and it will also protect us from early
returns.

[mattm, 2013-11-12 00:41:33]

On 2013/11/09 05:26:20, Ryan Sleevi (OoO until 11-18) wrote:
> could you use ThreadChecker (as a member var) instead of deriving from
> NonThreadSafe?

Done.

https://codereview.chromium.org/64723006/diff/80001/crypto/nss_util.cc
File crypto/nss_util.cc (right):

https://codereview.chromium.org/64723006/diff/80001/crypto/nss_util.cc#newcod...
crypto/nss_util.cc:531: DetachFromThread();
On 2013/11/11 22:10:15, wtc wrote:
> 
> If this can be called at the beginning of this function, we should do that.
This
> information should be more visible, and it will also protect us from early
> returns.

Done.

[commit-bot: I haz the power, 2013-11-12 00:52:18]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/mattm@chromium.org/64723006/190001

[wtc, 2013-11-12 02:05:47]

Patch set 2 LGTM.

[commit-bot: I haz the power, 2013-11-12 02:56:34]

Change committed as 234388